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

2015-02-26 Thread 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.zhan...@huawei.com
Signed-off-by: Zhou Yimin zhouyi...@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


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

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

2015-02-26 Thread Martin Kletzander 

On Thu, Feb 26, 2015 at 10:25:45AM +0100, Martin Kletzander wrote:

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.zhan...@huawei.com
Signed-off-by: Zhou Yimin zhouyi...@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);


Yet another idea, what if you used --if-exists get Interface ... and
then just checked for the empty line?  The code would be cleaner as well.


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

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

2015-02-25 Thread zhang bo 
On 2015/2/20 18:05, Martin Kletzander wrote:

 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.

 

Sorry for replying so late, we were having Spring-Festival holiday then:)

 Won't that data be missing then?

Yes, the portdata will be missing, but as that it's *OPTIONAL*, missing is not 
a big
deal.
We can tell that it's optional from the comments in 
qemuMigrationCookieNetworkXMLParse():
port data is optional, and may not exist.
It's optional as well as other external_ids for an ovs interface.

 
 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.
 

I would modify that if this patch is decided to be acceptable later.

 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?
 

Sorry for missing the detailed description of the original problem.

The problem was actually:
  After multiple times of migrating a domain, which has an ovs interface
with no portData set, with non-shared disk, nbd ports got overflowed.

The steps to reproduce the problem:
1 define and start a domain with its network configured as:
interface type='bridge'
  source bridge='br0'/
  virtualport type='openvswitch'
  /virtualport
  model type='virtio'/
  driver name='vhost' queues='4'/
/interface
2 do not set the network's portData.
3 migrate(ToURI2) it with flag 91(1011011), which means:
  VIR_MIGRATE_LIVE
  VIR_MIGRATE_PEER2PEER
  VIR_MIGRATE_PERSIST_DEST
  VIR_MIGRATE_UNDEFINE_SOURCE
  VIR_MIGRATE_NON_SHARED_DISK
4 migrate success, but we got an error log in libvirtd.log:
  error : virCommandWait:2423 : internal error: Child process (ovs-vsctl 
--timeout=5
get Interface vnet1 external_ids:PortData) unexpected exit status 1: ovs-vsctl: 
no
key PortData in Interface record vnet1 column external_ids
5 migrate it back, migrate it , migrate it back, ...
6 nbd port got overflowed.

The nbd port problem is directly caused by the problem in step 4. In fact the 
patch here just fixed
 problem 4. If we want to totally solve problem in step 6, lots more work has 
to be done.
 We'll study the nbd problem later. If you have any suggestion, please let me 
know, thanks.

The reasons for the problem is :
1 virNetDevOpenvswitchGetMigrateData() takes it as wrong if no portData is 
available for
 the ovs interface of a domain. (We think it's not appropriate, as portData is 
just OPTIONAL)
2 in func qemuMigrationBakeCookie(), it fails in 
qemuMigrationCookieAddNetwork(), and returns
 with -1. qemuMigrationCookieAddNBD() is not called thereafter, and mig-nbd is 
still NULL.
3 However, qemuMigrationRun() just *WARN* if qemuMigrationBakeCookie() fails, 
migration still successes.
 cookie is NULL, it's not baked on the src side.
4 On the destination side, it would alloc a port first and then free the nbd 
port in COOKIE.
 But the cookie is NULL due to qemuMigrationCookieAddNetwork() failure at src 
side. thus the nbd port
 is not freed.


 Signed-off-by: Zhang Bo oscar.zhan...@huawei.com
 Signed-off-by: Zhou Yimin zhouyi...@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)

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

2015-02-20 Thread 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.zhan...@huawei.com
Signed-off-by: Zhou Yimin zhouyi...@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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


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

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

2015-02-11 Thread Zhang Bo 
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.zhan...@huawei.com
Signed-off-by: Zhou Yimin zhouyi...@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);
+}
 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) {
+VIR_INFO(There is no need to set OVS port data for interface %s, 
ifname);
+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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list