On 05/03/2013 08:53 AM, Michal Privoznik wrote:
--- include/libvirt/libvirt.h.in | 10 +- src/qemu/qemu_capabilities.c | 79 ++++---- src/qemu/qemu_cgroup.c | 4 +- src/qemu/qemu_command.c | 419 +++++++++++++++++-------------------------- src/qemu/qemu_conf.c | 58 +++--- src/qemu/qemu_domain.c | 26 ++- src/qemu/qemu_driver.c | 113 ++++-------- src/qemu/qemu_hotplug.c | 15 +- src/qemu/qemu_migration.c | 20 +-- src/qemu/qemu_monitor_json.c | 63 ++----- src/qemu/qemu_monitor_text.c | 15 +- src/qemu/qemu_process.c | 64 +++---- src/remote/remote_driver.c | 2 +- 13 files changed, 333 insertions(+), 555 deletions(-)
Part 3 - the qemu changes
+++ b/src/qemu/qemu_capabilities.c @@ -384,7 +384,8 @@ virQEMUCapsParseMachineTypesStr(const char *output, VIR_REALLOC_N(qemuCaps->machineAliases, qemuCaps->nmachineTypes + 1) < 0) { VIR_FREE(name); VIR_FREE(canonical);
Seems like we could move some of this cleanup...
- goto no_memory; + virReportOOMError(); + goto error; } qemuCaps->nmachineTypes++; if (canonical) { @@ -402,8 +403,7 @@ virQEMUCapsParseMachineTypesStr(const char *output,
return 0;
-no_memory: - virReportOOMError(); +error: return -1;
...here, so that we don't have a 'goto/return'. But that can be a separate patch (especially since you'll be revisiting VIR_REALLOC anyway).
@@ -1736,17 +1728,18 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) goto no_memory; ret->nmachineTypes = qemuCaps->nmachineTypes; for (i = 0 ; i < qemuCaps->nmachineTypes ; i++) { - if (!(ret->machineTypes[i] = strdup(qemuCaps->machineTypes[i]))) - goto no_memory; + if (VIR_STRDUP(ret->machineTypes[i], qemuCaps->machineTypes[i]) < 0) + goto error; if (qemuCaps->machineAliases[i] && - !(ret->machineAliases[i] = strdup(qemuCaps->machineAliases[i]))) - goto no_memory; + VIR_STRDUP(ret->machineAliases[i], qemuCaps->machineAliases[i]) < 0) + goto error;
Can be simplified with NULL source.
@@ -1897,12 +1889,12 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps, if (VIR_ALLOC(mach) < 0) goto no_memory; if (qemuCaps->machineAliases[i]) { - if (!(mach->name = strdup(qemuCaps->machineAliases[i]))) + if (VIR_STRDUP(mach->name, qemuCaps->machineAliases[i]) < 0) goto no_memory;
double-oom, but I'm assuming you'll clean it later.
@@ -2091,16 +2083,11 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps, }
for (i = 0 ; i < nmachines ; i++) { - if (machines[i]->alias) { - if (!(qemuCaps->machineAliases[i] = strdup(machines[i]->alias))) { - virReportOOMError(); - goto cleanup; - } - } - if (!(qemuCaps->machineTypes[i] = strdup(machines[i]->name))) { - virReportOOMError(); + if (machines[i]->alias && + VIR_STRDUP(qemuCaps->machineAliases[i], machines[i]->alias) < 0) + goto cleanup;
Can be simplified.
+++ b/src/qemu/qemu_command.c @@ -2418,13 +2404,11 @@ static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport) if (port) { *port = '\0'; port += skip; - disk->hosts[disk->nhosts-1].port = strdup(port); - if (!disk->hosts[disk->nhosts-1].port) - goto no_memory; + if (VIR_STRDUP(disk->hosts[disk->nhosts-1].port, port) < 0) + goto error; } else { - disk->hosts[disk->nhosts-1].port = strdup("6789"); - if (!disk->hosts[disk->nhosts-1].port) - goto no_memory; + if (VIR_STRDUP(disk->hosts[disk->nhosts-1].port, "6789") < 0)
Pre-existing, but space around '-' while touching this.
@@ -5456,9 +5430,10 @@ qemuBuildCpuArgStr(const virQEMUDriverPtr driver, } virBufferAddLit(&buf, "host"); } else { - if (VIR_ALLOC(guest) < 0 || - (cpu->vendor_id && !(guest->vendor_id = strdup(cpu->vendor_id)))) + if (VIR_ALLOC(guest) < 0) goto no_memory; + if (cpu->vendor_id && VIR_STRDUP(guest->vendor_id, cpu->vendor_id) < 0) + goto cleanup;
Can be simplified.
@@ -8312,17 +8287,16 @@ static int qemuStringToArgvEnv(const char *args, next = strchr(curr, '\n');
if (next) { - arg = strndup(curr, next-curr); + if (VIR_STRNDUP(arg, curr, next-curr) < 0) + goto error;
Space around '-' while touching this.
@@ -9161,14 +9119,12 @@ qemuParseCommandLineChr(virDomainChrSourceDefPtr source, source->type = VIR_DOMAIN_CHR_TYPE_PTY; } else if (STRPREFIX(val, "file:")) { source->type = VIR_DOMAIN_CHR_TYPE_FILE; - source->data.file.path = strdup(val+strlen("file:")); - if (!source->data.file.path) - goto no_memory; + if (VIR_STRDUP(source->data.file.path, val+strlen("file:")) < 0) + goto error; } else if (STRPREFIX(val, "pipe:")) { source->type = VIR_DOMAIN_CHR_TYPE_PIPE; - source->data.file.path = strdup(val+strlen("pipe:")); - if (!source->data.file.path) - goto no_memory; + if (VIR_STRDUP(source->data.file.path, val+strlen("pipe:")) < 0) + goto error;
Space around '+' while touching this
@@ -9179,40 +9135,32 @@ qemuParseCommandLineChr(virDomainChrSourceDefPtr source, host2 = svc1 ? strchr(svc1, '@') : NULL; svc2 = host2 ? strchr(host2, ':') : NULL;
You know, we could use strchrnul() here (guaranteed by gnulib)...
- if (svc1 && (svc1 != val)) { - source->data.udp.connectHost = strndup(val, svc1-val); - - if (!source->data.udp.connectHost) - goto no_memory; - } + if (svc1 && svc1 != val && + VIR_STRNDUP(source->data.udp.connectHost, val, svc1-val) < 0) + goto error;
Space around '-'
if (svc1) { svc1++; - if (host2) - source->data.udp.connectService = strndup(svc1, host2-svc1); - else - source->data.udp.connectService = strdup(svc1);
- if (!source->data.udp.connectService) - goto no_memory; + if ((host2 && VIR_STRNDUP(source->data.udp.connectService, + svc1, host2 - svc1) < 0) || + (!host2 && VIR_STRDUP(source->data.udp.connectService, + svc1) < 0)) + goto error;
...then here, we could simplify things to always be a strndup (host2 would always be non-null, either because it hit ':' or the end of the string). This whole function could be simplified by doing the string parse with a bit more smarts.
@@ -9235,37 +9183,25 @@ qemuParseCommandLineChr(virDomainChrSourceDefPtr source, if (opt && strstr(opt, "server")) source->data.tcp.listen = true;
- source->data.tcp.host = strndup(val, svc-val); - if (!source->data.tcp.host) - goto no_memory; + if (VIR_STRNDUP(source->data.tcp.host, val, svc-val) < 0) + goto error;
Spaces around '-'
svc++; - if (opt) { - source->data.tcp.service = strndup(svc, opt-svc); - } else { - source->data.tcp.service = strdup(svc); - } - if (!source->data.tcp.service) - goto no_memory; + if ((opt && VIR_STRNDUP(source->data.tcp.service, svc, opt - svc) < 0) || + (!opt && VIR_STRDUP(source->data.tcp.service, svc) < 0)) + goto error;
Another place where strchrnul probably helps.
@@ -9318,13 +9254,9 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, next++;
if (p == val) { - if (next) - model = strndup(p, next - p - 1); - else - model = strdup(p); - - if (!model) - goto no_memory; + if ((next && VIR_STRNDUP(model, p, next - p -1) < 0) || + (!next && VIR_STRDUP(model, p) < 0)) + goto error;
I'll quit pointing out spots where strchrnul might help - at this point converting to use strchrnul is worth splitting into a separate cleanup patch (whether before or after this patch, I don't know which is easier).
@@ -10189,9 +10104,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
Shoot. I ran out of time for today. What I've seen so far in qemu is looking okay, though.
+++ b/src/remote/remote_driver.c @@ -3733,7 +3733,7 @@ static int remoteAuthMakeCredentials(sasl_interact_t *interact, } if (interact[*ncred].challenge) (*cred)[*ncred].challenge = interact[ninteract].challenge; - (*cred)[*ncred].prompt = interact[ninteract].prompt; + (*cred)[*ncred].prompt = (char *) interact[ninteract].prompt;
This one feels random (oh, it's associated with your proposed changes to libvirt.h, so it belongs to that patch, depending on what we decide there). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org