GNOME Bugzilla – Bug 754433
[review] vlan: improve assuming VLAN connections (flags, prio mappings) and some other fixes
Last modified: 2015-11-02 13:12:37 UTC
VLAN connections are not correctly assumed when flags or priority mappings are set on a VLAN device and/or in a VLAN connection profile.
I have pushed a branch with fixes for VLAN - jk/vlan-assume-and-fixes * adds nm_platform_set_vlan_flags() and sets the flags when activating connection * reads ingress/eggress mappings and adds them to the generated connection when assuming connections * reads GVRP flag from GVRP= variable in ifcfg-rh plugin to be compatible with Fedora/RHEL initscripts
> platform: get VLAN flags and apply them to the generated connection 'make check' fails for me with error: test-link.c:280:test_software: assertion failed (vlan_flags == VLAN_FLAGS): (1 == 0) > platform: get ingress/egress mapping and apply them to generated connection + for (i = 0; i < size; i++) { + nm_setting_vlan_add_priority (s_vlan, NM_VLAN_EGRESS_MAP, egress_map_from[i], egress_map_to[i]); + } Unneeded braces here. + if (!obj->vlan_egress_map_from && e_size) + obj->vlan_egress_map_from = g_slice_alloc (sizeof (guint32) * e_size); + if (!obj->vlan_egress_map_to && e_size) + obj->vlan_egress_map_to = g_slice_alloc (sizeof (guint32) * e_size); Don't those buffers need a realloc if e_size changes? And also to be freed somewhere? The rest looks good to me.
(In reply to Beniamino Galvani from comment #2) > > platform: get VLAN flags and apply them to the generated connection > > 'make check' fails for me with error: > > test-link.c:280:test_software: assertion failed (vlan_flags == > VLAN_FLAGS): (1 == 0) > Fixed by a new commit "platform: fix adding VLAN flags" > > platform: get ingress/egress mapping and apply them to generated connection > > + for (i = 0; i < size; i++) { > + nm_setting_vlan_add_priority (s_vlan, NM_VLAN_EGRESS_MAP, > egress_map_from[i], egress_map_to[i]); > + } > > Unneeded braces here. Fixed. > > + if (!obj->vlan_egress_map_from && e_size) > + obj->vlan_egress_map_from = g_slice_alloc (sizeof (guint32) > * e_size); > + if (!obj->vlan_egress_map_to && e_size) > + obj->vlan_egress_map_to = g_slice_alloc (sizeof (guint32) * > e_size); > > Don't those buffers need a realloc if e_size changes? And also to be freed > somewhere? > Sure, we need to release memory somehow. Unfortunately, I was not able to untangle how initialization/destroy of the objects actually work. Ideally, we would have a static array, but we do not know the maximum for egress maps.
>> platform: add nm_platform_set_vlan_flags() function The behavior of libnl is correct IMO. In vlan_set_flags(), you don't need set_type("vlan"). That is set by build_rtnl_link (ifindex, NULL, NM_LINK_TYPE_VLAN); looking at vlan_add(), it translates the flags: guint kernel_flags = 0; if (vlan_flags & NM_VLAN_FLAG_REORDER_HEADERS) kernel_flags |= VLAN_FLAG_REORDER_HDR; if (vlan_flags & NM_VLAN_FLAG_GVRP) kernel_flags |= VLAN_FLAG_GVRP; if (vlan_flags & NM_VLAN_FLAG_LOOSE_BINDING) kernel_flags |= VLAN_FLAG_LOOSE_BINDING; As our NMVlanFlags has actually identical values to the kernel values, let's be consistent about it and use everywhere our NMVlanFlags, not the kernel flags. Also, add a code comment to NMVlanFlags about that? How about having a define for all_flags, because it's redeclared in a later commit (again as NMVlanFlags type)? >> vlan: set flags for VLAN device when activating a VLAN connection Patch LGTM, but maybe we should refactor the code to set the VLAN properties... currently configuring VLAN needs 1 (vlanflags) + num_priorities(EGRESS) + num_priorities(INGRESS) netlink message -- then the same number of responses + cache-updates + link-changed-signals. It could all be done with one single nm_platform_vlan_set() call. >> platform: fix adding VLAN flags reuse a define for all_flags? >> platform: get VLAN flags and apply them to the generated connection nm_platform_vlan_get_info(): flags argument should be of type NMVlanFlags and same for NMPlatformLink.vlan_flags. + if (flags != nm_setting_vlan_get_flags (s_vlan)) + g_object_set (s_vlan, NM_SETTING_VLAN_FLAGS, flags, NULL); cast flags to (NMVlanFlags). The field NMFakePlatformLink.vlan_flags is wrong. Use NMFakePlatformLink.link.vlan_flags instead... Hm, guess we could also drop vlan_id and other fields there... _nmp_vt_cmd_plobj_init_from_nl_link: if (link_cached) { obj->vlan_id = link_cached->link.vlan_id; obj->vlan_flags = link_cached->link.vlan_flags; } vlan_get_info(): consistency about types. Currently NMPlatformLink.vlan_flags is guint32 (I suggest it to be NMVlanFlags). Anyway, @f and @flags should be of the same type. src/platform/nm-platform.c: It's very important to update nm_platform_link_cmp() too. Also, let's print it in nm_platform_link_to_string(). >> platform: get ingress/egress mapping and apply them to generated connection The advantage of having the new fields in the public NMPlatformLink structure is that you don't need any accessors like nm_platform_vlan_get_ingress_map(). The user can just: NMPlatformLink *plink; plink = nm_platform_link_get (NM_PLATFORM_GET, ifindex); if (plink) { // use plink->vlan_ingress_map directly. } Arguably, that reasoning makes some functions like nm_platform_vlan_get_info(), as is, obsolete. OTOH, if you use allocated fields, you *must* have accessors like nm_platform_vlan_get_egress_map() and the fields must be private. IOW, the public struct NMPlatformLink *must not* contain non-static fields (that require deep-copy). Otherwise a user might do: NMPlatformLink *my_copy = NULL, *plink; plink = nm_platform_link_get (NM_P_GET, ifindex); if (plink) my_copy = g_memdup (plink, sizeof (*plink)); ... # much later, an access to my_copy->vlan_egress_map_from might cause a crash # because we didn't do a deep copy. It's hard to get this right, so # just have no malloc'd pointers in the public struct. So, either have the fields somehow static, or move (at least the non-static fields) to NMPObjectLink. If you do that, you must properly extend _vt_cmd_obj_copy_link(), _vt_cmd_obj_dispose_link(), _vt_cmd_obj_equal_link(). If you extend the public structure NMPlatformLink, you must always update nm_platform_link_cmp(). It'd be nice to have a way to print those fields. Maybe extend nm_platform_link_to_string() too? How about you extend nm_platform_vlan_get_info() to also lookup the egress and ingress map, instead of adding two new functions (which involves more typing and two extra cache-lookups). Ok, the argument list for nm_platform_vlan_get_info() gets long, but that is the only downside there.
Oh, how about also supporting the new VLAN flag VLAN_FLAG_MVRP?
I think fetching the vlan properties should go along with bug 754570.
> platform: get ingress/egress mapping and apply them to generated connection In _nmp_vt_cmd_plobj_init_from_nl_link(), can the read egress table size be different than the existing size? What if obj->vlan_egress_map_from is already allocated, but now e_size is larger than it was before? Also, should vlan_get_egress_map() make the map_from/map_to arguments pointers to const arrays? Callers shouldn't be able to change the internal arrays.
(In reply to Thomas Haller from comment #4) > >> platform: add nm_platform_set_vlan_flags() function > In vlan_set_flags(), you don't need set_type("vlan"). That is set by > build_rtnl_link (ifindex, NULL, NM_LINK_TYPE_VLAN); > Fixed. > looking at vlan_add(), it translates the flags: > guint kernel_flags = 0; > if (vlan_flags & NM_VLAN_FLAG_REORDER_HEADERS) > kernel_flags |= VLAN_FLAG_REORDER_HDR; > if (vlan_flags & NM_VLAN_FLAG_GVRP) > kernel_flags |= VLAN_FLAG_GVRP; > if (vlan_flags & NM_VLAN_FLAG_LOOSE_BINDING) > kernel_flags |= VLAN_FLAG_LOOSE_BINDING; > > As our NMVlanFlags has actually identical values to the kernel values, let's > be consistent about it and use everywhere our NMVlanFlags, not the kernel > flags. > Also, add a code comment to NMVlanFlags about that? > Left it as it is now. > How about having a define for all_flags, because it's redeclared in a later > commit (again as NMVlanFlags type)? > All flags are defined with the last commit. > >> vlan: set flags for VLAN device when activating a VLAN connection > > Patch LGTM, but maybe we should refactor the code to set the VLAN > properties... > currently configuring VLAN needs 1 (vlanflags) + num_priorities(EGRESS) + > num_priorities(INGRESS) netlink message -- then the same number of responses > + cache-updates + link-changed-signals. > It could all be done with one single nm_platform_vlan_set() call. > Leaving performance tuning for later updates if we see it makes sense. > >> platform: fix adding VLAN flags > > reuse a define for all_flags? > Done. > >> platform: get VLAN flags and apply them to the generated connection > > nm_platform_vlan_get_info(): flags argument should be of type NMVlanFlags > and same for NMPlatformLink.vlan_flags. > > + if (flags != nm_setting_vlan_get_flags (s_vlan)) > + g_object_set (s_vlan, NM_SETTING_VLAN_FLAGS, flags, NULL); > > cast flags to (NMVlanFlags). > Done. > > The field NMFakePlatformLink.vlan_flags is wrong. Use > NMFakePlatformLink.link.vlan_flags instead... > Hm, guess we could also drop vlan_id and other fields there... > Done. > _nmp_vt_cmd_plobj_init_from_nl_link: > > if (link_cached) { > obj->vlan_id = link_cached->link.vlan_id; > obj->vlan_flags = link_cached->link.vlan_flags; > } > > vlan_get_info(): consistency about types. Currently > NMPlatformLink.vlan_flags is guint32 (I suggest it to be NMVlanFlags). > Anyway, @f and @flags should be of the same type. > > src/platform/nm-platform.c: > > It's very important to update nm_platform_link_cmp() too. Also, let's print > it in nm_platform_link_to_string(). > Done. > > >> platform: get ingress/egress mapping and apply them to generated connection > > The advantage of having the new fields in the public NMPlatformLink > structure is that you don't need any accessors like > nm_platform_vlan_get_ingress_map(). > > The user can just: > > NMPlatformLink *plink; > > plink = nm_platform_link_get (NM_PLATFORM_GET, ifindex); > if (plink) { > // use plink->vlan_ingress_map directly. > } > > > Arguably, that reasoning makes some functions like > nm_platform_vlan_get_info(), as is, obsolete. > > > OTOH, if you use allocated fields, you *must* have accessors like > nm_platform_vlan_get_egress_map() and the fields must be private. > IOW, the public struct NMPlatformLink *must not* contain non-static fields > (that require deep-copy). Otherwise a user might do: > > NMPlatformLink *my_copy = NULL, *plink; > > plink = nm_platform_link_get (NM_P_GET, ifindex); > if (plink) > my_copy = g_memdup (plink, sizeof (*plink)); > > ... > > # much later, an access to my_copy->vlan_egress_map_from might cause a > crash > # because we didn't do a deep copy. It's hard to get this right, so > # just have no malloc'd pointers in the public struct. > > > So, either have the fields somehow static, or move (at least the non-static > fields) to NMPObjectLink. > > > > If you do that, you must properly extend _vt_cmd_obj_copy_link(), > _vt_cmd_obj_dispose_link(), _vt_cmd_obj_equal_link(). > > If you extend the public structure NMPlatformLink, you must always update > nm_platform_link_cmp(). > > It'd be nice to have a way to print those fields. Maybe extend > nm_platform_link_to_string() too? > Done. First, I used static arrays for egress (for max 64 mappings), later commit reworks that for dynamic arrays. > > How about you extend nm_platform_vlan_get_info() to also lookup the egress > and ingress map, instead of adding two new functions (which involves more > typing and two extra cache-lookups). > Ok, the argument list for nm_platform_vlan_get_info() gets long, but that is > the only downside there. I do not like that idea. (In reply to Thomas Haller from comment #5) > Oh, how about also supporting the new VLAN flag VLAN_FLAG_MVRP? Done in the last commit. (In reply to Dan Williams from comment #7) > > platform: get ingress/egress mapping and apply them to generated connection > > In _nmp_vt_cmd_plobj_init_from_nl_link(), can the read egress table size be > different than the existing size? What if obj->vlan_egress_map_from is > already allocated, but now e_size is larger than it was before? > Fixed. > Also, should vlan_get_egress_map() make the map_from/map_to arguments > pointers to const arrays? Callers shouldn't be able to change the internal > arrays. Done. Repushed the branch (jk/vlan-assume-and-fixes) with the fixes and put a few new commits on the top.
Branch th/platform-parse-nl-bgo754570 completely refactors handling link-specific data. That branch is still WIP, but the relevant parts are ready. Could you please rebase this branch on top of the other?
+ flags = nm_setting_vlan_get_flags (s_vlan); + nm_platform_vlan_set_flags (NM_PLATFORM_GET, ifindex, flags); + num = nm_setting_vlan_get_num_priorities (s_vlan, NM_VLAN_INGRESS_MAP); for (i = 0; i < num; i++) { if (nm_setting_vlan_get_priority (s_vlan, NM_VLAN_INGRESS_MAP, i, &from, &to)) nm_platform_vlan_set_ingress_map (NM_PLATFORM_GET, ifindex, from, to); } num = nm_setting_vlan_get_num_priorities (s_vlan, NM_VLAN_EGRESS_MAP); for (i = 0; i < num; i++) { if (nm_setting_vlan_get_priority (s_vlan, NM_VLAN_EGRESS_MAP, i, &from, &to)) nm_platform_vlan_set_egress_map (NM_PLATFORM_GET, ifindex, from, to); } I don't think this actually works and is at probalby broken since platform-refactoring. nm_platform_vlan_set_egress_map() and nm_platform_vlan_set_ingress_map() creates a new rtnl_link object (with all fields cleared), and then sends it to kernel. Aside from being horribly inefficient, I think it will not work and only the very last value will be set. (if it actually works, a test would be appropriate where more then one in/egress properities are set). Why not make one single function: nm_platform_link_set_lnk_vlan (NMPlatform *platform, int ifindex, guint32 flags_mask, guint32 flags_set, ... entire-ingress-options, ... entire-egress-options); Passing flags_mask==0, means, not to set the flags at all, but only other properties. (note that the function name mirrors nm_platform_link_get_lnk_vlan() -- I dislike that we have nm_platform_vlan* names, instead of nm_platform_link*vlan*).
I merged the first part of the branch to master as: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=d07cf5c96cfe2deb185069c974d330db5699c448
Created attachment 314248 [details] [review] [PATCH] unmerged parts of jk/vlan-assume-and-fixes The remaining parts conflict with th/platform-parse-nl-bgo754570 (bug 754570), which heavily refactors platform. I will reimplement the functionality of this branch there. This patch might still be useful for backporting to older branches (which might need parts of this, but not the full th/platform-parse-nl-bgo754570). I park it here for history. The patch applies on master, commit d07cf5c96cfe2deb185069c974d330db5699c448
Closing BZ. Please reopen, if you want to resurrect the branch (patch from comment 12).
Addendum: Merged th/platform-parse-nl-bgo754570 branch. The remaining issues are now fixed on master. The most relevant commits are: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=a5ea141956eea99f0882061d12d229d36c9ea684 http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=5feda42813ddf5d51adf51c4e95cea51748a48fd