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(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library
http://libvirt.org