[libvirt] [PATCH] util: do not take it as wrong if no PortData is found while getting migrateData

[Martin Kletzander]

On Thu, Feb 12, 2015 at 12:08:54PM +0800, Zhang Bo wrote:
>The function virNetDevOpenvswitchGetMigrateData() uses the cmd ovs-vsctl to
>get portdata. If no portdata is available, rather than failure in running
>the cmd, we think we should just print a warning message here, rather than
>taking it as wrong, because PortData is just optional for an ovs interface.
>If we take it as wrong anyway, in function qemuMigrationBakeCookie() it will
>terminate in the middle, and cookies such as NBD would not be baked, further
>more errors would happen, such as nbd ports get overflowed, etc.
>Thus, we add an if branch in virNetDevOpenvswitchGetMigrateData() to just warn
>if portdata is unavailable.
>
>Signed-off-by: Zhang Bo <oscar.zhangbo at huawei.com>
>Signed-off-by: Zhou Yimin <zhouyimin at huawei.com>
>---
> src/util/virnetdevopenvswitch.c | 28 ++++++++++++++++++++++++----
> 1 file changed, 24 insertions(+), 4 deletions(-)
>

In my previous review I mixed what Set/Get functions do, so that's why
that made way less sense for me :)

Anyway, review v2:

>diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
>index e5c87bb..6116377 100644
>--- a/src/util/virnetdevopenvswitch.c
>+++ b/src/util/virnetdevopenvswitch.c
>@@ -30,9 +30,12 @@
> #include "virerror.h"
> #include "virmacaddr.h"
> #include "virstring.h"
>+#include "virlog.h"
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
>+VIR_LOG_INIT("util.netdevopenvswitch");
>+
> /**
>  * virNetDevOpenvswitchAddPort:
>  * @brname: the bridge name
>@@ -204,18 +207,29 @@ int virNetDevOpenvswitchRemovePort(const char *brname ATTRIBUTE_UNUSED, const ch
> int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname)
> {
>     virCommandPtr cmd = NULL;
>+    char *errbuf = NULL;
>     int ret = -1;
>
>     cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "get", "Interface",
>                                ifname, "external_ids:PortData", NULL);
>
>     virCommandSetOutputBuffer(cmd, migrate);
>+    virCommandSetErrorBuffer(cmd, &errbuf);
>
>     /* Run the command */
>-    if (virCommandRun(cmd, NULL) < 0) {
>-        virReportError(VIR_ERR_INTERNAL_ERROR,
>-                       _("Unable to run command to get OVS port data for "
>-                         "interface %s"), ifname);
>+    ret = virCommandRun(cmd, NULL);
>+    if (ret < 0) {
>+        /*PortData is optional, thus do not take it as wrong if the PortData is not found.*/

Missing space after asterisk and the line is long.  Either wrap it or
change to:

  /* PortData is optional, don't fail if there are none. */

>+        if (strstr(errbuf, "no key \"PortData\" in Interface record")) {
>+            VIR_WARN("Can't find OVS port data for interface %s", ifname);

Thinking about these two messages, VIR_DEBUG() would be enough, so we
don't spam the logs with useless info.

>+            if (*migrate)
>+                VIR_FREE(*migrate);

"make syntax-check" would tell you that you don't have to do this and
it would fail making sure you'll fix it ;)  Just remove the if,
VIR_FREE is safe for NULLs.

>+            ret = 0;
>+        } else {
>+            virReportError(VIR_ERR_INTERNAL_ERROR,
>+                                 _("Unable to run command to get OVS port data for "
>+                                 "interface %s"), ifname);

Indentation is off here.

>+        }
>         goto cleanup;
>     }
>
>@@ -223,6 +237,7 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname)
>     (*migrate)[strlen(*migrate) - 1] = '\0';
>     ret = 0;
>  cleanup:
>+    VIR_FREE(errbuf);
>     virCommandFree(cmd);
>     return ret;
> }
>@@ -241,6 +256,11 @@ int virNetDevOpenvswitchSetMigrateData(char *migrate, const char *ifname)
>     virCommandPtr cmd = NULL;
>     int ret = -1;
>
>+    if (NULL == migrate) {

Change it to (!migrate).

>+        VIR_INFO("There is no need to set OVS port data for interface %s", ifname);

Same as above with VIR_WARN().

>+        return 0;
>+    }
>+
>     cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "set",
>                                "Interface", ifname, NULL);
>     virCommandAddArgFormat(cmd, "external_ids:PortData=%s", migrate);
>--
>1.7.12.4
>

Anyway, even with those changes reflected in v2 I don't feel confident
to ACK this even though the code looks perfectly OK for me, so I'd
like a second opinion from someone more OVS savvy.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150226/cc7e9107/attachment-0001.sig>