Note: NMP_CACHE_ID_TYPE_ADDRROUTE_BY_IFINDEX perhaps needs renaming.

+         .cmd_plobj_to_string_id             = (const char *(*) (const NMPlatformObject *obj, char *buf, gsize len)) nm_platform_qdisc_to_string,

".cmd_plobj_to_string_id" should only print the ID parts (in this case ifindex+handle). Only for routes this is identical to .cmd_plobj_to_string, because essentially all properties of the route are part of the ID.


 libnm-core/nm-setting-tc-config.c:621:25: error: unused variable 'names' [-Werror,-Wunused-variable]
                        gs_free const char **names = NULL;



+const NMPObject *nmp_object_stackinit_id_qdisc (NMPObject *obj);

the purpose of the nmp_object_stackinit_id_*() functions is to initialize an object on the stack with the ID attributes. Since the ID are ifindex+handle, these two arguments are required.

Of course, if you don't need nmp_object_stackinit_id_qdisc(), you can drop it.



+    NMPlatformError (*qdisc_get)   (NMPlatform *self,
+                                        int ifindex,
+NMPlatformError nm_platform_qdisc_add   (NMPlatform *self,
+                                     NMPNlmFlags flags,

indentation (2x)


qdisc_get() is unused.


can you implement cmd_obj_is_alive() to validate ifindex>0 && handle>=0? Possibly also require kind!=NULL, etc.


+nm_platform_qdisc_cmp (const NMPlatformQdisc *a, const NMPlatformQdisc *b)
+{
+    NM_CMP_SELF (a, b);
+    NM_CMP_FIELD_STR_INTERNED (a, b, kind);
+    NM_CMP_FIELD (a, b, ifindex);
+    NM_CMP_FIELD (a, b, addr_family);
+    NM_CMP_FIELD (a, b, handle);

can you move the checks for "handle" and "ifindex" before "kind"? As they are part of the ID and cheaper to evaluate, the should be checked first to abort the comparison early (in the common case, that we compare to different (by ID) objects)
For hash-update, that doesn't matter, as you always have to hash all properties.


The name NMP_CACHE_ID_TYPE_ADDRROUTE_BY_IFINDEX, nmp_lookup_init_addrroute(), etc. are no longer correct. Should find a better name and rename the ADDRROUTE part.


Why does nm_platform_qdisc_sync() delete all entries first, and then adds them. Maybe existing (matching) entries should be preserved.


   NMPObject *q = g_ptr_array_index (plat_qdiscs, i);
   NMPObject *q = g_ptr_array_index (known_qdiscs, i);
   ^ const



+    if (nlmsg_append (msg, &tcm, sizeof (tcm), NLMSG_ALIGNTO) < 0) {
+         g_warn_if_reached ();
+         goto nla_put_failure;

nlmsg_append() can only fail with ENOMEM, in which case NM is anyway hosed (because the next failing g_malloc() will crash the application). So, it likely will never fail. Still, you already have "goto nla_put_failure;" which also does g_return_val_if_reached().
Please drop the g_warn_if_reached ();, it's unnecessary



+    if (seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK)
+         return NM_PLATFORM_ERROR_SUCCESS;
+
+nla_put_failure:
+    nlmsg_free (msg);

leaks @msg. Use nm_auto_nlmsg.

Also, the nla_put_failure label is only used because the NLA_PUT_*() macros require it. Don't use if for your own purpose, like
+    if (nle < 0) {
+         _LOGE ("do-add-qdisc: failed sending netlink request \"%s\" (%d)",
+               nl_geterror (nle), -nle);



+         nm_utils_strbuf_append (&buf, &len, "%s", s);
nm_utils_strbuf_append_str()


_new_from_nl_qdisc() does not check that KIND is present.



/* XXX: Perhapse the NMPClass should decide this. */

I think this is fine as is, but could you move gmsg and tcmsg closer to where they are used, inside the if-else?




+         case RTM_GETQDISC:

are these GETQDISC actually used? When user-space makes a RTM_GETROUTE request, kernel still replies with a RTM_NEWROUTE.

$ ./src/platform/tests/monitor

<debug> [1511258618.2746] platform: signal: qdisc   added: mq dev 3 handle 0 parent ffffffff info 1
<debug> [1511258618.2747] platform: signal: qdisc changed: fq_codel dev 3 handle 0 parent 4 info 1
<debug> [1511258618.2747] platform: signal: qdisc changed: fq_codel dev 3 handle 0 parent 3 info 1
<debug> [1511258618.2748] platform: signal: qdisc changed: fq_codel dev 3 handle 0 parent 2 info 1


this seems wrong. Looks like the ID of a qdisk is more then just ifindex+handle.

(In reply to Thomas Haller from comment #2)
> +         .cmd_plobj_to_string_id             = (const char *(*) (const
> NMPlatformObject *obj, char *buf, gsize len)) nm_platform_qdisc_to_string,
> 
> ".cmd_plobj_to_string_id" should only print the ID parts (in this case
> ifindex+handle). Only for routes this is identical to .cmd_plobj_to_string,
> because essentially all properties of the route are part of the ID.

Fixed

>  libnm-core/nm-setting-tc-config.c:621:25: error: unused variable 'names'
> [-Werror,-Wunused-variable]
>                         gs_free const char **names = NULL;

Fixed.

> +const NMPObject *nmp_object_stackinit_id_qdisc (NMPObject *obj);
> 
> the purpose of the nmp_object_stackinit_id_*() functions is to initialize an
> object on the stack with the ID attributes. Since the ID are ifindex+handle,
> these two arguments are required.
> 
> Of course, if you don't need nmp_object_stackinit_id_qdisc(), you can drop
> it.

Dropped.

> +    NMPlatformError (*qdisc_get)   (NMPlatform *self,
> +                                        int ifindex,
> +NMPlatformError nm_platform_qdisc_add   (NMPlatform *self,
> +                                     NMPNlmFlags flags,
> 
> indentation (2x)

Fixed.

> qdisc_get() is unused.

Dropped.

> can you implement cmd_obj_is_alive() to validate ifindex>0 && handle>=0?
> Possibly also require kind!=NULL, etc.

Why?

> +nm_platform_qdisc_cmp (const NMPlatformQdisc *a, const NMPlatformQdisc *b)
> +{
> +    NM_CMP_SELF (a, b);
> +    NM_CMP_FIELD_STR_INTERNED (a, b, kind);
> +    NM_CMP_FIELD (a, b, ifindex);
> +    NM_CMP_FIELD (a, b, addr_family);
> +    NM_CMP_FIELD (a, b, handle);
> 
> can you move the checks for "handle" and "ifindex" before "kind"? As they
> are part of the ID and cheaper to evaluate, the should be checked first to
> abort the comparison early (in the common case, that we compare to different
> (by ID) objects)

Uh, done. I don't understand why are they cheaper to evaluate though?

> The name NMP_CACHE_ID_TYPE_ADDRROUTE_BY_IFINDEX,
> nmp_lookup_init_addrroute(), etc. are no longer correct. Should find a
> better name and rename the ADDRROUTE part.

Not fixed yet, I haven't come up with a better name yet.
Suggestions welcome.

> Why does nm_platform_qdisc_sync() delete all entries first, and then adds
> them. Maybe existing (matching) entries should be preserved.

For simplicity. What would be the advantage of keeping the entries?

>    NMPObject *q = g_ptr_array_index (plat_qdiscs, i);
>    NMPObject *q = g_ptr_array_index (known_qdiscs, i);
>    ^ const

Fixed.

> +    if (nlmsg_append (msg, &tcm, sizeof (tcm), NLMSG_ALIGNTO) < 0) {
> +         g_warn_if_reached ();
> +         goto nla_put_failure;
> 
> nlmsg_append() can only fail with ENOMEM, in which case NM is anyway hosed
> (because the next failing g_malloc() will crash the application). So, it
> likely will never fail. Still, you already have "goto nla_put_failure;"
> which also does g_return_val_if_reached().
> Please drop the g_warn_if_reached ();, it's unnecessary

Fixed.

> +    if (seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK)
> +         return NM_PLATFORM_ERROR_SUCCESS;
> +
> +nla_put_failure:
> +    nlmsg_free (msg);
> 
> leaks @msg. Use nm_auto_nlmsg.

Fixed.

> Also, the nla_put_failure label is only used because the NLA_PUT_*() macros
> require it. Don't use if for your own purpose, like
> +    if (nle < 0) {
> +         _LOGE ("do-add-qdisc: failed sending netlink request \"%s\" (%d)",
> +               nl_geterror (nle), -nle);

Fixed.

> +         nm_utils_strbuf_append (&buf, &len, "%s", s);
> nm_utils_strbuf_append_str()

Done.

> _new_from_nl_qdisc() does not check that KIND is present.

Fixed.

> /* XXX: Perhapse the NMPClass should decide this. */
> 
> I think this is fine as is, but could you move gmsg and tcmsg closer to
> where they are used, inside the if-else?

Done.

> +         case RTM_GETQDISC:
> 
> are these GETQDISC actually used? When user-space makes a RTM_GETROUTE
> request, kernel still replies with a RTM_NEWROUTE.

Dropped.

(In reply to Thomas Haller from comment #3)
> $ ./src/platform/tests/monitor
> 
> <debug> [1511258618.2746] platform: signal: qdisc   added: mq dev 3 handle 0
> parent ffffffff info 1
> <debug> [1511258618.2747] platform: signal: qdisc changed: fq_codel dev 3
> handle 0 parent 4 info 1
> <debug> [1511258618.2747] platform: signal: qdisc changed: fq_codel dev 3
> handle 0 parent 3 info 1
> <debug> [1511258618.2748] platform: signal: qdisc changed: fq_codel dev 3
> handle 0 parent 2 info 1
> 
> 
> this seems wrong. Looks like the ID of a qdisk is more then just
> ifindex+handle.

That was wrong. The index in fact is ifindex+parent.

Pushed a new version for review.

> > can you implement cmd_obj_is_alive() to validate ifindex>0 && handle>=0?
> > Possibly also require kind!=NULL, etc.
>
> Why?

because there are certain assumptions what makes the ID of this object. If the ID is in an odd state, the object should be thrown away. For example, it's not added to the cache.

For example,
https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/platform/nmp-object.c?id=8649fcf02ad0646ebbf2daea2ededd0ee4a5745a#n190
asserts nm_assert (obj_a->object.ifindex > 0);
That assert is only correct, because in the lines above we filter out objects that are not nmp_object_is_visible() (a non-alive object, is also not visible).
Because a NMPlatformIP4Route is not alive/visible without valid ifindex, the line is never reached on current master.
In your code, a NMPlatformQDisc object with ifindex -1 would trigger the assertion.
You could just drop the assertion and have the index also work for negative ifindex, but that seems strange to do.

> > (by ID) objects)
>
> Uh, done. I don't understand why are they cheaper to evaluate though?

NM_CMP_FIELD (a, b, ifindex); just does a != of two integers while NM_CMP_FIELD_STR*() calls strcmp(). The former is faster and it has no other downside to check the int first.
Ok, the change also affects how QDisc objects are sorted (which doesn't matter as you nowhere rely on a particular sort-order), but even for that argument it would make more sense to first sort by the properties of the primary ID so that two objects that differ only by a non-ID part sort beside each other.

> > Why does nm_platform_qdisc_sync() delete all entries first, and then adds
> > them. Maybe existing (matching) entries should be preserved.
> 
> For simplicity. What would be the advantage of keeping the entries?

Calling sync twice should do nothing. For routes for example, adding and readding a route was harmful, because kernel would flush some caches (forgot the details, there was a bug about that). For QDisc I figure it could be harmful to temporarily remove the a entry, because it disrupts it for a moment.

Especially, if you first add and then remove, then it's trivial to implement, because the NMPObjects are comparable and can be put to a hash table.



nm_platform_qdisc_to_string() should print *all* properties, it lacks at least addr_family.

+    }
+
+        return nm_platform_qdisc_sync (nm_device_get_platform (self), ip_ifindex,
+                                   qdiscs);


indentation.


why do you call tc_commit() in nm_device_bring_up(). Shouldn't it be more done along with ipx_config_merge_and_apply()?


+    if (ifindex) {

I think we should always check (ifindex > 0), because in the past some code set ifindex -1 to indicate invalid ifindex.

Also, tc_commit() uses ip_ifindex not ifindex.



     NMP_OBJECT_TYPE_IP6_ROUTE,
 
+    NMP_OBJECT_TYPE_QDISC,
     NMP_OBJECT_TYPE_LNK_GRE,

either drop the new line, or move QDISC into the other block. It's much more similar with the non-lnk objects.



+int _nm_utils_parse_tc_qdisc_handle (const char *str, GError **error);

is only used by libnm-core internally. It should not be in "nm-core-internal.h", this one is API that is accessible from libnm-core *and* src.
Instead, put it to one of the nm*-private.h headers.

ah, clients/common/nm-meta-setting-desc.c

+#include "nm-core-internal.h"


This is not allowed. clients/* can only use public API of libnm. nm-core-internal.h can't be used.

> > The name NMP_CACHE_ID_TYPE_ADDRROUTE_BY_IFINDEX,
> > nmp_lookup_init_addrroute(), etc. are no longer correct. Should find a
> > better name and rename the ADDRROUTE part.
>
> Not fixed yet, I haven't come up with a better name yet.
> Suggestions welcome.

let's do s/addrroute/object/ and s/ADDRROUTE/OBJECT/. Together with the by-ifindex suffix, that seems suitable to me.

(In reply to Thomas Haller from comment #6)
> +    }
> +
> +        return nm_platform_qdisc_sync (nm_device_get_platform (self),
> ip_ifindex,
> +                                   qdiscs);
> 
> 
> indentation.

Fixed.

> why do you call tc_commit() in nm_device_bring_up(). Shouldn't it be more
> done along with ipx_config_merge_and_apply()?

I wasn't too sure about this.

Not fixed yet, but I'll look into it right away.

> +    if (ifindex) {
> 
> I think we should always check (ifindex > 0), because in the past some code
> set ifindex -1 to indicate invalid ifindex.

Fixed.

> Also, tc_commit() uses ip_ifindex not ifindex.

in nm_device_bring_up(), just before the check quoted above:

  ifindex = nm_device_get_ip_ifindex (self);

>      NMP_OBJECT_TYPE_IP6_ROUTE,
>  
> +    NMP_OBJECT_TYPE_QDISC,
>      NMP_OBJECT_TYPE_LNK_GRE,
> 
> either drop the new line, or move QDISC into the other block. It's much more
> similar with the non-lnk objects.

Fixed.

> +int _nm_utils_parse_tc_qdisc_handle (const char *str, GError **error);
> 
> is only used by libnm-core internally. It should not be in
> "nm-core-internal.h", this one is API that is accessible from libnm-core
> *and* src.
> Instead, put it to one of the nm*-private.h headers.

libnm-core/nm-utils-private.h:#error "nm-utils-private.h" must not be used outside of libnm-core/. Do you want "nm-core-internal.h"?

(In reply to Thomas Haller from comment #8)
> > > The name NMP_CACHE_ID_TYPE_ADDRROUTE_BY_IFINDEX,
> > > nmp_lookup_init_addrroute(), etc. are no longer correct. Should find a
> > > better name and rename the ADDRROUTE part.
> >
> > Not fixed yet, I haven't come up with a better name yet.
> > Suggestions welcome.
> 
> let's do s/addrroute/object/ and s/ADDRROUTE/OBJECT/. Together with the
> by-ifindex suffix, that seems suitable to me.

Done.

The updated branch was pushed that also adds support for filters.

I'm keeping notes about unfinished stuff here:
https://public.etherpad-mozilla.org/p/lr-tc

My test case:

  # nmcli c add ifname dum0 type dummy \
      ipv4.method link-local ipv6.method link-local \
      tc.qdiscs 'ingress, root handle 1234 fq_codel' \
      tc.tfilters 'parent ffff matchall action simple sdata Input, parent 1234 matchall action simple sdata Output'

  # tc qdisc show dev dum0
  qdisc fq_codel 1234: root refcnt 2 limit 10240p flows 1024 quantum 1514 target 5.0ms interval 100.0ms memory_limit 32Mb ecn
  qdisc ingress ffff: parent ffff:fff1 ----------------
 
  # tc filter show dev dum0
  filter parent 1234: protocol all pref 49152 matchall
  filter parent 1234: protocol all pref 49152 matchall handle 0x1
    not_in_hw
          action order 1: Simple <Output>
           index 27 ref 1 bind 1
 
  # tc filter show dev dum0 parent ffff:
  filter protocol all pref 49152 matchall
  filter protocol all pref 49152 matchall handle 0x1
    not_in_hw
          action order 1: Simple <Input>
           index 26 ref 1 bind 1

./tools/run-nm-test.sh -m -v src/settings/plugins/keyfile/tests/test-keyfile

reports leaks



nm_platform_*_add() should log a message like the other add functions do. It's helpful that all functions that mangle the platform log a message *before* they do it. (Extending contrib/scripts/NM-log should highlight the message would be great!).

For *_delete() the same, but I added a commit "platform: merge nm_platform_*_delete() delete functions" which does it.



_supported_cache_ids_qdisc and _supported_cache_ids_tfilter and _supported_cache_ids_ipx_address should be merge (in _supported_cache_ids_object)?


> platform/tests: tests qdisc caching behavior
    
NMPObject *obj1a = nmp_object_new (NMP_OBJECT_...

leaks. nm_auto_nmpobj?



I though the reason for making the libnm objects mutable is that it has may have additional attributes that are not fixed at compile-time (key-value pairs, like route attributes). Ok, that's only the case for NMTCAction.

If the object is mutable, do we need so many arguments to the _new() function?

If the objects like NMTCQdisc and NMTCFilter have no such attributes, do they need to be mutable?




+NMTCQdisc *nm_setting_tc_config_get_qdisc             (NMSettingTCConfig *setting,
+                                                       int idx);
+void       nm_setting_tc_config_remove_qdisc          (NMSettingTCConfig *setting,
+                                                       int idx);

idx should be an uint.




+              g_variant_builder_add (&qdisc_builder, "{sv}", "family",
+                                     g_variant_new_uint32 (nm_tc_qdisc_get_family (qdisc)));

the family is a signed integer.



+ * nm_tc_qdisc_set_info:
+ * @qdisc: the #NMTCQdisc
+ * @info: XXX
+ *
+ * XXX
+ *



»···qdisc->kind = g_intern_string (kind);

kind is some user provided string. It cannot be interned, because it can never be removed from the cache again. Also, at some places you assume that ->kind is != NULL, e.g. _nm_utils_string_append_tc_qdisc_rest()
Either, ensure that it's not possible to construct a NMTCQdisc without a kind, or you must anticipate that it may be missing.



>> libnm-core/tests: test NMSettingTCConfig qdisc support
    
nmtst_assert_success (nm_connection_replace_settings (connection2, dbus, &error), error);

This doesn't work. nmtst_assert_success() is a macro, and accesses error first. Even if it wouldn't be a macro, then I don't think that this is guaranteed to work by C standard.


Btw, I am not convinced that we should add new test programs. Or if you add one, then please call it libnm-core/tests/test-setting, so that it's not specifically for tc setting. One test program can contain a multitude of tests, instead of having many programs with few tests. The only argument for more programs would be that you can run them in parallel. But we already have a decent set of programs, so that if you do `make -j 5 check`, several tests run in parallel already. Adding more programs, just makes it slower in practice.
I think we should merge all test-setting-*.c files in one, but for now don't add new ones.



>> XXX libnm-core: add functionality for dealing with tc-style qdisc specifiers
    
_nm_utils_string_append_tc_qdisc_rest() concatenates @kind without checking that it might have a space. A string-representation must be able to serialize/deserialize everything that can be set in the API.
That is, either 
 - it is not possible to create an NMTCQdisc instance with such kind and
   nm_tc_qdisc_new() fails.
 - nm_utils_tc_qdisc_to_str() fails with an error. When following this route, then at least nm_connection_verify() must reject all NMTCQdisc instances that can't be serialized. Otherwise, you end up in a situation where a client specifies a connection that verifies, but storing to disc fails.
 - nm_utils_tc_qdisc_to_str() must escape or quote special values correctly.


I am really convinced about this iproute2 syntax. We already have nm_utils_parse_variant_attributes() which defines a format suitable for routes (key=value). Why do we need yet another string format (key value), which doesn't even support special characters like whitespace. nm_utils_parse_variant_attributes() at least supports backslash escaping. Ok, nmcli's _set_fcn_ip4_config_routes (ARGS_SET_FCN) does not support escaping, which is sad. nmcli handling should really be fixed there. For new code, let's make a string format that can handle all values.

Also, nm_utils_parse_variant_attributes() implements a general (reusable) mechanism, while NMVariantAttributeSpec describes the details. Based on NMVariantAttributeSpec one could implement tab completion for routes and tc alike (yes, it doesn't exist yet, but it's doable).
Your implementation does not separate the description from the format, hence, to implement tab-completion we would have to implement it specifically for tc.

Ok, this comparison with nm_utils_parse_variant_attributes() is not fair, because that for route attributes the attributes are some strings. On the other hand, all properties of NMTCQdisc are known statically. Dunno



+    str_clean = g_strstrip (g_strdup (str));
+    actionv = _nm_utils_strsplit_set (str_clean, " \t", 0);

_nm_utils_strsplit_set() should be dropped (entirely). Use nm_utils_strsplit_set() instead.





+DEFINE_REMOVER_INDEX_OR_VALUE (_remove_fcn_tc_config_tfilters,
+                               NM_SETTING_TC_CONFIG,
+                               nm_setting_tc_config_get_num_tfilters,
+                               nm_setting_tc_config_remove_tfilter,
+                               _validate_and_remove_tc_tfilter)

Wish we would clean this up and not add more of this (TODO).




I still think nm_platform_qdisc_sync() should not be like this. Existing object should not be touched.



Pushed a few commits.

(In reply to Thomas Haller from comment #10)
> ./tools/run-nm-test.sh -m -v src/settings/plugins/keyfile/tests/test-keyfile
> 
> reports leaks

Fixed.

> nm_platform_*_add() should log a message like the other add functions do.
> It's helpful that all functions that mangle the platform log a message
> *before* they do it. (Extending contrib/scripts/NM-log should highlight the
> message would be great!).

Fixed.

> For *_delete() the same, but I added a commit "platform: merge
> nm_platform_*_delete() delete functions" which does it.

Applied.

> _supported_cache_ids_qdisc and _supported_cache_ids_tfilter and
> _supported_cache_ids_ipx_address should be merge (in
> _supported_cache_ids_object)?

Done.

> > platform/tests: tests qdisc caching behavior
>     
> NMPObject *obj1a = nmp_object_new (NMP_OBJECT_...
> 
> leaks. nm_auto_nmpobj?

Fixed.

> I though the reason for making the libnm objects mutable is that it has may
> have additional attributes that are not fixed at compile-time (key-value
> pairs, like route attributes). Ok, that's only the case for NMTCAction.
> 
> If the object is mutable, do we need so many arguments to the _new()
> function?
> 
> If the objects like NMTCQdisc and NMTCFilter have no such attributes, do
> they need to be mutable?

Yes. They don't have attributes currently, but eventually will have. They all need to be mutable.

I've now changed them to take only the mandatory arguments in constructor.

> +NMTCQdisc *nm_setting_tc_config_get_qdisc             (NMSettingTCConfig
> *setting,
> +                                                       int idx);
> +void       nm_setting_tc_config_remove_qdisc          (NMSettingTCConfig
> *setting,
> +                                                       int idx);
> 
> idx should be an uint.

Fixed.

> +              g_variant_builder_add (&qdisc_builder, "{sv}", "family",
> +                                     g_variant_new_uint32
> (nm_tc_qdisc_get_family (qdisc)));
> 
> the family is a signed integer.

Fixed.

> + * nm_tc_qdisc_set_info:
> + * @qdisc: the #NMTCQdisc
> + * @info: XXX
> + *
> + * XXX
> + *

Dropped.

> »···qdisc->kind = g_intern_string (kind);
> 
> kind is some user provided string. It cannot be interned, because it can
> never be removed from the cache again. Also, at some places you assume that
> ->kind is != NULL, e.g. _nm_utils_string_append_tc_qdisc_rest()
> Either, ensure that it's not possible to construct a NMTCQdisc without a
> kind, or you must anticipate that it may be missing.

(This still needs fixing)

> >> libnm-core/tests: test NMSettingTCConfig qdisc support
>     
> nmtst_assert_success (nm_connection_replace_settings (connection2, dbus,
> &error), error);
> 
> This doesn't work. nmtst_assert_success() is a macro, and accesses error
> first. Even if it wouldn't be a macro, then I don't think that this is
> guaranteed to work by C standard.

Uh, you're right.

I always thought the function arguments are evaluated left to right -- the same as for the comma operator.


> Btw, I am not convinced that we should add new test programs. Or if you add
> one, then please call it libnm-core/tests/test-setting, so that it's not
> specifically for tc setting. One test program can contain a multitude of
> tests, instead of having many programs with few tests. The only argument for
> more programs would be that you can run them in parallel. But we already
> have a decent set of programs, so that if you do `make -j 5 check`, several
> tests run in parallel already. Adding more programs, just makes it slower in
> practice.
> I think we should merge all test-setting-*.c files in one, but for now don't
> add new ones.

Merged the tests together.

> >> XXX libnm-core: add functionality for dealing with tc-style qdisc specifiers
>     
> _nm_utils_string_append_tc_qdisc_rest() concatenates @kind without checking
> that it might have a space. A string-representation must be able to
> serialize/deserialize everything that can be set in the API.
> That is, either 
>  - it is not possible to create an NMTCQdisc instance with such kind and
>    nm_tc_qdisc_new() fails.
>  - nm_utils_tc_qdisc_to_str() fails with an error. When following this
> route, then at least nm_connection_verify() must reject all NMTCQdisc
> instances that can't be serialized. Otherwise, you end up in a situation
> where a client specifies a connection that verifies, but storing to disc
> fails.
>  - nm_utils_tc_qdisc_to_str() must escape or quote special values correctly.
> 
> 
> I am really convinced about this iproute2 syntax. We already have
> nm_utils_parse_variant_attributes() which defines a format suitable for
> routes (key=value). Why do we need yet another string format (key value),
> which doesn't even support special characters like whitespace.
> nm_utils_parse_variant_attributes() at least supports backslash escaping.
> Ok, nmcli's _set_fcn_ip4_config_routes (ARGS_SET_FCN) does not support
> escaping, which is sad. nmcli handling should really be fixed there. For new
> code, let's make a string format that can handle all values.
> 
> Also, nm_utils_parse_variant_attributes() implements a general (reusable)
> mechanism, while NMVariantAttributeSpec describes the details. Based on
> NMVariantAttributeSpec one could implement tab completion for routes and tc
> alike (yes, it doesn't exist yet, but it's doable).
> Your implementation does not separate the description from the format,
> hence, to implement tab-completion we would have to implement it
> specifically for tc.
> 
> Ok, this comparison with nm_utils_parse_variant_attributes() is not fair,
> because that for route attributes the attributes are some strings. On the
> other hand, all properties of NMTCQdisc are known statically. Dunno

I've added commits that make use of NMVariantAttributeSpec. They may eventually be folded in to preceeding commits.

That fixes the quoting thing.

As for the iproute syntax, I believe it is by far the most favorable way to serialize the objects. It is well known and will ease interoperability with the legacy network service. (I assume we'll have to extend the ifcfg plugin with tc support, given we don't support carrying over the ifcfg connections to keyfile when tc setting is added and the users sort of expect the ifcfg files to be present).

> +    str_clean = g_strstrip (g_strdup (str));
> +    actionv = _nm_utils_strsplit_set (str_clean, " \t", 0);
> 
> _nm_utils_strsplit_set() should be dropped (entirely). Use
> nm_utils_strsplit_set() instead.

Done.

> +DEFINE_REMOVER_INDEX_OR_VALUE (_remove_fcn_tc_config_tfilters,
> +                               NM_SETTING_TC_CONFIG,
> +                               nm_setting_tc_config_get_num_tfilters,
> +                               nm_setting_tc_config_remove_tfilter,
> +                               _validate_and_remove_tc_tfilter)
> 
> Wish we would clean this up and not add more of this (TODO).

I didn't do anything about this.

> I still think nm_platform_qdisc_sync() should not be like this. Existing
> object should not be touched.

(This still needs fixing).

> Pushed a few commits.

Thanks applied.

Pushed an updated branch.

Pushed another branch, I believe I addressed the remaining issues.

got merged. Closing.