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

23 May 2013


      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

    