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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list