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

Re: [libvirt] [PATCH v4 05/13] Adapt to VIR_STRDUP and VIR_STRNDUP in src/qemu/*



On 05/20/2013 11:55 AM, Michal Privoznik wrote:
> ---
>  src/qemu/qemu_capabilities.c |  79 +++-----
>  src/qemu/qemu_cgroup.c       |   4 +-
>  src/qemu/qemu_command.c      | 428 +++++++++++++++++--------------------------
>  src/qemu/qemu_conf.c         |  64 +++----
>  src/qemu/qemu_domain.c       |  26 +--
>  src/qemu/qemu_driver.c       | 129 ++++---------
>  src/qemu/qemu_hotplug.c      |  15 +-
>  src/qemu/qemu_migration.c    |  17 +-
>  src/qemu/qemu_monitor_json.c |  63 ++-----
>  src/qemu/qemu_monitor_text.c |  15 +-
>  src/qemu/qemu_process.c      |  64 +++----
>  11 files changed, 333 insertions(+), 571 deletions(-)
> 

> @@ -1912,12 +1899,11 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps,
>          if (VIR_ALLOC(mach) < 0)
>              goto no_memory;
>          if (qemuCaps->machineAliases[i]) {
> -            if (!(mach->name = strdup(qemuCaps->machineAliases[i])))
> -                goto no_memory;
> -            if (!(mach->canonical = strdup(qemuCaps->machineTypes[i])))
> +            if (VIR_STRDUP(mach->name, qemuCaps->machineAliases[i]) < 0 ||
> +                VIR_STRDUP(mach->canonical, qemuCaps->machineTypes[i]) < 0)
>                  goto no_memory;

silent->noisy - probably a good change, but may be worth
s/no_memory/error/ in the label so I don't assume it's a double-oom.

> +++ b/src/qemu/qemu_command.c
> @@ -8549,17 +8524,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;
>              if (*next == '\'' ||
>                  *next == '"')
>                  next++;
>          } else {
> -            arg = strdup(curr);
> +            if (VIR_STRDUP(arg, curr) < 0)
> +                goto error;
>          }

Perhaps worth shortening to:

if (VIR_STRNDUP(arg, curr, next ? next - curr : strlen(curr)) < 0)
    goto error;

but I'll leave that as your call.

> @@ -9416,40 +9372,32 @@ qemuParseCommandLineChr(virDomainChrSourceDefPtr source,
>          host2 = svc1 ? strchr(svc1, '@') : NULL;
>          svc2 = host2 ? strchr(host2, ':') : NULL;
>  
> -        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;
>  
>          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;

Looks simpler as:

if (VIR_STRNDUP(source->dta.udp.connectService, sv1,
                host2 ? host2 - svc1 : strlen(svc1)) < 0)
    goto error;

Hmm, this makes me wonder if it is worth making VIR_STRNDUP slightly
smarter, where if the length argument is -1, then it uses strlen
internally, so we could get away with clients written as:

if (VIR_STRNDUP(source->dta.udp.connectService, sv1,
                host2 ? host2 - svc1 : -1) < 0)

After all, we've done magic like that for virBufferAdd (pass -1 instead
of strlen, and virBuffer does the right thing on your behalf).  But if
we DO decide to add the magic, it should be in a standalone patch and
with a testsuite addition.

> @@ -9472,37 +9420,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;
>          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))

Again, looks nicer as:

if (VIR_STRNDUP(source->data.tcp.service, svc,
                opt ? opt - svc : strlen(svc)) < 0)

[okay, so maybe the magic -1 really is nicer than making clients write
strlen]

> +            goto error;
>      } else if (STRPREFIX(val, "unix:")) {
>          const char *opt;
>          val += strlen("unix:");
>          opt = strchr(val, ',');
>          source->type = VIR_DOMAIN_CHR_TYPE_UNIX;
> -        if (opt) {
> -            if (strstr(opt, "listen"))
> -                source->data.nix.listen = true;
> -            source->data.nix.path = strndup(val, opt-val);
> -        } else {
> -            source->data.nix.path = strdup(val);
> -        }
> -        if (!source->data.nix.path)
> -            goto no_memory;
> +        if ((opt && VIR_STRNDUP(source->data.nix.path, val, opt - val) < 0) ||
> +            (!opt && VIR_STRDUP(source->data.nix.path, val) < 0))

and another site

> @@ -9555,13 +9491,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))

and another site

> @@ -9584,13 +9516,9 @@ qemuParseCommandLineCPU(virDomainDefPtr dom,
>              if (*p == '\0' || *p == ',')
>                  goto syntax;
>  
> -            if (next)
> -                feature = strndup(p, next - p - 1);
> -            else
> -                feature = strdup(p);
> -
> -            if (!feature)
> -                goto no_memory;
> +            if ((next && VIR_STRNDUP(feature, p, next - p - 1) < 0) ||
> +                (!next && VIR_STRDUP(feature, p) < 0))

and another

> @@ -9648,13 +9576,9 @@ qemuParseCommandLineCPU(virDomainDefPtr dom,
>              if (*p == '\0' || *p == ',')
>                  goto syntax;
>  
> -            if (next)
> -                feature = strndup(p, next - p - 1);
> -            else
> -                feature = strdup(p);
> -
> -            if (!feature)
> -                goto no_memory;
> +            if ((next && VIR_STRNDUP(feature, p, next - p - 1) < 0) ||
> +                (!next && VIR_STRDUP(feature, p) < 0))
> +                goto error;

and another.

> @@ -10843,7 +10751,7 @@ static int qemuParseProcFileStrings(int pid_value,
>      str[nstr-1] = NULL;
>  
>      ret = nstr-1;
> -    *list = str;
> +    *list = (const char **) str;

Bummer that C's type system makes us add casts, but this one is correct
- nothing you can do about it.

> +++ b/src/qemu/qemu_conf.c
> @@ -1237,8 +1233,8 @@ qemuAddSharedDevice(virQEMUDriverPtr driver,
>          if (!(new_entry = qemuSharedDeviceEntryCopy(entry)))
>              goto cleanup;
>  
> -        if ((VIR_EXPAND_N(new_entry->domains, new_entry->ref, 1) < 0) ||
> -            !(new_entry->domains[new_entry->ref - 1] = strdup(name))) {
> +        if (VIR_EXPAND_N(new_entry->domains, new_entry->ref, 1) < 0 ||
> +            VIR_STRDUP(new_entry->domains[new_entry->ref - 1], name) < 0) {
>              qemuSharedDeviceEntryFree(new_entry, NULL);
>              virReportOOMError();

double-oom, but that's okay since a later pass over VIR_EXPAND_N will
also touch this code.

> @@ -1249,9 +1245,9 @@ qemuAddSharedDevice(virQEMUDriverPtr driver,
>              goto cleanup;
>          }
>      } else {
> -        if ((VIR_ALLOC(entry) < 0) ||
> -            (VIR_ALLOC_N(entry->domains, 1) < 0) ||
> -            !(entry->domains[0] = strdup(name))) {
> +        if (VIR_ALLOC(entry) < 0 ||
> +            VIR_ALLOC_N(entry->domains, 1) < 0 ||
> +            VIR_STRDUP(entry->domains[0], name) < 0) {
>              qemuSharedDeviceEntryFree(entry, NULL);
>              virReportOOMError();

another double-oom, also okay.

> +++ b/src/qemu/qemu_driver.c
> @@ -11250,9 +11223,8 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
>      }
>  
>      if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0 ||
> -        !(source = strdup(snap->file)) ||
> -        (persistDisk &&
> -         !(persistSource = strdup(source)))) {
> +        VIR_STRDUP(source, snap->file) < 0 ||
> +        (persistDisk && VIR_STRDUP(persistSource, source) < 0)) {
>          virReportOOMError();

double-oom, but okay because of a later cleanup pass on virAsprintf

> +++ b/src/qemu/qemu_process.c
> @@ -2666,12 +2660,12 @@ qemuProcessUpdateState(virQEMUDriverPtr driver, virDomainObjPtr vm)
>      if (state == VIR_DOMAIN_PAUSED && running) {
>          newState = VIR_DOMAIN_RUNNING;
>          newReason = VIR_DOMAIN_RUNNING_UNPAUSED;
> -        msg = strdup("was unpaused");
> +        ignore_value(VIR_STRDUP(msg, "was unpaused"));

Since we only use msg in a VIR_DEBUG, this should use VIR_STRDUP_QUIET
and update the message to use NULLSTR(msg) (ie. no need to report an OOM
back to the user just because we failed to print a debug message).

>      } else if (state == VIR_DOMAIN_RUNNING && !running) {
>          if (reason == VIR_DOMAIN_PAUSED_SHUTTING_DOWN) {
>              newState = VIR_DOMAIN_SHUTDOWN;
>              newReason = VIR_DOMAIN_SHUTDOWN_UNKNOWN;
> -            msg = strdup("shutdown");
> +            ignore_value(VIR_STRDUP(msg, "shutdown"));

same story here.

>          } else {
>              newState = VIR_DOMAIN_PAUSED;
>              newReason = reason;
> @@ -2681,7 +2675,7 @@ qemuProcessUpdateState(virQEMUDriverPtr driver, virDomainObjPtr vm)
>      } else if (state == VIR_DOMAIN_SHUTOFF && running) {
>          newState = VIR_DOMAIN_RUNNING;
>          newReason = VIR_DOMAIN_RUNNING_BOOTED;
> -        msg = strdup("finished booting");
> +        ignore_value(VIR_STRDUP(msg, "finished booting"));

and here.  The debug message in question, a few lines later, is:

        VIR_DEBUG("Domain %s %s while its monitor was disconnected;"
                  " changing state to %s (%s)",
                  vm->def->name,
                  msg,
                  virDomainStateTypeToString(newState),
                  virDomainStateReasonToString(newState, newReason));


I pointed out a few ideas for improvement, but didn't spot any serious
flaws.  ACK with those tweaks.

-- 
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]