GNOME Bugzilla – Bug 777251
[review] refactor setting of MTU in NMDevice [th/device-mtu-bgo777251]
Last modified: 2017-01-16 16:36:29 UTC
please review
Looks generally all right to me. > ab3be53 device: cleanup setting of mtu in NMDevice NAK for this; it's just too ugly. Maybe could be improved with better naming, such as "_writable" instead of a trailing underscore and a comment around the union. > 42f6eef device: reset previous MTU when device disconnects > For now, restore the previous MTU on deactivatoin iff NetworkManager changed ^^^^^^^^^^^^ > d75ac2a device: set a per-device default MTU on activation > + bool mtu_initialized:1; > + > bool up; /* IFF_UP */ You could just :1 the following bool to fold it in?
> device: cleanup setting of mtu in NMDevice This doesn't seem an improvement to me, as it doesn't improve readability neither maintainability of code. Probably, it would have been more useful if all the writes had to go through a single function (like for @ip4_state). > device: reset previous MTU when device disconnects src/devices/nm-device.c: In function ‘nm_device_cleanup’: src/devices/nm-device.c:11482:4: error: ‘ifindex’ may be used uninitialized in this function [-Werror=maybe-uninitialized] nm_platform_link_set_mtu (NM_PLATFORM_GET, ifindex, priv->mtu_initial); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > device: refactor setting user-configured MTU during config commit + guint32 (*v_get_mtu) (NMDevice *self, gboolean *out_is_user_config); The name looks different from other methods, what does the 'v' stand for? > device/ppp: hook up setting MTU for PPP connection via virtual v_get_mtu() function I don't think this needs to be changed. The existing code is to ensure that the MTU of the underlying device is (ppp.mtu + 8) to have room for encapsulation, while the new code sets the MTU of the IP interface (PPP) to (ppp.mtu + 8).
(In reply to Beniamino Galvani from comment #2) > > device: cleanup setting of mtu in NMDevice > > This doesn't seem an improvement to me, as it doesn't improve > readability neither maintainability of code. Probably, it would have > been more useful if all the writes had to go through a single function > (like for @ip4_state). dropped. > > device: reset previous MTU when device disconnects > > src/devices/nm-device.c: In function ‘nm_device_cleanup’: > src/devices/nm-device.c:11482:4: error: ‘ifindex’ may be used uninitialized > in this function [-Werror=maybe-uninitialized] > nm_platform_link_set_mtu (NM_PLATFORM_GET, ifindex, priv->mtu_initial); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ fixed. > > device: refactor setting user-configured MTU during config commit > > + guint32 (*v_get_mtu) (NMDevice *self, gboolean > *out_is_user_config); > > The name looks different from other methods, what does the 'v' stand for? I think names should be generally unique project wide (so I can grep for stuff). I didn't want to spoil the name "get_mtu" for something as uncommon as the virtual function, so it has a "v" prefix to make it unique. I didn't know a better name. (yes, I know "grep -w"). I wouldn't mind if all virtual function pointers had such a "v_" prefix, but yes, it's without precedence. Overall, not an overly strong justification, I don't mind. What is your suggestion? > > device/ppp: hook up setting MTU for PPP connection via virtual v_get_mtu() function > > I don't think this needs to be changed. The existing code is to ensure > that the MTU of the underlying device is (ppp.mtu + 8) to have room > for encapsulation, while the new code sets the MTU of the IP interface > (PPP) to (ppp.mtu + 8). ah, you are right, I missed that it sets ifindex, not ip_ifindex. I dropped the patch. This bypasses the recording of the initial MTU from "device: reset previous MTU when device disconnects". What a mess. For now I leave it as is.
(In reply to Thomas Haller from comment #3) > I think names should be generally unique project wide (so I can grep for > stuff). > I didn't want to spoil the name "get_mtu" for something as uncommon as the > virtual function, so it has a "v" prefix to make it unique. I didn't know a > better name. > (yes, I know "grep -w"). > I wouldn't mind if all virtual function pointers had such a "v_" prefix, but > yes, it's without precedence. > > Overall, not an overly strong justification, I don't mind. What is your > suggestion? I'm not very good at naming things... maybe something like get_configured_mtu()?
renamed, and merged: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=2b51d39671bc7fa6fa78a730333647c137e8ac3b