GNOME Bugzilla – Bug 707617
[review thaller/IP6Config_replace] core: update existing IP6Config of device instead of replacing it
Last modified: 2013-09-25 21:15:49 UTC
Please review the following branch thaller/IP6Config_replace http://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=thaller/IP6Config_replace The first commit adds some functions that are used later. Actually, only the IP6 version is used up to now. So maybe drop the IP4 version? The second commit adds nm_ip6_config_replace The third commit then changes the behaviour how the IP6Config of a device gets updated. The commit message sums up the purpose of all of this: When the IP6Config changes, a new configuration gets assembled. Before, whenever the new configuration was different than the current one, the IP6Config of the device was completely replaced. This also meant, that the old dbus IP6Config object was removed and the new one was exported. Now instead of recreating a new configuration, it updates the existing (already exported) configuration in place.
The same issues with regenerating the config exist with the IP4Config object, it's just that the kernel doesn't make routing changes as often so we don't see the escalating object path number. I think the same changes that you're making for IP6 should also be made for IP4. Also, is there any reason to make the return value from the CMP functions an int? If we're not using these functions for sorting or anything, then it might jsut be simpler (and less code) to make them do a direct comparison and return a boolean instead. Next, in nm_ip6_config_replace(), we can actually do this: NMIP6ConfigPrivate *dst_priv = NM_IP6_CONFIG_GET_PRIVATE (dst); NMIP6ConfigPrivate *src_priv = NM_IP6_CONFIG_GET_PRIVATE (src); if (src_priv->never_default != dst_priv->never_default) { dst_priv->never_default = src_priv->never_default; has_changes = TRUE; } if (!IN6_ARE_ADDR_EQUAL (&src_priv->gateway, &dst_priv->gateway)) { nm_ip6_config_set_gateway (dst, &src_priv->gateway); has_changes = TRUE; } Since this is the same class, we can access the private data from both instances, and we don't need to use temporary variables or accessors at all. Obviously that only works for things that are easily accessible, eg I don't think we should do this for address comparison because it would be more code. But in the end, both ways are equivalent, so it's your choice which way to go. Also, I'd freeze + thaw the GObject property notifications at the start and end, using g_object_freeze_notify() and g_object_thaw_notify(), so that identical property notifications are condensed into a single one. Otherwise clients will get multiple notifications each time an address is added or removed, when we really just want one for the entire 'addresses' property. Lastly, in the last patch, we only need to notify of the new IP6Config object in nm_device_set_ip6_config() if the actual *object* has changed. But we still need to emit the signals[IP6_CONFIG_CHANGED] since the Policy listens to that for internal changes to the object. Which is somewhat wierd, but we'll fix that later. So something like: if (priv->ip6_config != old_config) g_object_notify (G_OBJECT (self), NM_DEVICE_IP6_CONFIG); if (has_changes) g_signal_emit (self, signals[IP6_CONFIG_CHANGED], 0, priv->ip6_config, old_config); g_clear_object (&old_config); should be the right thing. That ensure that we only emit the D-Bus signal when the actual object changes (clients need to listen to updates on each individual property to get changes to addresses or routes, but that has always been the case), and that we emit the internal IP6_CONFIG_CHANGED signal whenever the object properties changed.
(In reply to comment #1) I pushed some fixup!/squash! commits in response to comment #1. See thaller/IP6Config_replace > The same issues with regenerating the config exist with the IP4Config object, > it's just that the kernel doesn't make routing changes as often so we don't see > the escalating object path number. I think the same changes that you're making > for IP6 should also be made for IP4. Done. One question is about _update_ip4_address. It does not get called when new_address is NULL. Maybe it should? > Also, is there any reason to make the return value from the CMP functions an > int? If we're not using these functions for sorting or anything, then it might > jsut be simpler (and less code) to make them do a direct comparison and return > a boolean instead. IMO a *_cmp method is more useful then *_is_equal, because it can also be used for ordering. It's true, it is not needed anywhere ATM. But I think the compare pattern is well known (strcmp, memcmp) and not significantly more complex. Do you still want me to change it? > Next, in nm_ip6_config_replace(), we can actually do this: > > NMIP6ConfigPrivate *dst_priv = NM_IP6_CONFIG_GET_PRIVATE (dst); > NMIP6ConfigPrivate *src_priv = NM_IP6_CONFIG_GET_PRIVATE (src); > > if (src_priv->never_default != dst_priv->never_default) { > dst_priv->never_default = src_priv->never_default; > has_changes = TRUE; > } > > if (!IN6_ARE_ADDR_EQUAL (&src_priv->gateway, &dst_priv->gateway)) { > nm_ip6_config_set_gateway (dst, &src_priv->gateway); > has_changes = TRUE; > } > > Since this is the same class, we can access the private data from both > instances, and we don't need to use temporary variables or accessors at all. > Obviously that only works for things that are easily accessible, eg I don't > think we should do this for address comparison because it would be more code. > But in the end, both ways are equivalent, so it's your choice which way to go. Done. > Also, I'd freeze + thaw the GObject property notifications at the start and > end, using g_object_freeze_notify() and g_object_thaw_notify(), so that > identical property notifications are condensed into a single one. Otherwise > clients will get multiple notifications each time an address is added or > removed, when we really just want one for the entire 'addresses' property. You mean inside of nm_ip*_config_replace? I think, that although IP?Config has glib properties, no "notify::*" signal gets emitted whenever you change any of these properties (e.g. look at nm_ip4_config_add_address). What is there to freeze? > Lastly, in the last patch, we only need to notify of the new IP6Config object > in nm_device_set_ip6_config() if the actual *object* has changed. But we still > need to emit the signals[IP6_CONFIG_CHANGED] since the Policy listens to that > for internal changes to the object. Which is somewhat wierd, but we'll fix > that later. So something like: > > if (priv->ip6_config != old_config) > g_object_notify (G_OBJECT (self), NM_DEVICE_IP6_CONFIG); > > if (has_changes) > g_signal_emit (self, signals[IP6_CONFIG_CHANGED], 0, priv->ip6_config, > old_config); > > g_clear_object (&old_config); > > should be the right thing. That ensure that we only emit the D-Bus signal when > the actual object changes (clients need to listen to updates on each individual > property to get changes to addresses or routes, but that has always been the > case), and that we emit the internal IP6_CONFIG_CHANGED signal whenever the > object properties changed. Done. Please review anew, thank you!!
+1 for *_cmp() functions. Just a sidenote, you might later want to extend the cmp API to be able to compare only partially or to use a different sort order (could be used in nm-rdisc, for example) in form of flags. Regarding the 'replace', I think what you actually do is not 'replace' but 'update'. You're keeping the original object and just synchronize the data. In the same source file, there's already a couple of similar functions and all of them are called *_update(). Also, if you look at the commit message of the next commit, you see: “update existing IP6Config of device instead of replacing it” which only confirms that the naming doesn't match what you're saying you're doing. Also, I recommend to keep the IPv4 and IPv6 configs in sync as much as possible.
(In reply to comment #1) > The same issues with regenerating the config exist with the IP4Config object, > it's just that the kernel doesn't make routing changes as often so we don't see > the escalating object path number. I'm curious about the details. Since we don't use kernel router discovery, there's no practical reason for a difference in protocols when kernel-initiated routing table changes are concerned. > I think the same changes that you're making > for IP6 should also be made for IP4. +1 Please keep the IPv4 and IPv6 implementations equivalent.
(In reply to comment #3) > Regarding the 'replace', I think what you actually do is not 'replace' but > 'update'. You're keeping the original object and just synchronize the data. In > the same source file, there's already a couple of similar functions and all of > them are called *_update(). > > Also, if you look at the commit message of the next commit, you see: “update > existing IP6Config of device instead of replacing it” which only confirms that > the naming doesn't match what you're saying you're doing. About the naming: As a side note: we have: nm_ip._config_merge_setting (NMIP.Config *config, NMSettingIP.Config *setting) which merges setting into config and nm_ip._config_update_setting (NMIP.Config *config, NMSettingIP.Config *setting) which merges config into setting. I think this is confusing. They should be renamed and the order of the arguments of one function should be changed. Then we have nm_ip._config_merge and nm_ip._config_subtract which do what you would expect from their name. They all "update" dst somehow. How about nm_ip._config_set instead? > Also, I recommend to keep the IPv4 and IPv6 configs in sync as much as > possible. I agree. I think as of now, they are as similar as possible. Or do you see an issue?
(In reply to comment #2) > (In reply to comment #1) > > I pushed some fixup!/squash! commits in response to comment #1. See > thaller/IP6Config_replace > > > > The same issues with regenerating the config exist with the IP4Config object, > > it's just that the kernel doesn't make routing changes as often so we don't see > > the escalating object path number. I think the same changes that you're making > > for IP6 should also be made for IP4. > Done. One question is about _update_ip4_address. It does not get called when > new_address is NULL. Maybe it should? Actually yes, since _update_ip4_address() actually just reads SIOCGIFADDR, we should just call it unconditionally from the end of nm_device_set_ip4_config(). It will handle NULL itself. > > Also, is there any reason to make the return value from the CMP functions an > > int? If we're not using these functions for sorting or anything, then it might > > jsut be simpler (and less code) to make them do a direct comparison and return > > a boolean instead. > > IMO a *_cmp method is more useful then *_is_equal, because it can also be used > for ordering. It's true, it is not needed anywhere ATM. But I think the compare > pattern is well known (strcmp, memcmp) and not significantly more complex. Do > you still want me to change it? No, that's fine, just asking. While the pattern is well-known, one problem with these functions is that it's easier to get the comparison for equality wrong, since when using equality comparisons you have to specify the expected value, plus they aren't named well. eg, I'd rather use g_str_equal() than strcmp () == 0, but g_str_equal() isn't for general purpose use, only GHashTable. Basically, it's just a bit easier to introduce bugs with the strcmp()-style return values where a boolean return makes that less likely. But I'm fine with the CMP functions the way they are. > > Next, in nm_ip6_config_replace(), we can actually do this: > > > > NMIP6ConfigPrivate *dst_priv = NM_IP6_CONFIG_GET_PRIVATE (dst); > > NMIP6ConfigPrivate *src_priv = NM_IP6_CONFIG_GET_PRIVATE (src); > > > > if (src_priv->never_default != dst_priv->never_default) { > > dst_priv->never_default = src_priv->never_default; > > has_changes = TRUE; > > } > > > > if (!IN6_ARE_ADDR_EQUAL (&src_priv->gateway, &dst_priv->gateway)) { > > nm_ip6_config_set_gateway (dst, &src_priv->gateway); > > has_changes = TRUE; > > } > > > > Since this is the same class, we can access the private data from both > > instances, and we don't need to use temporary variables or accessors at all. > > Obviously that only works for things that are easily accessible, eg I don't > > think we should do this for address comparison because it would be more code. > > But in the end, both ways are equivalent, so it's your choice which way to go. > > Done. > > > > Also, I'd freeze + thaw the GObject property notifications at the start and > > end, using g_object_freeze_notify() and g_object_thaw_notify(), so that > > identical property notifications are condensed into a single one. Otherwise > > clients will get multiple notifications each time an address is added or > > removed, when we really just want one for the entire 'addresses' property. > > You mean inside of nm_ip*_config_replace? I think, that although IP?Config has > glib properties, no "notify::*" signal gets emitted whenever you change any of > these properties (e.g. look at nm_ip4_config_add_address). What is there to > freeze? Yeah, we should actually do g_object_notify() for every property that we chagne. Previously this wasn't done because the object itself changed every time properties changed, but like you've seen this causes extra work for clients. So here's what we should do: 1) Add D-Bus properties for Gateway, Searches, and MSS. (ignore PTP address right now, we're not sure yet if that's really useful with IPv6) 2) add g_object_notify() anywhere one of the properties gets changed, added, or deleted; nm_ip6_config_capture(), nm_ip6_config_merge_setting(), nm_ip6_config_replace(), nm_ip6_config_set_gateway(), nm_ip6_config_add_address(), nm_ip6_config_del_address(), nm_ip6_config_reset_addresses(), etc. 3) freeze/thaw at the beginning of nm_ip6_config_replace(), nm_ip6_config_capture(), nm_ip6_config_merge_setting(), nm_ip6_config_subtract(), etc; anywhere that does multiple changes at the same time
Just a small cosmetic thing - align ending \ in macros _CMP_POINTER and _CMP_FIELD Also I think you should just fixup, squash the commits in the branch so that we can see the latest version. (I understand you want to make the changes more explicit, but it is harder to review the commits).
Hi, new version of the branch thaller/IP6Config_replace, reworked and rebased on master. Please especially note: - "core: update existing IP[46]Config of device instead of replac" * make changes in src/nm-policy.c because now old_config most of the time is the same object as new_config. Is it ok? - "core: add nm_ip[46]_config_clone functions" * Functions are not used ATM. I added them, because it was easy and they might be useful. Leave or remove them? Please review. Thanks, Thomas
> core: add nm_platform_ip[46]_*_cmp functions >+#define _CMP_FIELD(a, b, field) \ >+ do { \ >+ if (((a)->field) != ((b)->field)) \ >+ return (((a)->field) < ((b)->field)) ? -1 : 1; \ >+ } while (0) glib provides macros that we'd normally use here: G_STMT_START { \ if (((a)->field) != ((b)->field)) \ return (((a)->field) < ((b)->field)) ? -1 : 1; \ } G_STMT_END saves the reader from briefly thinking that the code is supposed to be looping. any reason you didn't define a macro for the memcmp parts? > core: add nm_ip[46]_config_replace functions >+/** >+ * nm_ip4_config_replace() >+ * @dst: config from which to remove everything in @src >+ * @src: config to remove from @dst >+ * @relevant_changes: return whether there are changes to the >+ * destination object that are relevant. This is equal to >+ * nm_ip4_config_equal showing any difference. >+ * >+ * Removes everything in @src from @dst. >+ * >+ * Returns: whether the dst instance changed in any way (including minor changes, >+ * that are not signaled by the output parameter relevant_changes). >+ */ First line should end with ":", not "()", the reference to nm_ip4_config_equal should have "()" after it (with no space between name and parens), and the "dst" in the "Returns" line should have a "@". Same in the ip6 version. (It doesn't *really* matter since we're not actually running gtk-doc on this file, but you want to get used to writing things the right way, for when you're making changes to libnm-util and libnm-glib, where it does matter.) Also, you forgot to rewrite the body ("Removes...") after cut+pasting it from nm_ip4_config_subtract(). The return values of this function and its ip6 equivalent don't get used. It would be simpler to just make the "relevant_changes" output be the return value instead (which then also simplifies the code because then you can get rid of has_minor_changes). > core: add nm_ip[46]_config_clone functions > * Functions are not used ATM. I added them, because it was easy and > they might be useful. Leave or remove them? I'd say remove; we have enough code as it is, we don't need more code that's not being used. :) > core: update existing IP[46]Config of device instead of replacing it (bgo #707617) > Also, add new gobject properties 'gateway' and 'searches' to the config class, hadn't dcbw said to merge searches into domains? If you do keep them, you don't need separate demarshal_searches() and demarshal_domain() in libnm-glib; just change the _nm_object_queue_notify() call to pass pspec->name, and then you can use the same demarshaller for both. > +#define _NOTIFY(config, prop) do { g_object_notify_by_pspec (G_OBJECT (config), obj_properties[prop]); } while (0) same G_STMT_START comment as before, although you don't even need them in this case since the macro expands to a single statement.
(In reply to comment #10) Hi Dan, Thank you for your comments. I implemented all your remarks and pushed a new version of the branch. Except: > The return values of this function and its ip6 equivalent don't get used. It > would be simpler to just make the "relevant_changes" output be the return value > instead (which then also simplifies the code because then you can get rid of > has_minor_changes). I know, YAGNI; but I think that *_replace returning false, although it changed @dst in any way, violates the principle of least surprise. I'd prefer the signature as it is. > hadn't dcbw said to merge searches into domains? dcbw reconsidered this on IRC.
Pushed now to master. http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=db9b7e10aca5456ec4960b75617e032209b98bc1 db9b7e1 core: update existing IP[46]Config of device instead of replacing it (bgo #707617) f0fccd9 core: add nm_ip[46]_config_replace functions b1113a0 core: add nm_platform_ip[46]_*_cmp functions