[libvirt] [PATCH 0/4] more memory leak patches
More in the ongoing battle to make libvirtd not so leaky. Also related, and still waiting on a review for: virExec (and thus virCommand) have undefined behavior https://www.redhat.com/archives/libvir-list/2010-December/msg00242.html avoid matchpathcon memory leak (although libselinux 2.0.97 finally patched that leak upstream) https://www.redhat.com/archives/libvir-list/2010-December/msg00001.html The leak in qemu_conf covered in patch 3 was introduced with the conversion to virCommand; if you like patch 4, I'd rather combine those into one patch (but patch 3 is a minimal plug). Eric Blake (4): tests: plug memory leaks conf: plug memory leaks qemu: plug memory leak command: ease use with virBuffer src/conf/domain_conf.c | 10 ++++++++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_conf.c | 42 +++++++++--------------------------------- src/util/command.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/util/command.h | 17 +++++++++++++++++ tests/qemuxml2argvtest.c | 3 +-- 6 files changed, 82 insertions(+), 35 deletions(-) -- 1.7.3.2
* tests/qemuxml2argvtest.c (testCompareXMLToArgvFiles): Don't allocate, since we don't use virDomainChrDefFree. --- Makes it easier to run valgrind on qemuxml2argv to spot the leaks in libvirt proper. tests/qemuxml2argvtest.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index db2d006..5387432 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -86,8 +86,7 @@ static int testCompareXMLToArgvFiles(const char *xml, monitor_chr.type = VIR_DOMAIN_CHR_TYPE_UNIX; monitor_chr.data.nix.path = (char *)"/tmp/test-monitor"; monitor_chr.data.nix.listen = 1; - if (!(monitor_chr.info.alias = strdup("monitor"))) - goto fail; + monitor_chr.info.alias = (char *)"monitor"; flags = QEMUD_CMD_FLAG_VNC_COLON | QEMUD_CMD_FLAG_NO_REBOOT | -- 1.7.3.2
On 12/10/2010 02:17 PM, Eric Blake wrote:
* tests/qemuxml2argvtest.c (testCompareXMLToArgvFiles): Don't allocate, since we don't use virDomainChrDefFree. ---
Makes it easier to run valgrind on qemuxml2argv to spot the leaks in libvirt proper.
tests/qemuxml2argvtest.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index db2d006..5387432 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -86,8 +86,7 @@ static int testCompareXMLToArgvFiles(const char *xml, monitor_chr.type = VIR_DOMAIN_CHR_TYPE_UNIX; monitor_chr.data.nix.path = (char *)"/tmp/test-monitor"; monitor_chr.data.nix.listen = 1; - if (!(monitor_chr.info.alias = strdup("monitor"))) - goto fail; + monitor_chr.info.alias = (char *)"monitor";
flags = QEMUD_CMD_FLAG_VNC_COLON | QEMUD_CMD_FLAG_NO_REBOOT |
Well, to be totally safe and correct with this, you might instead want to create a function virDomainChrDefClear(), derived from virDomainChrDefFree(), and call that when you're done with monitor_chr (or just ALLOC monitor_chr instead of putting it on the stack, then call virDomainChrDefFree(). I'm okay with this fix anyway, as it stays within the spirit of the original code, and technically does fix the problem, so ACK.
On 12/10/2010 01:07 PM, Laine Stump wrote:
+++ b/tests/qemuxml2argvtest.c @@ -86,8 +86,7 @@ static int testCompareXMLToArgvFiles(const char *xml, monitor_chr.type = VIR_DOMAIN_CHR_TYPE_UNIX; monitor_chr.data.nix.path = (char *)"/tmp/test-monitor"; monitor_chr.data.nix.listen = 1; - if (!(monitor_chr.info.alias = strdup("monitor"))) - goto fail; + monitor_chr.info.alias = (char *)"monitor";
flags = QEMUD_CMD_FLAG_VNC_COLON | QEMUD_CMD_FLAG_NO_REBOOT |
Well, to be totally safe and correct with this, you might instead want to create a function virDomainChrDefClear(),
test code would be the only client derived from
virDomainChrDefFree(), and call that when you're done with monitor_chr (or just ALLOC monitor_chr instead of putting it on the stack, then call virDomainChrDefFree().
That idea crossed my head, then I noticed it would cause a bogus free() of data.nix.path unless I turned that into a strdup() as well.
I'm okay with this fix anyway, as it stays within the spirit of the original code, and technically does fix the problem, so ACK.
Yep, definitely in the spirit of existing code. Pushed as-is. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/conf/domain_conf.c (virDomainGraphicsDefParseXML) (virDomainDeviceVirtioSerialAddressParseXML) (virDomainDiskDefFree): Free various leaks. --- All real leaks in libvirt, and all present in 0.8.6 (if not earlier). src/conf/domain_conf.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5b0fd55..d516fbe 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -524,6 +524,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) for (i = 0 ; i < def->nhosts ; i++) virDomainDiskHostDefFree(&def->hosts[i]); + VIR_FREE(def->hosts); VIR_FREE(def); } @@ -1420,6 +1421,7 @@ virDomainDeviceVirtioSerialAddressParseXML( cleanup: VIR_FREE(controller); VIR_FREE(bus); + VIR_FREE(port); return ret; } @@ -3491,6 +3493,8 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, int flags) { if (!name || !mode) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("spice channel missing name/mode")); + VIR_FREE(name); + VIR_FREE(mode); goto error; } @@ -3498,14 +3502,20 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, int flags) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unknown spice channel name %s"), name); + VIR_FREE(name); + VIR_FREE(mode); goto error; } if ((modeval = virDomainGraphicsSpiceChannelModeTypeFromString(mode)) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unknown spice channel mode %s"), mode); + VIR_FREE(name); + VIR_FREE(mode); goto error; } + VIR_FREE(name); + VIR_FREE(mode); def->data.spice.channels[nameval] = modeval; } -- 1.7.3.2
On 12/10/2010 02:17 PM, Eric Blake wrote:
* src/conf/domain_conf.c (virDomainGraphicsDefParseXML) (virDomainDeviceVirtioSerialAddressParseXML) (virDomainDiskDefFree): Free various leaks. ---
All real leaks in libvirt, and all present in 0.8.6 (if not earlier).
src/conf/domain_conf.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5b0fd55..d516fbe 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -524,6 +524,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
for (i = 0 ; i< def->nhosts ; i++) virDomainDiskHostDefFree(&def->hosts[i]); + VIR_FREE(def->hosts);
VIR_FREE(def); } @@ -1420,6 +1421,7 @@ virDomainDeviceVirtioSerialAddressParseXML( cleanup: VIR_FREE(controller); VIR_FREE(bus); + VIR_FREE(port); return ret; }
@@ -3491,6 +3493,8 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, int flags) { if (!name || !mode) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("spice channel missing name/mode")); + VIR_FREE(name); + VIR_FREE(mode); goto error; }
@@ -3498,14 +3502,20 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, int flags) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unknown spice channel name %s"), name); + VIR_FREE(name); + VIR_FREE(mode); goto error; } if ((modeval = virDomainGraphicsSpiceChannelModeTypeFromString(mode))< 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unknown spice channel mode %s"), mode); + VIR_FREE(name); + VIR_FREE(mode); goto error; } + VIR_FREE(name); + VIR_FREE(mode);
def->data.spice.channels[nameval] = modeval; }
ACK.
On 12/10/2010 12:45 PM, Laine Stump wrote:
On 12/10/2010 02:17 PM, Eric Blake wrote:
* src/conf/domain_conf.c (virDomainGraphicsDefParseXML) (virDomainDeviceVirtioSerialAddressParseXML) (virDomainDiskDefFree): Free various leaks.
ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/qemu/qemu_conf.c (qemudBuildCommandLine): Don't leak rbd_hosts. --- Leak introduced post-0.8.6. src/qemu/qemu_conf.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index fb0b29a..4f19037 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -4765,8 +4765,11 @@ qemudBuildCommandLine(virConnectPtr conn, virBufferFreeAndReset(&rbd_hosts); goto no_memory; } - if (has_rbd_hosts) - virCommandAddEnvPair(cmd, "CEPH_ARGS", virBufferContentAndReset(&rbd_hosts)); + if (has_rbd_hosts) { + char *optstr = virBufferContentAndReset(&rbd_hosts); + virCommandAddEnvPair(cmd, "CEPH_ARGS", optstr); + VIR_FREE(optstr); + } if (qemuCmdFlags & QEMUD_CMD_FLAG_FSDEV) { for (i = 0 ; i < def->nfss ; i++) { @@ -5516,6 +5519,7 @@ qemudBuildCommandLine(virConnectPtr conn, error: for (i = 0; i <= last_good_net; i++) virDomainConfNWFilterTeardown(def->nets[i]); + virBufferFreeAndReset(&rbd_hosts); virCommandFree(cmd); return NULL; } -- 1.7.3.2
On 12/10/2010 02:18 PM, Eric Blake wrote:
* src/qemu/qemu_conf.c (qemudBuildCommandLine): Don't leak rbd_hosts. ---
Leak introduced post-0.8.6.
src/qemu/qemu_conf.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index fb0b29a..4f19037 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -4765,8 +4765,11 @@ qemudBuildCommandLine(virConnectPtr conn, virBufferFreeAndReset(&rbd_hosts); goto no_memory; } - if (has_rbd_hosts) - virCommandAddEnvPair(cmd, "CEPH_ARGS", virBufferContentAndReset(&rbd_hosts)); + if (has_rbd_hosts) { + char *optstr = virBufferContentAndReset(&rbd_hosts); + virCommandAddEnvPair(cmd, "CEPH_ARGS", optstr); + VIR_FREE(optstr); + }
if (qemuCmdFlags& QEMUD_CMD_FLAG_FSDEV) { for (i = 0 ; i< def->nfss ; i++) { @@ -5516,6 +5519,7 @@ qemudBuildCommandLine(virConnectPtr conn, error: for (i = 0; i<= last_good_net; i++) virDomainConfNWFilterTeardown(def->nets[i]); + virBufferFreeAndReset(&rbd_hosts); virCommandFree(cmd); return NULL; }
ACK
* src/util/command.h (virCommandAddArgBuffer) (virCommandAddEnvBuffer): New prototypes. * src/util/command.c (virCommandAddArgBuffer) (virCommandAddEnvBuffer): Implement them. * src/libvirt_private.syms (command.h): Export them. * src/qemu/qemu_conf.c (qemudBuildCommandLine): Use them, plugging a memory leak on rbd_hosts in the process. --- Transferring malloc'd contents from one struct to another can be tricky; hopefully this is easy enough to understand and hard enough to abuse to warrant the syntactic shorthand that it provides to qemu_conf. It helps that this is a transfer from one robust wrapper to another; since we do NOT want something more direct, like: /* Add malloc'd str to cmd, transferring ownership of who should free it */ void virCommandTransferArg(virCommandPtr cmd, char *str); because that just invites bugs from freeing in the wrong place. src/libvirt_private.syms | 2 ++ src/qemu/qemu_conf.c | 46 +++++++++------------------------------------- src/util/command.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/util/command.h | 17 +++++++++++++++++ 4 files changed, 71 insertions(+), 37 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c5a114c..b24ca70 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -85,10 +85,12 @@ virCgroupSetSwapHardLimit; # command.h virCommandAddArg; +virCommandAddArgBuffer; virCommandAddArgFormat; virCommandAddArgList; virCommandAddArgPair; virCommandAddArgSet; +virCommandAddEnvBuffer; virCommandAddEnvPair; virCommandAddEnvPass; virCommandAddEnvPassCommon; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 4f19037..f4b524e 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -4466,7 +4466,6 @@ qemudBuildCommandLine(virConnectPtr conn, } if (def->os.nBootDevs) { virBuffer boot_buf = VIR_BUFFER_INITIALIZER; - char *bootstr; virCommandAddArg(cmd, "-boot"); boot[def->os.nBootDevs] = '\0'; @@ -4481,14 +4480,7 @@ qemudBuildCommandLine(virConnectPtr conn, virBufferVSprintf(&boot_buf, "%s", boot); } - if (virBufferError(&boot_buf)) { - virReportOOMError(); - goto error; - } - - bootstr = virBufferContentAndReset(&boot_buf); - virCommandAddArg(cmd, bootstr); - VIR_FREE(bootstr); + virCommandAddArgBuffer(cmd, &boot_buf); } if (def->os.kernel) @@ -4622,7 +4614,7 @@ qemudBuildCommandLine(virConnectPtr conn, disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { for (j = 0 ; j < disk->nhosts ; j++) { if (!has_rbd_hosts) { - virBufferAddLit(&rbd_hosts, "-m "); + virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m "); has_rbd_hosts = true; } else { virBufferAddLit(&rbd_hosts, ","); @@ -4727,7 +4719,7 @@ qemudBuildCommandLine(virConnectPtr conn, snprintf(file, PATH_MAX, "rbd:%s,", disk->src); for (j = 0 ; j < disk->nhosts ; j++) { if (!has_rbd_hosts) { - virBufferAddLit(&rbd_hosts, "-m "); + virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m "); has_rbd_hosts = true; } else { virBufferAddLit(&rbd_hosts, ","); @@ -4761,15 +4753,8 @@ qemudBuildCommandLine(virConnectPtr conn, } } - if (virBufferError(&rbd_hosts)) { - virBufferFreeAndReset(&rbd_hosts); - goto no_memory; - } - if (has_rbd_hosts) { - char *optstr = virBufferContentAndReset(&rbd_hosts); - virCommandAddEnvPair(cmd, "CEPH_ARGS", optstr); - VIR_FREE(optstr); - } + if (has_rbd_hosts) + virCommandAddEnvBuffer(cmd, &rbd_hosts); if (qemuCmdFlags & QEMUD_CMD_FLAG_FSDEV) { for (i = 0 ; i < def->nfss ; i++) { @@ -5082,7 +5067,6 @@ qemudBuildCommandLine(virConnectPtr conn, if ((def->ngraphics == 1) && def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { virBuffer opt = VIR_BUFFER_INITIALIZER; - char *optstr; if (qemuCmdFlags & QEMUD_CMD_FLAG_VNC_COLON) { if (def->graphics[0]->data.vnc.listenAddr) @@ -5121,15 +5105,9 @@ qemudBuildCommandLine(virConnectPtr conn, virBufferVSprintf(&opt, "%d", def->graphics[0]->data.vnc.port - 5900); } - if (virBufferError(&opt)) { - virBufferFreeAndReset(&opt); - goto no_memory; - } - - optstr = virBufferContentAndReset(&opt); - virCommandAddArgList(cmd, "-vnc", optstr, NULL); - VIR_FREE(optstr); + virCommandAddArg(cmd, "-vnc"); + virCommandAddArgBuffer(cmd, &opt); if (def->graphics[0]->data.vnc.keymap) { virCommandAddArgList(cmd, "-k", def->graphics[0]->data.vnc.keymap, NULL); @@ -5171,7 +5149,6 @@ qemudBuildCommandLine(virConnectPtr conn, } else if ((def->ngraphics == 1) && def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { virBuffer opt = VIR_BUFFER_INITIALIZER; - char *optstr; if (!(qemuCmdFlags & QEMUD_CMD_FLAG_SPICE)) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -5214,13 +5191,8 @@ qemudBuildCommandLine(virConnectPtr conn, } } - if (virBufferError(&opt)) - goto no_memory; - - optstr = virBufferContentAndReset(&opt); - - virCommandAddArgList(cmd, "-spice", optstr, NULL); - VIR_FREE(optstr); + virCommandAddArg(cmd, "-spice"); + virCommandAddArgBuffer(cmd, &opt); if (def->graphics[0]->data.spice.keymap) virCommandAddArgList(cmd, "-k", def->graphics[0]->data.spice.keymap, NULL); diff --git a/src/util/command.c b/src/util/command.c index 5e2b19a..90c0d3a 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -315,6 +315,28 @@ virCommandAddEnvString(virCommandPtr cmd, const char *str) /* + * Convert a buffer containing preformatted name=value into an + * environment variable of the child + */ +void +virCommandAddEnvBuffer(virCommandPtr cmd, virBufferPtr buf) +{ + if (!cmd || cmd->has_error) + return; + + /* env plus trailing NULL. */ + if (virBufferError(buf) || + VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1) < 0) { + cmd->has_error = ENOMEM; + virBufferFreeAndReset(buf); + return; + } + + cmd->env[cmd->nenv++] = virBufferContentAndReset(buf); +} + + +/* * Pass an environment variable to the child * using current process' value */ @@ -381,6 +403,27 @@ virCommandAddArg(virCommandPtr cmd, const char *val) /* + * Convert a buffer into a command line argument to the child + */ +void +virCommandAddArgBuffer(virCommandPtr cmd, virBufferPtr buf) +{ + if (!cmd || cmd->has_error) + return; + + /* Arg plus trailing NULL. */ + if (virBufferError(buf) || + VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1) < 0) { + cmd->has_error = ENOMEM; + virBufferFreeAndReset(buf); + return; + } + + cmd->args[cmd->nargs++] = virBufferContentAndReset(buf); +} + + +/* * Add a command line argument created by a printf-style format */ void diff --git a/src/util/command.h b/src/util/command.h index 9b04e68..59d0ee3 100644 --- a/src/util/command.h +++ b/src/util/command.h @@ -24,6 +24,7 @@ # include "internal.h" # include "util.h" +# include "buf.h" typedef struct _virCommand virCommand; typedef virCommand *virCommandPtr; @@ -110,6 +111,15 @@ void virCommandAddEnvPair(virCommandPtr cmd, */ void virCommandAddEnvString(virCommandPtr cmd, const char *str) ATTRIBUTE_NONNULL(2); + +/* + * Convert a buffer containing preformatted name=value into an + * environment variable of the child. + * Correctly transfers memory errors or contents from buf to cmd. + */ +void virCommandAddEnvBuffer(virCommandPtr cmd, + virBufferPtr buf); + /* * Pass an environment variable to the child * using current process' value @@ -129,6 +139,13 @@ void virCommandAddArg(virCommandPtr cmd, const char *val) ATTRIBUTE_NONNULL(2); /* + * Convert a buffer into a command line argument to the child. + * Correctly transfers memory errors or contents from buf to cmd. + */ +void virCommandAddArgBuffer(virCommandPtr cmd, + virBufferPtr buf); + +/* * Add a command line argument created by a printf-style format */ void virCommandAddArgFormat(virCommandPtr cmd, -- 1.7.3.2
On 12/10/2010 02:18 PM, Eric Blake wrote:
* src/util/command.h (virCommandAddArgBuffer) (virCommandAddEnvBuffer): New prototypes. * src/util/command.c (virCommandAddArgBuffer) (virCommandAddEnvBuffer): Implement them. * src/libvirt_private.syms (command.h): Export them. * src/qemu/qemu_conf.c (qemudBuildCommandLine): Use them, plugging a memory leak on rbd_hosts in the process. ---
Transferring malloc'd contents from one struct to another can be tricky; hopefully this is easy enough to understand and hard enough to abuse to warrant the syntactic shorthand that it provides to qemu_conf. It helps that this is a transfer from one robust wrapper to another; since we do NOT want something more direct, like:
/* Add malloc'd str to cmd, transferring ownership of who should free it */ void virCommandTransferArg(virCommandPtr cmd, char *str);
because that just invites bugs from freeing in the wrong place.
Well, you *could* make it less error_prone by having it be a macro which NULLed out the original pointer as a part of the transfer. That would still leave the possibility of people sending literal strings, or buffers allocated on the stack, as str, leading to virCommand attempting to free something that wasn't malloc'ed, so it's just as well to not even give them the chance ;-) BTW, I'm really loving the ability to just forget about error checking until after all the args are added. It really cuts down on the lines of code, as well as making it easier to follow what's really happening.
src/libvirt_private.syms | 2 ++ src/qemu/qemu_conf.c | 46 +++++++++------------------------------------- src/util/command.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/util/command.h | 17 +++++++++++++++++ 4 files changed, 71 insertions(+), 37 deletions(-)
ACK. The new APIs and their uses all look fine to me.
On 12/10/2010 12:58 PM, Laine Stump wrote:
On 12/10/2010 02:18 PM, Eric Blake wrote:
* src/util/command.h (virCommandAddArgBuffer) (virCommandAddEnvBuffer): New prototypes. * src/util/command.c (virCommandAddArgBuffer) (virCommandAddEnvBuffer): Implement them. * src/libvirt_private.syms (command.h): Export them. * src/qemu/qemu_conf.c (qemudBuildCommandLine): Use them, plugging a memory leak on rbd_hosts in the process. ---
BTW, I'm really loving the ability to just forget about error checking until after all the args are added. It really cuts down on the lines of code, as well as making it easier to follow what's really happening.
Me too!
src/libvirt_private.syms | 2 ++ src/qemu/qemu_conf.c | 46 +++++++++------------------------------------- src/util/command.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/util/command.h | 17 +++++++++++++++++ 4 files changed, 71 insertions(+), 37 deletions(-)
ACK. The new APIs and their uses all look fine to me.
Actually, I noticed a minor problem. I squashed 3 and 4 together, then squashed this in, then pushed: diff --git i/src/util/command.c w/src/util/command.c index 90c0d3a..f9d475e 100644 --- i/src/util/command.c +++ w/src/util/command.c @@ -316,13 +316,16 @@ virCommandAddEnvString(virCommandPtr cmd, const char *str) /* * Convert a buffer containing preformatted name=value into an - * environment variable of the child + * environment variable of the child. + * Correctly transfers memory errors or contents from buf to cmd. */ void virCommandAddEnvBuffer(virCommandPtr cmd, virBufferPtr buf) { - if (!cmd || cmd->has_error) + if (!cmd || cmd->has_error) { + virBufferFreeAndReset(buf); return; + } /* env plus trailing NULL. */ if (virBufferError(buf) || @@ -403,13 +406,16 @@ virCommandAddArg(virCommandPtr cmd, const char *val) /* - * Convert a buffer into a command line argument to the child + * Convert a buffer into a command line argument to the child. + * Correctly transfers memory errors or contents from buf to cmd. */ void virCommandAddArgBuffer(virCommandPtr cmd, virBufferPtr buf) { - if (!cmd || cmd->has_error) + if (!cmd || cmd->has_error) { + virBufferFreeAndReset(buf); return; + } /* Arg plus trailing NULL. */ if (virBufferError(buf) || -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Laine Stump