[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 17/34] Adapt to VIR_STRDUP and VIR_STRNDUP in src/qemu/*



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]