[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.
>

Won't that data be missing then?

I would format the subject line as "don't fail if..." to make it brief
and clean, but that's just a nit.  Anyway, I see multiple problems
with this patch.

But first let me ask a question; how come there are no PortData on the
destination, when we set them unconditionally?  I mean how did you get
to this problem in the first place?

>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(-)
>
>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.*/
>+        if (strstr(errbuf, "no key \"PortData\" in Interface record")) {
>+            VIR_WARN("Can't find OVS port data for interface %s", ifname);
>+            if (*migrate)
>+                VIR_FREE(*migrate);
>+            ret = 0;
>+        } else {
>+            virReportError(VIR_ERR_INTERNAL_ERROR,
>+                                 _("Unable to run command to get OVS port data for "
>+                                 "interface %s"), ifname);
>+        }

If there are no PortData set, then why don't we get that info from the
cookie and run the ovs-vsctl based on that?

>         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) {

We don't use Yoda conditions, sorry.

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

this is not candidate for VIR_INFO(), rather for VIR_WARN(), but it
shouldn't be needed.  How can we get here with migrate == NULL?

I'm afraid I have to NACK this patch for now, but it might be caused
by some missing ovs knowledge (highly possible).  Feel free to correct
me, though.

>+        return 0;
>+    }
>+
>     cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "set",
>                                "Interface", ifname, NULL);
>     virCommandAddArgFormat(cmd, "external_ids:PortData=%s", migrate);
>--
>1.7.12.4
>
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- 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/20150220/2c4a98fe/attachment-0001.sig>