From 5233e6b913e85ae05c265058cec0984b83a057d0 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Tue, 18 Mar 2014 13:39:52 -0500 Subject: [PATCH 1/5] dcb: set all Priority Group options at the same time (rh #799241) First, lldpad doesn't support disabling priority groups (e:0) without specifying a complete priority group config (which wouldn't be used anyway, since you're turning it off!). While this bug is being fixed upstream, we'll just ignore errors turning off PG, since if you're using DCB on an interface, you probably want to use it all the time. Second, lldpad really wants all PG options on the same configuration line, not split apart, because it validates the complete package of options before applying them, regardless of whether or not they are given in the same command. Since NM was just emitting all the options in separate dcbtool invocations anyway, just combine them all into a single invocation. --- src/nm-dcb.c | 74 ++++++++++++++++++++++++-------------------- src/tests/test-dcb.c | 12 +++---- 2 files changed, 47 insertions(+), 39 deletions(-) diff --git a/src/nm-dcb.c b/src/nm-dcb.c index d42cf23ea..89a6507c1 100644 --- a/src/nm-dcb.c +++ b/src/nm-dcb.c @@ -154,61 +154,69 @@ _dcb_setup (const char *iface, /* Priority Groups */ flags = nm_setting_dcb_get_priority_group_flags (s_dcb); - SET_FLAGS (flags, "pg"); if (flags & NM_SETTING_DCB_FLAG_ENABLE) { - char buf[10]; + GString *s; + gboolean success; guint id; + s = g_string_sized_new (150); + + g_string_append_printf (s, "pg e:1 a:%c w:%c", + flags & NM_SETTING_DCB_FLAG_ADVERTISE ? '1' : '0', + flags & NM_SETTING_DCB_FLAG_WILLING ? '1' : '0'); + /* Priority Groups */ + g_string_append (s, " pgid:"); for (i = 0; i < 8; i++) { id = nm_setting_dcb_get_priority_group_id (s_dcb, i); g_assert (id < 8 || id == 15); - buf[i] = (id < 8) ? ('0' + id) : 'f'; + g_string_append_c (s, (id < 8) ? ('0' + id) : 'f'); } - buf[i] = 0; - if (!do_helper (iface, DCBTOOL, run_func, user_data, error, "pg pgid:%s", buf)) - return FALSE; /* Priority Group Bandwidth */ - if (!do_helper (iface, DCBTOOL, run_func, user_data, error, "pg pgpct:%u,%u,%u,%u,%u,%u,%u,%u", - nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 0), - nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 1), - nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 2), - nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 3), - nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 4), - nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 5), - nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 6), - nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 7))) - return FALSE; + g_string_append_printf (s, " pgpct:%u,%u,%u,%u,%u,%u,%u,%u", + nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 0), + nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 1), + nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 2), + nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 3), + nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 4), + nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 5), + nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 6), + nm_setting_dcb_get_priority_group_bandwidth (s_dcb, 7)); /* Priority Bandwidth */ - if (!do_helper (iface, DCBTOOL, run_func, user_data, error, "pg uppct:%u,%u,%u,%u,%u,%u,%u,%u", - nm_setting_dcb_get_priority_bandwidth (s_dcb, 0), - nm_setting_dcb_get_priority_bandwidth (s_dcb, 1), - nm_setting_dcb_get_priority_bandwidth (s_dcb, 2), - nm_setting_dcb_get_priority_bandwidth (s_dcb, 3), - nm_setting_dcb_get_priority_bandwidth (s_dcb, 4), - nm_setting_dcb_get_priority_bandwidth (s_dcb, 5), - nm_setting_dcb_get_priority_bandwidth (s_dcb, 6), - nm_setting_dcb_get_priority_bandwidth (s_dcb, 7))) - return FALSE; + g_string_append_printf (s, " uppct:%u,%u,%u,%u,%u,%u,%u,%u", + nm_setting_dcb_get_priority_bandwidth (s_dcb, 0), + nm_setting_dcb_get_priority_bandwidth (s_dcb, 1), + nm_setting_dcb_get_priority_bandwidth (s_dcb, 2), + nm_setting_dcb_get_priority_bandwidth (s_dcb, 3), + nm_setting_dcb_get_priority_bandwidth (s_dcb, 4), + nm_setting_dcb_get_priority_bandwidth (s_dcb, 5), + nm_setting_dcb_get_priority_bandwidth (s_dcb, 6), + nm_setting_dcb_get_priority_bandwidth (s_dcb, 7)); /* Strict Bandwidth */ + g_string_append (s, " strict:"); for (i = 0; i < 8; i++) - buf[i] = nm_setting_dcb_get_priority_strict_bandwidth (s_dcb, i) ? '1' : '0'; - buf[i] = 0; - if (!do_helper (iface, DCBTOOL, run_func, user_data, error, "pg strict:%s", buf)) - return FALSE; + g_string_append_c (s, nm_setting_dcb_get_priority_strict_bandwidth (s_dcb, i) ? '1' : '0'); /* Priority Traffic Class */ + g_string_append (s, " up2tc:"); for (i = 0; i < 8; i++) { id = nm_setting_dcb_get_priority_traffic_class (s_dcb, i); g_assert (id < 8); - buf[i] = '0' + id; + g_string_append_c (s, '0' + id); } - buf[i] = 0; - if (!do_helper (iface, DCBTOOL, run_func, user_data, error, "pg up2tc:%s", buf)) + + success = do_helper (iface, DCBTOOL, run_func, user_data, error, s->str); + g_string_free (s, TRUE); + if (!success) return FALSE; + } else { + /* Ignore disable failure since lldpad <= 0.9.46 does not support disabling + * priority groups without specifying an entire PG config. + */ + do_helper (iface, DCBTOOL, run_func, user_data, error, "pg e:0"); } return TRUE; diff --git a/src/tests/test-dcb.c b/src/tests/test-dcb.c index 0a0924423..c950ac6d0 100644 --- a/src/tests/test-dcb.c +++ b/src/tests/test-dcb.c @@ -221,12 +221,12 @@ test_dcb_priority_groups (void) "dcbtool sc eth0 app:iscsi e:0 a:0 w:0", "dcbtool sc eth0 app:fip e:0 a:0 w:0", "dcbtool sc eth0 pfc e:0 a:0 w:0", - "dcbtool sc eth0 pg e:1 a:1 w:1", - "dcbtool sc eth0 pg pgid:765f3210", - "dcbtool sc eth0 pg pgpct:10,40,5,10,5,20,7,3", - "dcbtool sc eth0 pg uppct:100,50,33,25,20,16,14,12", - "dcbtool sc eth0 pg strict:01010101", - "dcbtool sc eth0 pg up2tc:01201201", + "dcbtool sc eth0 pg e:1 a:1 w:1" \ + " pgid:765f3210" \ + " pgpct:10,40,5,10,5,20,7,3" \ + " uppct:100,50,33,25,20,16,14,12" \ + " strict:01010101" \ + " up2tc:01201201", NULL }, }; NMSettingDcb *s_dcb; From 95d199e04b4a43a211307873e6cf1918d6d241e0 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Wed, 19 Mar 2014 14:42:51 -0500 Subject: [PATCH 2/5] dcb: fix memory leak --- src/nm-dcb.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nm-dcb.c b/src/nm-dcb.c index 89a6507c1..37b037998 100644 --- a/src/nm-dcb.c +++ b/src/nm-dcb.c @@ -321,6 +321,7 @@ run_helper (char **argv, guint which, gpointer user_data, GError **error) "Failed to run '%s'", cmdline); success = FALSE; } + g_free (outmsg); g_free (errmsg); g_free (cmdline); From 4515099a3ea35026cd552573049172db903212fe Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Wed, 19 Mar 2014 15:15:26 -0500 Subject: [PATCH 3/5] dcb: clean up FCoE too --- src/nm-dcb.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/nm-dcb.c b/src/nm-dcb.c index 37b037998..d29332f78 100644 --- a/src/nm-dcb.c +++ b/src/nm-dcb.c @@ -343,6 +343,14 @@ nm_dcb_setup (const char *iface, NMSettingDcb *s_dcb, GError **error) gboolean nm_dcb_cleanup (const char *iface, GError **error) { - return _dcb_cleanup (iface, run_helper, GUINT_TO_POINTER (DCBTOOL), error); + gboolean success; + + success = _dcb_cleanup (iface, run_helper, GUINT_TO_POINTER (DCBTOOL), error); + if (success) { + /* Only report FCoE errors if DCB cleanup was successful */ + success = _fcoe_cleanup (iface, run_helper, GUINT_TO_POINTER (FCOEADM), success ? error : NULL); + } + + return success; } From 32670b52647046536f876b974480059a3d2c4c02 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Wed, 19 Mar 2014 15:39:51 -0500 Subject: [PATCH 4/5] dcb: ignore fcoeadm success errors like "Connection already created" $ /usr/sbin/fcoeadm -m fabric -c enp3s0f0 fcoeadm: Connection already created on interface enp3s0f0 Try 'fcoeadm --help' for more information. $ echo $? 3 $ Also now log error output of failed commands instead of only when debug logging is enabled. --- src/nm-dcb.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/nm-dcb.c b/src/nm-dcb.c index d29332f78..cef7ed993 100644 --- a/src/nm-dcb.c +++ b/src/nm-dcb.c @@ -315,11 +315,19 @@ run_helper (char **argv, guint which, gpointer user_data, GError **error) &outmsg, &errmsg, &exit_status, error); /* Log any stderr output */ if (success && WIFEXITED (exit_status) && WEXITSTATUS (exit_status) && (errmsg || outmsg)) { - nm_log_dbg (LOGD_DCB, "'%s' failed: '%s'", - cmdline, (errmsg && strlen (errmsg)) ? errmsg : outmsg); - g_set_error (error, NM_DCB_ERROR, NM_DCB_ERROR_HELPER_FAILED, - "Failed to run '%s'", cmdline); - success = FALSE; + gboolean ignore_error = FALSE; + + /* Ignore fcoeadm "success" errors like when FCoE is already set up */ + if (errmsg && strstr (errmsg, "Connection already created")) + ignore_error = TRUE; + + if (ignore_error == FALSE) { + nm_log_warn (LOGD_DCB, "'%s' failed: '%s'", + cmdline, (errmsg && strlen (errmsg)) ? errmsg : outmsg); + g_set_error (error, NM_DCB_ERROR, NM_DCB_ERROR_HELPER_FAILED, + "Failed to run '%s'", cmdline); + success = FALSE; + } } g_free (outmsg); g_free (errmsg); From ebc06a001582b780ee29ab4eea838042300b46aa Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Thu, 20 Mar 2014 12:39:44 -0500 Subject: [PATCH 5/5] dcb: turn off all DCB features when disabling DCB Don't just disable DCB, but turn off the features too. --- src/nm-dcb.c | 22 ++++++++++++++++++++-- src/tests/test-dcb.c | 8 +++++++- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/nm-dcb.c b/src/nm-dcb.c index cef7ed993..d937d341d 100644 --- a/src/nm-dcb.c +++ b/src/nm-dcb.c @@ -228,8 +228,26 @@ _dcb_cleanup (const char *iface, gpointer user_data, GError **error) { - /* FIXME: do we need to turn off features individually here? */ - return do_helper (iface, DCBTOOL, run_func, user_data, error, "dcb off"); + const char *cmds[] = { + "dcb off", + "app:fcoe e:0", + "app:iscsi e:0", + "app:fip e:0", + "pfc e:0", + "pg e:0", + NULL + }; + const char **iter = cmds; + gboolean success = TRUE; + + /* Turn everything off and return first error we get (if any) */ + while (iter && *iter) { + if (!do_helper (iface, DCBTOOL, run_func, user_data, success ? error : NULL, *iter)) + success = FALSE; + iter++; + } + + return success; } gboolean diff --git a/src/tests/test-dcb.c b/src/tests/test-dcb.c index c950ac6d0..5763507f2 100644 --- a/src/tests/test-dcb.c +++ b/src/tests/test-dcb.c @@ -268,7 +268,13 @@ static void test_dcb_cleanup (void) { static DcbExpected expected = { 0, - { "dcbtool sc eth0 dcb off", NULL }, + { "dcbtool sc eth0 dcb off", + "dcbtool sc eth0 app:fcoe e:0", + "dcbtool sc eth0 app:iscsi e:0", + "dcbtool sc eth0 app:fip e:0", + "dcbtool sc eth0 pfc e:0", + "dcbtool sc eth0 pg e:0", + NULL }, }; GError *error = NULL; gboolean success;