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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list