GNOME Bugzilla – Bug 734009
[review] dcbw/rh990480-ibft-vlan: move iBFT connection handling to a separate, generic plugin
Last modified: 2014-09-19 18:57:15 UTC
iBFT is an ACPI firmware table that describes settings for mounting your rootfs via an iSCSI adapter, eg SCSI-over-IP. Obviously since your rootfs comes from the network, you need to configure your NIC to access that network. These settings are stored in an ACPI table that is exposed by the kernel and the 'iscsiadm' utility. On Fedora, Anaconda/dracut used to write out an ifcfg file with BOOTPROTO=ibft which directed the NM ifcfg-rh plugin to read the IP details from iBFT. That worked OK. Well, now we need to add VLAN support. And that means that instead of just handling IPv4 settings, we need to actually construct entire connections with the data from the iBFT records. That turns out to be somewhat icky. So there are two approaches in this branch: #1: ifcfg-rh: read and construct iBFT VLAN connections (rh #990480) This is the keep-it-all-in-ifcfg-rh approach. It's not bad, but it's more complicated than I think it needs to be. #2: ibft: add settings plugin for reading iBFT configuration ifcfg-rh: remove iBFT handling in preference of the ibft plugin An alternate approach which creates an entirely separate settings plugin for the iBFT data, and removes all iBFT processing from ifcfg-rh. We don't actually care about any of the additional ifcfg file stuff, since the data for the connection comes from the firmware anyway. Yes, technically you could add more IP addresses and routes or an MTU to the ifcfg file and have NM apply them, but the initscripts have never supported BOOTPROT=ibft, and you need to do this earlier than NM anyway (eg, see rh #1076465). I tentatively think it's acceptable to ignore that functionality. rh #990480 and rh #831002 cover some of the background here. The anaconda/dracut folks would like to write out really simple ifcfg files anyway, which don't match the ifcfg specification, which is somewhat icky. But why do they even have to do that, if just to tell NM to read the data from ibft? Why shouldn't NM just read the iBFT data itself without ifcfg files? That's what this approach does.
One unresolved question, if we go with the 'ibft' settings plugin approach, is how to get the plugin installed seamlessly on upgrade. We could defer that to packaging perhaps and just have the Fedora NM packages add the 'ibft' settings plugin to conf.d.
The setting plugin approach makes sense. That would also mean we'd now get ibft support on non-ifcfg-rh platforms, right?
(In reply to comment #2) > The setting plugin approach makes sense. That would also mean we'd now get ibft > support on non-ifcfg-rh platforms, right? Yes.
Repushed; I dropped the first approach and leave only the ibft plugin approach.
>> ifcfg-rh: convert iBFT tests to g_assert() maybe drop inet_ntoa32(), we alrady have nm_utils_inet4_ntop() which does the same. Branch adds functions like th/bgo696936_normalize_settings nmtst_assert_connection_verifies_without_normalization() which saves lines of code and does more careful asserts. Actually, there should be a local function that wraps connection_from_file(), does not need GError arguments, but instead verifies the connection. >> ifcfg-rh: rework iBFT/iSCSI firmware data reading get_int_full() should be replaced by nm_utils_ascii_str_to_int64(). I think match_iscsiadm_tag() calling assert is overly severe. Just g_return in that case. Also, it would wrongly match: TAG_AB=xyz when searching for tag="TAG_A". You should just check at position (line[strlen(tag)] == '=') +#define CLEAR_ARRAY(a) { if (a->len) g_ptr_array_remove_range (a, 0, a->len); } G_STMT_START {} G_STMT_END >> ifcfg-rh: trivial: use get_int_full() instead of strtoll() again, I would use nm_utils_ascii_str_to_int64(). >> ibft: add settings plugin for reading iBFT configuration (bgo #734009) Unmanaged devices can be configured through NetworkManager.conf and the normal 'keyfile' mechanisms. todo: this 'keyfile' mechanism should really be part of the general section... tabs: G_DEFINE_TYPE_EXTENDED (SCPluginIbft, sc_plugin_ibft, G_TYPE_OBJECT, 0, »···»···»···»···»···»···G_IMPLEMENT_INTERFACE (NM_TYPE_SYSTEM_CONFIG_INTERFACE, »···»···»···»···»···»···»···»···»···»···»··· system_config_interface_init)) >> ifcfg-rh: remove iBFT handling (use the ibft plugin instead) (bgo #734009) (rh #990480) bootproto = svGetValue (parsed, "BOOTPROTO", FALSE); if (bootproto && !g_ascii_strcasecmp (bootproto, "ibft")) { - ibft_data = read_ibft_config (parsed, iscsiadm_path, error); - if (!ibft_data) + g_free (bootproto); goto done; } hm. so before we leaked bootproto? Just for aesthetics, fix the commit before? >> ifcfg-rh: more conversions to g_assert() g_assert_cmpstr (inet_ntoa32 (nm_ip4_address_get_gateway (ip4_addr)), ==, "192.168.1.1"); seems like this pattern is common. I would rather: static inline void nmtst_assert_ip4_address_equals (guint32 addr, const char *expected, const char *loc) { guint32 addr2 = nmtst_inet4_from_string (expected); if (addr != addr2) g_error ("assert: %s: ip4 address '%s' expected, but got %s", loc, expected, nm_utils_inet4_ntop (addr, NULL)); } #define nmtst_assert_ip4_address_equals(addr, expected) \ nmtst_assert_ip4_address_equals (addr, expected, G_STRLOC) rest looks good.
(In reply to comment #5) > >> ifcfg-rh: convert iBFT tests to g_assert() > > maybe drop inet_ntoa32(), we alrady have nm_utils_inet4_ntop() which does the > same. Done. > Branch adds functions like th/bgo696936_normalize_settings > nmtst_assert_connection_verifies_without_normalization() which saves lines of > code and does more careful asserts. > > Actually, there should be a local function that wraps connection_from_file(), > does not need GError arguments, but instead verifies the connection. Yeah, that's probably what should happen. I've left that for a one-time cleanup though. > >> ifcfg-rh: rework iBFT/iSCSI firmware data reading > > get_int_full() should be replaced by nm_utils_ascii_str_to_int64(). Done. > I think match_iscsiadm_tag() calling assert is overly severe. Just g_return in > that case. Also, it would wrongly match: > > TAG_AB=xyz > > when searching for tag="TAG_A". > > You should just check at position (line[strlen(tag)] == '=') I've fixed this up, please review. > +#define CLEAR_ARRAY(a) { if (a->len) g_ptr_array_remove_range (a, 0, a->len); > } > G_STMT_START {} G_STMT_END I dropped the patch that added this. > >> ifcfg-rh: trivial: use get_int_full() instead of strtoll() > > again, I would use nm_utils_ascii_str_to_int64(). I just dropped this patch. > >> ibft: add settings plugin for reading iBFT configuration (bgo #734009) > > Unmanaged devices can be configured through NetworkManager.conf and the > normal 'keyfile' mechanisms. > > todo: this 'keyfile' mechanism should really be part of the general section... Yeah, also true. > tabs: > G_DEFINE_TYPE_EXTENDED (SCPluginIbft, sc_plugin_ibft, G_TYPE_OBJECT, 0, > »···»···»···»···»···»···G_IMPLEMENT_INTERFACE (NM_TYPE_SYSTEM_CONFIG_INTERFACE, > »···»···»···»···»···»···»···»···»···»···»··· system_config_interface_init)) Fixed. > >> ifcfg-rh: remove iBFT handling (use the ibft plugin instead) (bgo #734009) (rh #990480) > > bootproto = svGetValue (parsed, "BOOTPROTO", FALSE); > if (bootproto && !g_ascii_strcasecmp (bootproto, "ibft")) { > - ibft_data = read_ibft_config (parsed, iscsiadm_path, error); > - if (!ibft_data) > + g_free (bootproto); > goto done; > } > > hm. so before we leaked bootproto? Just for aesthetics, fix the commit before? Since I dropped the commits that added the code that leaked it, this is no longer necessary. > >> ifcfg-rh: more conversions to g_assert() > > static inline void > nmtst_assert_ip4_address_equals (guint32 addr, const char *expected, const char > *loc) > { > guint32 addr2 = nmtst_inet4_from_string (expected); > > if (addr != addr2) > g_error ("assert: %s: ip4 address '%s' expected, but got %s", > loc, expected, nm_utils_inet4_ntop (addr, NULL)); > } > #define nmtst_assert_ip4_address_equals(addr, expected) \ > nmtst_assert_ip4_address_equals (addr, expected, G_STRLOC) I added this part to the commit that adds the ibft plugin, and then used it in the ibft and ifcfg-rh testcases. I dropped the ibft-related changes to the ifcfg-rh plugin, since those were basically the in-progress commits that was then just copied over to the ibft plugin. Repushed!
(In reply to comment #6) > (In reply to comment #5) > > I think match_iscsiadm_tag() calling assert is overly severe. Just g_return in > > that case. Also, it would wrongly match: > > > > TAG_AB=xyz > > > > when searching for tag="TAG_A". > > > > You should just check at position (line[strlen(tag)] == '=') > > I've fixed this up, please review. looks better now. I still dislike that it modifies the input argument @data from parse_ibft_config (const GPtrArray *data, GError **error, ...) and then it returns a char* pointer (although the caller is not supposed to free/modify it). I see, that doing a g_strdup on every result is unnecessary too (and requires lots of g_free calls). How about read_ibft_blocks() already cleaning up white spaces so that no g_strstrip is needed in match_iscsiadm_tag()? > > >> ifcfg-rh: more conversions to g_assert() > > > > static inline void > > nmtst_assert_ip4_address_equals (guint32 addr, const char *expected, const char > > *loc) > > { > > guint32 addr2 = nmtst_inet4_from_string (expected); > > > > if (addr != addr2) > > g_error ("assert: %s: ip4 address '%s' expected, but got %s", > > loc, expected, nm_utils_inet4_ntop (addr, NULL)); > > } > > #define nmtst_assert_ip4_address_equals(addr, expected) \ > > nmtst_assert_ip4_address_equals (addr, expected, G_STRLOC) > minor complaint: Passing expected=NULL, is valid (and effectively means 0.0.0.0). But the g_error() message should print ("expected ? expected : "any"). src/settings/plugins/ibft/tests/test-ibft.c: read_block() should assert that it found the expected block (for the given MAC address). what is with connection_diff()? const guint8 expected_mac_address[ETH_ALEN] = { 0x00, 0x33, 0x21, 0x98, 0xb9, 0xf0 }; now that we soon have the cleaned up hwaddr functions, it might be better specifying all hardware addresses as strings. That way, when you grep for "00:33:21:98:b9:f0" you find the relevant places. then you would use: - g_assert (memcmp (array->data, &expected_mac_address[0], ETH_ALEN) == 0); + g_assert (nm_utils_hwaddr_equals (array->data, ETH_ALEN, expected_mac_address, -1); (btw, I still see a use for a real equals function, contrary to nm_utils_hwaddr_matches() that only compares part of Infiniband hw-addresses)) actually, I feel that g_assert (nm_utils_hwaddr_equals (array->data, ETH_ALEN, expected_mac_address, -1); still has the downside that the assertion failure prints no reason. Might be even worth to add a nmtst_assert_hwaddr_equals() with g_error() printing the textual representation of array->data. Regarding the one-time-cleanup, I still would cherry-pick: 883770c - nmtst: add assertion functions for verify() connection e4817a4 - fixup! nmtst: add assertion functions for verify() connection and use nmtst_assert_connection_verifies_without_normalization(). rest looks good
> ibft: add settings plugin for reading iBFT configuration (bgo #734009) >+ -I$(top_srcdir)/src/logging \ >+ -I$(top_srcdir)/src/platform \ >+ -I$(top_srcdir)/src/settings \ >+ -I$(top_srcdir)/src/posix-signals \ >+ -I$(top_srcdir)/src/config \ logging, posix-signals, and config no longer exist. >+ * (C) Copyright 2014 Red Hat, Inc. drop the "(C)" everywhere, and double-check the dates. (eg, errors.h claims 2008-2013) >+#ifndef __COMMON_H__ >+#define __COMMON_H__ wrong name (that's in errors.h) >+ object = g_object_new (NM_TYPE_IBFT_CONNECTION, NULL); >+ if (object) { That call can't fail, you don't need to check. >+ * Dan Williams <dcbw@redhat.com> You include that in plugin.c and plugin.h but not any of the other files. (I suggest removing it; other people are going to make changes to the file later.) >+set_property (GObject *object, guint prop_id, >+ const GValue *value, GParamSpec *pspec) indentation (plugin.c) >+ g_assert (parse_ibft_config (block, NULL, ISCSI_VLAN_ID_TAG, &vlan_id_str, NULL)); don't put real code inside g_assert(), since it could get compiled out >+ -DSYSCONFDIR=\"nonexistent\" \ >+ -DSBINDIR=\"nonexistent\" huh? (tests/Makefile.am. Also, same problem with -Ilogging, etc as the other Makefile.am) >+ iscsiadm-test-bad-ipaddr \ >+ iscsiadm-test-bad-gateway \ The names of these two files are reversed (bad-ipaddr has the bad gateway, and bad-gateway has the bad IP address). >+/* NetworkManager system settings service - keyfile plugin "ibft plugin" (test-ibft.c) >+#if 0 >+static void >+connection_diff (NMConnection *a, NMConnection *b) what's this for? >+ e = ether_aton (s_hwaddr); boo! use nm_utils_hwaddr_aton(). >+test_read_ibft_dhcp (void) maybe the non-VLAN test cases should be asserting that there is no VLAN setting?
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > I think match_iscsiadm_tag() calling assert is overly severe. Just g_return in > > > that case. Also, it would wrongly match: > > > > > > TAG_AB=xyz > > > > > > when searching for tag="TAG_A". > > > > > > You should just check at position (line[strlen(tag)] == '=') > > > > I've fixed this up, please review. > > looks better now. > > I still dislike that it modifies the input argument @data from > parse_ibft_config (const GPtrArray *data, GError **error, ...) > and then it returns a char* pointer (although the caller is not supposed to > free/modify it). Pushed a fixup commit for this. > > > >> ifcfg-rh: more conversions to g_assert() > > > > > > static inline void > > > nmtst_assert_ip4_address_equals (guint32 addr, const char *expected, const char > > > *loc) > > > { > > > guint32 addr2 = nmtst_inet4_from_string (expected); > > > > > > if (addr != addr2) > > > g_error ("assert: %s: ip4 address '%s' expected, but got %s", > > > loc, expected, nm_utils_inet4_ntop (addr, NULL)); > > > } > > > #define nmtst_assert_ip4_address_equals(addr, expected) \ > > > nmtst_assert_ip4_address_equals (addr, expected, G_STRLOC) > > > > minor complaint: Passing expected=NULL, is valid (and effectively means > 0.0.0.0). But the g_error() message should print ("expected ? expected : > "any"). Fixed > src/settings/plugins/ibft/tests/test-ibft.c: > read_block() should assert that it found the expected block (for the given > MAC address). Fixed. > what is with connection_diff()? Removed. It's sometimes useful to find out why a connection that's written out isn't the same as the original in an easy-to-read way. However, since ibft doesn't write out connections anyway, it's useless here. > const guint8 expected_mac_address[ETH_ALEN] = { 0x00, 0x33, 0x21, 0x98, 0xb9, > 0xf0 }; > > now that we soon have the cleaned up hwaddr functions, it might be better > specifying all hardware addresses as strings. That way, when you grep for > "00:33:21:98:b9:f0" you find the relevant places. > > > then you would use: > - g_assert (memcmp (array->data, &expected_mac_address[0], ETH_ALEN) == 0); > + g_assert (nm_utils_hwaddr_equals (array->data, ETH_ALEN, > expected_mac_address, -1); Done. > actually, I feel that > g_assert (nm_utils_hwaddr_equals (array->data, ETH_ALEN, > expected_mac_address, -1); > still has the downside that the assertion failure prints no reason. Might be > even worth to add a nmtst_assert_hwaddr_equals() with g_error() printing the > textual representation of array->data. Done. > Regarding the one-time-cleanup, I still would cherry-pick: > 883770c - nmtst: add assertion functions for verify() connection > e4817a4 - fixup! nmtst: add assertion functions for verify() connection > and use nmtst_assert_connection_verifies_without_normalization(). Done.
(In reply to comment #8) > > ibft: add settings plugin for reading iBFT configuration (bgo #734009) > > >+ -I$(top_srcdir)/src/logging \ > >+ -I$(top_srcdir)/src/platform \ > >+ -I$(top_srcdir)/src/settings \ > >+ -I$(top_srcdir)/src/posix-signals \ > >+ -I$(top_srcdir)/src/config \ > > logging, posix-signals, and config no longer exist. > > >+ * (C) Copyright 2014 Red Hat, Inc. > > drop the "(C)" everywhere, and double-check the dates. (eg, errors.h claims > 2008-2013) > > >+#ifndef __COMMON_H__ > >+#define __COMMON_H__ > > wrong name (that's in errors.h) > > >+ object = g_object_new (NM_TYPE_IBFT_CONNECTION, NULL); > >+ if (object) { > > That call can't fail, you don't need to check. > > >+ * Dan Williams <dcbw@redhat.com> > > You include that in plugin.c and plugin.h but not any of the other files. (I > suggest removing it; other people are going to make changes to the file later.) > > >+set_property (GObject *object, guint prop_id, > >+ const GValue *value, GParamSpec *pspec) > > indentation (plugin.c) > > >+ g_assert (parse_ibft_config (block, NULL, ISCSI_VLAN_ID_TAG, &vlan_id_str, NULL)); > > don't put real code inside g_assert(), since it could get compiled out > > >+ -DSYSCONFDIR=\"nonexistent\" \ > >+ -DSBINDIR=\"nonexistent\" > > huh? (tests/Makefile.am. Also, same problem with -Ilogging, etc as the other > Makefile.am) > > >+ iscsiadm-test-bad-ipaddr \ > >+ iscsiadm-test-bad-gateway \ > > The names of these two files are reversed (bad-ipaddr has the bad gateway, and > bad-gateway has the bad IP address). > > >+/* NetworkManager system settings service - keyfile plugin > > "ibft plugin" (test-ibft.c) > > >+#if 0 > >+static void > >+connection_diff (NMConnection *a, NMConnection *b) > > what's this for? > > >+ e = ether_aton (s_hwaddr); > > boo! use nm_utils_hwaddr_aton(). > > >+test_read_ibft_dhcp (void) > > maybe the non-VLAN test cases should be asserting that there is no VLAN > setting? All fixed in a fixup! commit.
Pushed one fixup commit. I rewrote remove_most_whitespace() because the previous behavior was strange in the presence of white-space *inside* the key, or more then one '=' characters... which should be the case normally, but I prefer clear behavior. Only thing I dislike is that nmtst_assert_ip4_address_equals() for infiniband addresses does not do a full comparison but ignores the first half. I think for nmtst_assert_*_equals() we don't want this fuzzy behaviour... The rest looks good
(In reply to comment #11) > Pushed one fixup commit. > > I rewrote remove_most_whitespace() because the previous behavior was strange in > the presence of white-space *inside* the key, or more then one '=' > characters... which should be the case normally, but I prefer clear behavior. Looks fine to me, thanks. > Only thing I dislike is that nmtst_assert_ip4_address_equals() for infiniband > addresses does not do a full comparison but ignores the first half. I think for > nmtst_assert_*_equals() we don't want this fuzzy behaviour... I added a fixup! commit with some additional code to verify IB addresses. See if that's what you had in mind? Repushed.
Another push with two commits that came from debugging in rh#990480. In short, at startup, add_device(eth0) calls system_create_virtual_devices(), which created eth0.123 because there was a valid saved connection for eth0.123. Unfortunately, eth0.123 already existed and had configuration, but system_create_virtual_devices() doesn't allow assuming the existing connection because it calls add_device() with genearte_con = FALSE.
Pushed one fixup! commit. all new commits look good to me.
For nmtst_assert_hwaddr_equals(), there's really no point in using nm_utils_hwaddr_matches() at all; if you need the code to do an aton+memcmp for the InfiniBand case, you might as well just simplify things and do that in all cases. (But keep a comment explaining why). All the other changes look good.
(In reply to comment #14) > Pushed one fixup! commit. > > > all new commits look good to me. Hmm, I may have inadvertently blown that away on a repush. Do you happen to still have it around somewhere?
(In reply to comment #16) > (In reply to comment #14) > > Pushed one fixup! commit. > > > > > > all new commits look good to me. > > Hmm, I may have inadvertently blown that away on a repush. Do you happen to > still have it around somewhere? Now, is still there: febc32e8b84f7c7bb6a87443f5d2f32ef3004c8d dcbw/rh990480-ibft-vlan Anyway: diff --git a/src/settings/plugins/ibft/reader.c b/src/settings/plugins/ibft/reader.c index cb47427..908ba69 100644 --- a/src/settings/plugins/ibft/reader.c +++ b/src/settings/plugins/ibft/reader.c @@ -129,6 +129,6 @@ read_ibft_blocks (const char *iscsiadm_path, gboolean success = FALSE; - g_return_val_if_fail (iscsiadm_path != NULL, NULL); - g_return_val_if_fail (out_blocks != NULL && *out_blocks == NULL, NULL); + g_return_val_if_fail (iscsiadm_path != NULL, FALSE); + g_return_val_if_fail (out_blocks != NULL && *out_blocks == NULL, FALSE); if (!g_spawn_sync ("/", (char **) argv, (char **) envp, 0,
(In reply to comment #15) > For nmtst_assert_hwaddr_equals(), there's really no point in using > nm_utils_hwaddr_matches() at all; if you need the code to do an aton+memcmp for > the InfiniBand case, you might as well just simplify things and do that in all > cases. (But keep a comment explaining why). > > All the other changes look good. Fixed that up, now converts manually and compares the whole thing. I squashed everything (including thaller's fixup) and merged to git master.