5:01 p.m.
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