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

Martin Kletzander <mkletzan@xxxxxxxxxx> · Fri, 20 Feb 2015 11:05:05 +0100

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@xxxxxxxxxx>
Signed-off-by: Zhou Yimin <zhouyimin@xxxxxxxxxx>
---
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
pgp6umIDFMDUt.pgp

Description: PGP signature
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



