[libvirt] [PATCH] Fix adding ports to OVS bridges without VLAN tags
The introduction of the new VLAN code, along with the fix from 5e465df6be8bcb00f0b4bff831e91f4042fae272, caused the addition of OVS ports to fail with the following message: ovs-vsctl: 00002|vsctl|ERR|: missing column name This fix takes into account the VLAN arguments are optional, and correctly sets up the command line to run the "ovs-vsctl" command to add ports to the OVS bridge. Signed-off-by: Kyle Mestery <kmestery@cisco.com> CC: Eric Blake <eblake@redhat.com> --- V2: - Use virBufferUse() to check if a buffer is in use. Found by Eric Blake. --- src/util/virnetdevopenvswitch.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 00271a0..764f478 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -104,9 +104,15 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, } cmd = virCommandNew(OVSVSCTL); + + virCommandAddArgList(cmd, "--", "--may-exist", "add-port", + brname, ifname, NULL); + + if (virBufferUse(&buf) != 0) + virCommandAddArgList(cmd, virBufferCurrentContent(&buf), NULL); + if (ovsport->profileID[0] == '\0') { - virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferCurrentContent(&buf), + virCommandAddArgList(cmd, "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, @@ -114,8 +120,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, "external-ids:iface-status=active", NULL); } else { - virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferCurrentContent(&buf), + virCommandAddArgList(cmd, "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, -- 1.7.11.4
On Thu, Aug 30, 2012 at 04:38:06PM -0400, Kyle Mestery wrote:
The introduction of the new VLAN code, along with the fix from 5e465df6be8bcb00f0b4bff831e91f4042fae272, caused the addition of OVS ports to fail with the following message:
ovs-vsctl: 00002|vsctl|ERR|: missing column name
This fix takes into account the VLAN arguments are optional, and correctly sets up the command line to run the "ovs-vsctl" command to add ports to the OVS bridge.
Signed-off-by: Kyle Mestery <kmestery@cisco.com> CC: Eric Blake <eblake@redhat.com> --- V2: - Use virBufferUse() to check if a buffer is in use. Found by Eric Blake. --- src/util/virnetdevopenvswitch.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 00271a0..764f478 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -104,9 +104,15 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, }
cmd = virCommandNew(OVSVSCTL); + + virCommandAddArgList(cmd, "--", "--may-exist", "add-port", + brname, ifname, NULL); + + if (virBufferUse(&buf) != 0) + virCommandAddArgList(cmd, virBufferCurrentContent(&buf), NULL); + if (ovsport->profileID[0] == '\0') { - virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferCurrentContent(&buf), + virCommandAddArgList(cmd, "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, @@ -114,8 +120,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, "external-ids:iface-status=active", NULL); } else { - virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferCurrentContent(&buf), + virCommandAddArgList(cmd, "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id,
Okay, ACK, pushed, thanks ! Still there is something which looks wrong, if we don't have a profileID why do we end up with "" instead of NULL ? I'm seeing various tests for profileID[0] over conf/*.c and util/*.c , and that sounds wrong to me. if there is no data, store NULL ! Then test for profileID instead of profileID[0]. Then there is no risk of a crash because abscence of data led to NULL instead of an empty string, the code is more resilient ! I expect a followup patch cleaning this up, but after 0.10.1 ... thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Aug 30, 2012, at 10:23 PM, Daniel Veillard wrote:
On Thu, Aug 30, 2012 at 04:38:06PM -0400, Kyle Mestery wrote:
The introduction of the new VLAN code, along with the fix from 5e465df6be8bcb00f0b4bff831e91f4042fae272, caused the addition of OVS ports to fail with the following message:
ovs-vsctl: 00002|vsctl|ERR|: missing column name
This fix takes into account the VLAN arguments are optional, and correctly sets up the command line to run the "ovs-vsctl" command to add ports to the OVS bridge.
Signed-off-by: Kyle Mestery <kmestery@cisco.com> CC: Eric Blake <eblake@redhat.com> --- V2: - Use virBufferUse() to check if a buffer is in use. Found by Eric Blake. --- src/util/virnetdevopenvswitch.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 00271a0..764f478 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -104,9 +104,15 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, }
cmd = virCommandNew(OVSVSCTL); + + virCommandAddArgList(cmd, "--", "--may-exist", "add-port", + brname, ifname, NULL); + + if (virBufferUse(&buf) != 0) + virCommandAddArgList(cmd, virBufferCurrentContent(&buf), NULL); + if (ovsport->profileID[0] == '\0') { - virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferCurrentContent(&buf), + virCommandAddArgList(cmd, "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, @@ -114,8 +120,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, "external-ids:iface-status=active", NULL); } else { - virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferCurrentContent(&buf), + virCommandAddArgList(cmd, "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id,
Okay, ACK,
pushed, thanks !
Still there is something which looks wrong, if we don't have a profileID why do we end up with "" instead of NULL ? I'm seeing various tests for profileID[0] over conf/*.c and util/*.c , and that sounds wrong to me. if there is no data, store NULL ! Then test for profileID instead of profileID[0]. Then there is no risk of a crash because abscence of data led to NULL instead of an empty string, the code is more resilient !
I expect a followup patch cleaning this up, but after 0.10.1 ... thanks !
Thanks Daniel, I'll work on the followup patch today. Kyle
Daniel
On Fri, Aug 31, 2012 at 01:32:34PM +0000, Kyle Mestery (kmestery) wrote:
On Aug 30, 2012, at 10:23 PM, Daniel Veillard wrote:
On Thu, Aug 30, 2012 at 04:38:06PM -0400, Kyle Mestery wrote: [...] Still there is something which looks wrong, if we don't have a profileID why do we end up with "" instead of NULL ? I'm seeing various tests for profileID[0] over conf/*.c and util/*.c , and that sounds wrong to me. if there is no data, store NULL ! Then test for profileID instead of profileID[0]. Then there is no risk of a crash because abscence of data led to NULL instead of an empty string, the code is more resilient !
I expect a followup patch cleaning this up, but after 0.10.1 ... thanks !
Thanks Daniel, I'll work on the followup patch today.
No hurry, because I just rolled 0.10.1 out so that won't make it (and it's not urgent). Giving 0.10.1 a try would be nice too, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Aug 31, 2012, at 9:09 AM, Daniel Veillard wrote:
On Fri, Aug 31, 2012 at 01:32:34PM +0000, Kyle Mestery (kmestery) wrote:
On Aug 30, 2012, at 10:23 PM, Daniel Veillard wrote:
On Thu, Aug 30, 2012 at 04:38:06PM -0400, Kyle Mestery wrote: [...] Still there is something which looks wrong, if we don't have a profileID why do we end up with "" instead of NULL ? I'm seeing various tests for profileID[0] over conf/*.c and util/*.c , and that sounds wrong to me. if there is no data, store NULL ! Then test for profileID instead of profileID[0]. Then there is no risk of a crash because abscence of data led to NULL instead of an empty string, the code is more resilient !
I expect a followup patch cleaning this up, but after 0.10.1 ... thanks !
Thanks Daniel, I'll work on the followup patch today.
No hurry, because I just rolled 0.10.1 out so that won't make it (and it's not urgent). Giving 0.10.1 a try would be nice too,
thanks !
Daniel
Hi Daniel: Picking this back up. struct _virNetDevVPortProfile contains the following: /* this member is used when virtPortType == 802.1Qbh|openvswitch */ /* this is a null-terminated character string */ char profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX]; To address your comments around checking for profileID[0], we could make profileID here a pointer, and allocate it when we allocate a struct _virNetDevVPortProfile object. But to me, having a fixed length string in this structure doesn't seem wrong. Copying Laine here as well for his comments, but I'm inclined to leave things as they are. Thanks, Kyle
On Tue, Sep 04, 2012 at 09:03:22PM +0000, Kyle Mestery (kmestery) wrote:
On Aug 31, 2012, at 9:09 AM, Daniel Veillard wrote:
On Fri, Aug 31, 2012 at 01:32:34PM +0000, Kyle Mestery (kmestery) wrote:
On Aug 30, 2012, at 10:23 PM, Daniel Veillard wrote:
On Thu, Aug 30, 2012 at 04:38:06PM -0400, Kyle Mestery wrote: [...] Still there is something which looks wrong, if we don't have a profileID why do we end up with "" instead of NULL ? I'm seeing various tests for profileID[0] over conf/*.c and util/*.c , and that sounds wrong to me. if there is no data, store NULL ! Then test for profileID instead of profileID[0]. Then there is no risk of a crash because abscence of data led to NULL instead of an empty string, the code is more resilient !
I expect a followup patch cleaning this up, but after 0.10.1 ... thanks !
Thanks Daniel, I'll work on the followup patch today.
No hurry, because I just rolled 0.10.1 out so that won't make it (and it's not urgent). Giving 0.10.1 a try would be nice too,
thanks !
Daniel
Hi Daniel:
Picking this back up. struct _virNetDevVPortProfile contains the following:
/* this member is used when virtPortType == 802.1Qbh|openvswitch */ /* this is a null-terminated character string */ char profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX];
To address your comments around checking for profileID[0], we could make profileID here a pointer, and allocate it when we allocate a struct _virNetDevVPortProfile object. But to me, having a fixed length string in this structure doesn't seem wrong. Copying Laine here as well for his comments, but I'm inclined to leave things as they are.
Hum ... okay, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 09/04/2012 05:03 PM, Kyle Mestery (kmestery) wrote:
On Aug 31, 2012, at 9:09 AM, Daniel Veillard wrote:
On Fri, Aug 31, 2012 at 01:32:34PM +0000, Kyle Mestery (kmestery) wrote:
On Aug 30, 2012, at 10:23 PM, Daniel Veillard wrote:
On Thu, Aug 30, 2012 at 04:38:06PM -0400, Kyle Mestery wrote: [...] Still there is something which looks wrong, if we don't have a profileID why do we end up with "" instead of NULL ? I'm seeing various tests for profileID[0] over conf/*.c and util/*.c , and that sounds wrong to me. if there is no data, store NULL ! Then test for profileID instead of profileID[0]. Then there is no risk of a crash because abscence of data led to NULL instead of an empty string, the code is more resilient !
I expect a followup patch cleaning this up, but after 0.10.1 ... thanks !
Thanks Daniel, I'll work on the followup patch today. No hurry, because I just rolled 0.10.1 out so that won't make it (and it's not urgent). Giving 0.10.1 a try would be nice too,
thanks !
Daniel
Hi Daniel:
Picking this back up. struct _virNetDevVPortProfile contains the following:
/* this member is used when virtPortType == 802.1Qbh|openvswitch */ /* this is a null-terminated character string */ char profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX];
To address your comments around checking for profileID[0], we could make profileID here a pointer, and allocate it when we allocate a struct _virNetDevVPortProfile object. But to me, having a fixed length string in this structure doesn't seem wrong. Copying Laine here as well for his comments, but I'm inclined to leave things as they I'm not sure why all the strings in virNetDevVPortProfile were made fixed length, but that's what we ended up with (possibly because the initial fields were all fixed length, and the person adding things that could be variable length wanted to avoid potentially forgetting to free a subordinate memory chunk when freeing a virNetDevVPortProfile? Or maybe netlink really does limit the maximum length of the profileID?). As a matter of fact, that's why the "xxx_specified" booleans are there - because there is no implicit "specified" flag due to a pointer being NULL/non-NULL.
I'm with Daniel in preferring the string attributes of virNetDevVPortProfile to each be a pointer to a separate chunk of memory (and we should maybe do that in a cleanup patch), but in this case the new code added by Kyle is correct.
participants (4)
-
Daniel Veillard -
Kyle Mestery -
Kyle Mestery (kmestery)
-
Laine Stump