[libvirt] [PATCH] Fix a crash when using Open vSwitch virtual ports
Fixup buffer usage when handling VLANs. Also fix the logic used to determine if the virNetDevVlanPtr is valid or not. Fixes crashes in the latest code when using Open vSwitch virtualports. Signed-off-by: Kyle Mestery <kmestery@cisco.com> --- src/util/virnetdevopenvswitch.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index b903ae4..cdbc5ef 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -59,7 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, char *ifaceid_ex_id = NULL; char *profile_ex_id = NULL; char *vmid_ex_id = NULL; - virBufferPtr buf; + virBuffer buf = VIR_BUFFER_INITIALIZER; virMacAddrFormat(macaddr, macaddrstr); virUUIDFormat(ovsport->interfaceID, ifuuidstr); @@ -79,13 +79,12 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ovsport->profileID) < 0) goto out_of_memory; } - if (virtVlan) { - if (VIR_ALLOC(buf) < 0) - goto out_of_memory; + + if (virtVlan && virtVlan->nTags > 0) { /* Trunk port first */ - if (virtVlan->trunk) { - virBufferAddLit(buf, "trunk="); + if (virtVlan->trunk == true) { + virBufferAddLit(&buf, "trunk="); /* * Trunk ports have at least one VLAN. Do the first one @@ -93,21 +92,21 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, * start of the for loop if there are more than one VLANs * on this trunk port. */ - virBufferAsprintf(buf, "%d", virtVlan->tag[i]); + virBufferAsprintf(&buf, "%d", virtVlan->tag[i]); for (i = 1; i < virtVlan->nTags; i++) { - virBufferAddLit(buf, ","); - virBufferAsprintf(buf, "%d", virtVlan->tag[i]); + virBufferAddLit(&buf, ","); + virBufferAsprintf(&buf, "%d", virtVlan->tag[i]); } } else if (virtVlan->nTags) { - virBufferAsprintf(buf, "tag=%d", virtVlan->tag[0]); + virBufferAsprintf(&buf, "tag=%d", virtVlan->tag[0]); } } cmd = virCommandNew(OVSVSCTL); if (ovsport->profileID[0] == '\0') { virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferContentAndReset(buf), + brname, ifname, virBufferCurrentContent(&buf), "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, @@ -116,7 +115,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, NULL); } else { virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferContentAndReset(buf), + brname, ifname, virBufferCurrentContent(&buf), "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, @@ -135,7 +134,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ret = 0; cleanup: - VIR_FREE(buf); + if (virBufferUse(&buf) > 0) + virBufferFreeAndReset(&buf); VIR_FREE(attachedmac_ex_id); VIR_FREE(ifaceid_ex_id); VIR_FREE(vmid_ex_id); -- 1.7.11.4
On 08/30/2012 02:44 AM, Kyle Mestery wrote:
Fixup buffer usage when handling VLANs. Also fix the logic used to determine if the virNetDevVlanPtr is valid or not. Fixes crashes in the latest code when using Open vSwitch virtualports.
Hi Kyle, I just waive my patch then apply Laine and your patch, it works well for me now, so give a ACK. Regards, Alex
Signed-off-by: Kyle Mestery<kmestery@cisco.com> --- src/util/virnetdevopenvswitch.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index b903ae4..cdbc5ef 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -59,7 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, char *ifaceid_ex_id = NULL; char *profile_ex_id = NULL; char *vmid_ex_id = NULL; - virBufferPtr buf; + virBuffer buf = VIR_BUFFER_INITIALIZER;
virMacAddrFormat(macaddr, macaddrstr); virUUIDFormat(ovsport->interfaceID, ifuuidstr); @@ -79,13 +79,12 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ovsport->profileID)< 0) goto out_of_memory; } - if (virtVlan) { - if (VIR_ALLOC(buf)< 0) - goto out_of_memory; + + if (virtVlan&& virtVlan->nTags> 0) {
/* Trunk port first */ - if (virtVlan->trunk) { - virBufferAddLit(buf, "trunk="); + if (virtVlan->trunk == true) { + virBufferAddLit(&buf, "trunk=");
/* * Trunk ports have at least one VLAN. Do the first one @@ -93,21 +92,21 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, * start of the for loop if there are more than one VLANs * on this trunk port. */ - virBufferAsprintf(buf, "%d", virtVlan->tag[i]); + virBufferAsprintf(&buf, "%d", virtVlan->tag[i]);
for (i = 1; i< virtVlan->nTags; i++) { - virBufferAddLit(buf, ","); - virBufferAsprintf(buf, "%d", virtVlan->tag[i]); + virBufferAddLit(&buf, ","); + virBufferAsprintf(&buf, "%d", virtVlan->tag[i]); } } else if (virtVlan->nTags) { - virBufferAsprintf(buf, "tag=%d", virtVlan->tag[0]); + virBufferAsprintf(&buf, "tag=%d", virtVlan->tag[0]); } }
cmd = virCommandNew(OVSVSCTL); if (ovsport->profileID[0] == '\0') { virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferContentAndReset(buf), + brname, ifname, virBufferCurrentContent(&buf), "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, @@ -116,7 +115,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, NULL); } else { virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferContentAndReset(buf), + brname, ifname, virBufferCurrentContent(&buf), "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, @@ -135,7 +134,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
ret = 0; cleanup: - VIR_FREE(buf); + if (virBufferUse(&buf)> 0) + virBufferFreeAndReset(&buf); VIR_FREE(attachedmac_ex_id); VIR_FREE(ifaceid_ex_id); VIR_FREE(vmid_ex_id);
On 08/29/2012 08:45 PM, Alex Jia wrote:
On 08/30/2012 02:44 AM, Kyle Mestery wrote:
Fixup buffer usage when handling VLANs. Also fix the logic used to determine if the virNetDevVlanPtr is valid or not. Fixes crashes in the latest code when using Open vSwitch virtualports.
Hi Kyle, I just waive my patch then apply Laine and your patch, it works well for me now, so give a ACK.
See my NACK for why we need a v2. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/29/2012 11:44 AM, Kyle Mestery wrote:
Fixup buffer usage when handling VLANs. Also fix the logic used to determine if the virNetDevVlanPtr is valid or not. Fixes crashes in the latest code when using Open vSwitch virtualports.
Signed-off-by: Kyle Mestery <kmestery@cisco.com> ---
It's hard to see the real fix amongst the churn.
src/util/virnetdevopenvswitch.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index b903ae4..cdbc5ef 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -59,7 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, char *ifaceid_ex_id = NULL; char *profile_ex_id = NULL; char *vmid_ex_id = NULL; - virBufferPtr buf; + virBuffer buf = VIR_BUFFER_INITIALIZER;
Yes, the old code risked freeing an uninitialized pointer.
virMacAddrFormat(macaddr, macaddrstr); virUUIDFormat(ovsport->interfaceID, ifuuidstr); @@ -79,13 +79,12 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ovsport->profileID) < 0) goto out_of_memory; } - if (virtVlan) { - if (VIR_ALLOC(buf) < 0) - goto out_of_memory; + + if (virtVlan && virtVlan->nTags > 0) {
This line also impacts behavior, unrelated to the change between 'buf' being heap-allocated vs. stack-allocated...
/* Trunk port first */ - if (virtVlan->trunk) { - virBufferAddLit(buf, "trunk="); + if (virtVlan->trunk == true) {
'cond == true' as a condition is a pointless change, 'cond' is sufficient.
@@ -135,7 +134,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
ret = 0; cleanup: - VIR_FREE(buf); + if (virBufferUse(&buf) > 0) + virBufferFreeAndReset(&buf);
...and this change fixes both a potential memory leak, where the old VIR_FREE(buf) risked leaking the contents of buf, and the potential free(garbage). Note that the new code is wrong as well - you should unconditionally free a virBufferPtr's contents, rather than conditionally freeing it based on virBufferUse() returning non-zero. That is, I think the real patch here would be simply this (untested) version: diff --git i/src/util/virnetdevopenvswitch.c w/src/util/virnetdevopenvswitch.c index b903ae4..e8b9f4a 100644 --- i/src/util/virnetdevopenvswitch.c +++ w/src/util/virnetdevopenvswitch.c @@ -59,7 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, char *ifaceid_ex_id = NULL; char *profile_ex_id = NULL; char *vmid_ex_id = NULL; - virBufferPtr buf; + virBufferPtr buf = NULL; virMacAddrFormat(macaddr, macaddrstr); virUUIDFormat(ovsport->interfaceID, ifuuidstr); @@ -79,7 +79,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ovsport->profileID) < 0) goto out_of_memory; } - if (virtVlan) { + if (virtVlan && virtVlan->tag[0]) { if (VIR_ALLOC(buf) < 0) goto out_of_memory; @@ -135,6 +135,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ret = 0; cleanup: + virBufferFreeAndReset(&buf); VIR_FREE(buf); VIR_FREE(attachedmac_ex_id); VIR_FREE(ifaceid_ex_id); -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/29/2012 10:32 PM, Eric Blake wrote:
On 08/29/2012 11:44 AM, Kyle Mestery wrote:
Fixup buffer usage when handling VLANs. Also fix the logic used to determine if the virNetDevVlanPtr is valid or not. Fixes crashes in the latest code when using Open vSwitch virtualports.
Signed-off-by: Kyle Mestery <kmestery@cisco.com> ---
It's hard to see the real fix amongst the churn.
- virBufferPtr buf; + virBuffer buf = VIR_BUFFER_INITIALIZER;
Yes, the old code risked freeing an uninitialized pointer.
- if (virtVlan) { - if (VIR_ALLOC(buf) < 0) - goto out_of_memory; + + if (virtVlan && virtVlan->nTags > 0) {
This line also impacts behavior, unrelated to the change between 'buf' being heap-allocated vs. stack-allocated...
That said, I _like_ using stack-allocated instead of heap-allocated, but that means that I think there's two separate issues (virtVlan->nTags vs. virBuffer usage cleanup), so maybe this deserves two patches instead of one...
Note that the new code is wrong as well - you should unconditionally free a virBufferPtr's contents, rather than conditionally freeing it based on virBufferUse() returning non-zero.
That is, I think the real patch here would be simply this (untested) version:
This 3-liner to fix the bug at hand would be the first patch, then another to convert to stack-allocation with no semantic changes.
diff --git i/src/util/virnetdevopenvswitch.c w/src/util/virnetdevopenvswitch.c index b903ae4..e8b9f4a 100644 --- i/src/util/virnetdevopenvswitch.c +++ w/src/util/virnetdevopenvswitch.c @@ -59,7 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, char *ifaceid_ex_id = NULL; char *profile_ex_id = NULL; char *vmid_ex_id = NULL; - virBufferPtr buf; + virBufferPtr buf = NULL;
virMacAddrFormat(macaddr, macaddrstr); virUUIDFormat(ovsport->interfaceID, ifuuidstr); @@ -79,7 +79,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ovsport->profileID) < 0) goto out_of_memory; } - if (virtVlan) { + if (virtVlan && virtVlan->tag[0]) { if (VIR_ALLOC(buf) < 0) goto out_of_memory;
@@ -135,6 +135,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
ret = 0; cleanup: + virBufferFreeAndReset(&buf); VIR_FREE(buf); VIR_FREE(attachedmac_ex_id); VIR_FREE(ifaceid_ex_id);
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Aug 29, 2012 at 10:32:38PM -0700, Eric Blake wrote:
On 08/29/2012 11:44 AM, Kyle Mestery wrote:
/* Trunk port first */ - if (virtVlan->trunk) { - virBufferAddLit(buf, "trunk="); + if (virtVlan->trunk == true) {
'cond == true' as a condition is a pointless change, 'cond' is sufficient.
Okay, i pushed a tiny cleanup for this, 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 08/30/2012 01:32 AM, Eric Blake wrote:
[...] That is, I think the real patch here would be simply this (untested) version:
diff --git i/src/util/virnetdevopenvswitch.c w/src/util/virnetdevopenvswitch.c index b903ae4..e8b9f4a 100644 --- i/src/util/virnetdevopenvswitch.c +++ w/src/util/virnetdevopenvswitch.c @@ -59,7 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, char *ifaceid_ex_id = NULL; char *profile_ex_id = NULL; char *vmid_ex_id = NULL; - virBufferPtr buf; + virBufferPtr buf = NULL;
virMacAddrFormat(macaddr, macaddrstr); virUUIDFormat(ovsport->interfaceID, ifuuidstr); @@ -79,7 +79,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ovsport->profileID) < 0) goto out_of_memory; } - if (virtVlan) { + if (virtVlan && virtVlan->tag[0]) {
If you were to believe that virtVlan might be sent with no tags (which isn't the case with my patch applied...), then checking for non-0 virtVLan->tag[0] without checking for non-NULL virtVlan->tag would itself lead to a crash.
On Aug 30, 2012, at 3:52 AM, Laine Stump wrote:
On 08/30/2012 01:32 AM, Eric Blake wrote:
[...] That is, I think the real patch here would be simply this (untested) version:
diff --git i/src/util/virnetdevopenvswitch.c w/src/util/virnetdevopenvswitch.c index b903ae4..e8b9f4a 100644 --- i/src/util/virnetdevopenvswitch.c +++ w/src/util/virnetdevopenvswitch.c @@ -59,7 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, char *ifaceid_ex_id = NULL; char *profile_ex_id = NULL; char *vmid_ex_id = NULL; - virBufferPtr buf; + virBufferPtr buf = NULL;
virMacAddrFormat(macaddr, macaddrstr); virUUIDFormat(ovsport->interfaceID, ifuuidstr); @@ -79,7 +79,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ovsport->profileID) < 0) goto out_of_memory; } - if (virtVlan) { + if (virtVlan && virtVlan->tag[0]) {
If you were to believe that virtVlan might be sent with no tags (which isn't the case with my patch applied...), then checking for non-0 virtVLan->tag[0] without checking for non-NULL virtVlan->tag would itself lead to a crash.
Just saw all the cleanups to this patch and the fact it was pushed. Thanks for all the comments and cleanups to it Erik and Laine! And thanks for pushing it up. Kyle
On Wed, Aug 29, 2012 at 02:44:36PM -0400, Kyle Mestery wrote:
Fixup buffer usage when handling VLANs. Also fix the logic used to determine if the virNetDevVlanPtr is valid or not. Fixes crashes in the latest code when using Open vSwitch virtualports.
Signed-off-by: Kyle Mestery <kmestery@cisco.com> --- src/util/virnetdevopenvswitch.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index b903ae4..cdbc5ef 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -59,7 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, char *ifaceid_ex_id = NULL; char *profile_ex_id = NULL; char *vmid_ex_id = NULL; - virBufferPtr buf; + virBuffer buf = VIR_BUFFER_INITIALIZER;
virMacAddrFormat(macaddr, macaddrstr); virUUIDFormat(ovsport->interfaceID, ifuuidstr); @@ -79,13 +79,12 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ovsport->profileID) < 0) goto out_of_memory; } - if (virtVlan) { - if (VIR_ALLOC(buf) < 0) - goto out_of_memory; + + if (virtVlan && virtVlan->nTags > 0) {
/* Trunk port first */ - if (virtVlan->trunk) { - virBufferAddLit(buf, "trunk="); + if (virtVlan->trunk == true) { + virBufferAddLit(&buf, "trunk=");
/* * Trunk ports have at least one VLAN. Do the first one @@ -93,21 +92,21 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, * start of the for loop if there are more than one VLANs * on this trunk port. */ - virBufferAsprintf(buf, "%d", virtVlan->tag[i]); + virBufferAsprintf(&buf, "%d", virtVlan->tag[i]);
for (i = 1; i < virtVlan->nTags; i++) { - virBufferAddLit(buf, ","); - virBufferAsprintf(buf, "%d", virtVlan->tag[i]); + virBufferAddLit(&buf, ","); + virBufferAsprintf(&buf, "%d", virtVlan->tag[i]); } } else if (virtVlan->nTags) { - virBufferAsprintf(buf, "tag=%d", virtVlan->tag[0]); + virBufferAsprintf(&buf, "tag=%d", virtVlan->tag[0]); } }
cmd = virCommandNew(OVSVSCTL); if (ovsport->profileID[0] == '\0') { virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferContentAndReset(buf), + brname, ifname, virBufferCurrentContent(&buf), "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, @@ -116,7 +115,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, NULL); } else { virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferContentAndReset(buf), + brname, ifname, virBufferCurrentContent(&buf), "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, @@ -135,7 +134,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
ret = 0; cleanup: - VIR_FREE(buf); + if (virBufferUse(&buf) > 0) + virBufferFreeAndReset(&buf);
that looks fine up to here, where we could leak in theory, virBufferUse() return the amount of bytes used in the buffer, not if the buffer was allocated. Sounds to me we can use virBufferFreeAndReset() directly without the test
VIR_FREE(attachedmac_ex_id); VIR_FREE(ifaceid_ex_id); VIR_FREE(vmid_ex_id);
I pushed with that small change, 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 08/30/2012 01:49 PM, Daniel Veillard wrote: Daniel, BTW, To test this patch again, it works for me :)
Fixup buffer usage when handling VLANs. Also fix the logic used to determine if the virNetDevVlanPtr is valid or not. Fixes crashes in the latest code when using Open vSwitch virtualports.
Signed-off-by: Kyle Mestery<kmestery@cisco.com> --- src/util/virnetdevopenvswitch.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index b903ae4..cdbc5ef 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -59,7 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, char *ifaceid_ex_id = NULL; char *profile_ex_id = NULL; char *vmid_ex_id = NULL; - virBufferPtr buf; + virBuffer buf = VIR_BUFFER_INITIALIZER;
virMacAddrFormat(macaddr, macaddrstr); virUUIDFormat(ovsport->interfaceID, ifuuidstr); @@ -79,13 +79,12 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ovsport->profileID)< 0) goto out_of_memory; } - if (virtVlan) { - if (VIR_ALLOC(buf)< 0) - goto out_of_memory; + + if (virtVlan&& virtVlan->nTags> 0) {
/* Trunk port first */ - if (virtVlan->trunk) { - virBufferAddLit(buf, "trunk="); + if (virtVlan->trunk == true) { + virBufferAddLit(&buf, "trunk=");
/* * Trunk ports have at least one VLAN. Do the first one @@ -93,21 +92,21 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, * start of the for loop if there are more than one VLANs * on this trunk port. */ - virBufferAsprintf(buf, "%d", virtVlan->tag[i]); + virBufferAsprintf(&buf, "%d", virtVlan->tag[i]);
for (i = 1; i< virtVlan->nTags; i++) { - virBufferAddLit(buf, ","); - virBufferAsprintf(buf, "%d", virtVlan->tag[i]); + virBufferAddLit(&buf, ","); + virBufferAsprintf(&buf, "%d", virtVlan->tag[i]); } } else if (virtVlan->nTags) { - virBufferAsprintf(buf, "tag=%d", virtVlan->tag[0]); + virBufferAsprintf(&buf, "tag=%d", virtVlan->tag[0]); } }
cmd = virCommandNew(OVSVSCTL); if (ovsport->profileID[0] == '\0') { virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferContentAndReset(buf), + brname, ifname, virBufferCurrentContent(&buf), "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, @@ -116,7 +115,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, NULL); } else { virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferContentAndReset(buf), + brname, ifname, virBufferCurrentContent(&buf), "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, @@ -135,7 +134,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
ret = 0; cleanup: - VIR_FREE(buf); + if (virBufferUse(&buf)> 0) + virBufferFreeAndReset(&buf);
On Wed, Aug 29, 2012 at 02:44:36PM -0400, Kyle Mestery wrote: that looks fine up to here, where we could leak in theory, virBufferUse() return the amount of bytes used in the buffer, not if the buffer was allocated. Sounds to me we can use virBufferFreeAndReset() directly without the test
VIR_FREE(attachedmac_ex_id); VIR_FREE(ifaceid_ex_id); VIR_FREE(vmid_ex_id);
I pushed with that small change,
thanks !
Daniel
participants (6)
-
Alex Jia -
Daniel Veillard -
Eric Blake -
Kyle Mestery -
Kyle Mestery (kmestery)
-
Laine Stump