On 05/20/2013 11:55 AM, Michal Privoznik wrote:
> ---
> src/conf/capabilities.c | 30 ++++-------
> src/conf/cpu_conf.c | 20 ++++----
> src/conf/domain_conf.c | 119 ++++++++++++--------------------------------
> src/conf/domain_event.c | 41 +++++++--------
> src/conf/node_device_conf.c | 28 +++++------
> src/conf/nwfilter_conf.c | 17 ++-----
> src/conf/nwfilter_params.c | 34 ++++---------
> src/conf/snapshot_conf.c | 11 ++--
> src/conf/storage_conf.c | 13 ++---
> src/conf/virchrdev.c | 12 ++---
> 10 files changed, 107 insertions(+), 218 deletions(-)
>
> @@ -228,7 +228,7 @@ virCapabilitiesAddHostFeature(virCapsPtr caps,
> caps->host.nfeatures, 1) < 0)
> return -1;
>
> - if ((caps->host.features[caps->host.nfeatures] = strdup(name)) == NULL)
> + if (VIR_STRDUP(caps->host.features[caps->host.nfeatures], name) < 0)
> return -1;
silent->noisy, but all callers reported OOM via a no_memory label, and
I'm okay with double-oom if you are still planning on making another
pass to reduce the number of no_memory labels in the code base.
> @@ -250,7 +250,7 @@ virCapabilitiesAddHostMigrateTransport(virCapsPtr caps,
> caps->host.nmigrateTrans, 1) < 0)
> return -1;
>
> - if ((caps->host.migrateTrans[caps->host.nmigrateTrans] = strdup(name)) == NULL)
> + if (VIR_STRDUP(caps->host.migrateTrans[caps->host.nmigrateTrans], name) < 0)
> return -1;
silent->noisy. esx_driver.c and qemu_capabilities aren't checking for
error (they should), xen_hypervisor has double-oom via no_memory label,
and tests/vmx2xmltest.c and tests/xml2vmxtest.c can get by as-is.
> @@ -334,7 +334,7 @@ virCapabilitiesAllocMachines(const char *const *names, int nnames)
>
> for (i = 0; i < nnames; i++) {
> if (VIR_ALLOC(machines[i]) < 0 ||
> - !(machines[i]->name = strdup(names[i]))) {
> + VIR_STRDUP(machines[i]->name, names[i]) < 0) {
> virCapabilitiesFreeMachines(machines, nnames);
> return NULL;
silent->noisy, but all callers (outside tests) used no_memory label and
now have double-oom.
> }
> @@ -392,17 +392,14 @@ virCapabilitiesAddGuest(virCapsPtr caps,
> if (VIR_ALLOC(guest) < 0)
> goto no_memory;
>
> - if ((guest->ostype = strdup(ostype)) == NULL)
> + if (VIR_STRDUP(guest->ostype, ostype) < 0)
> goto no_memory;
Local double-oom. You might want to clean this one up now.
>
> guest->arch.id = arch;
> guest->arch.wordsize = virArchGetWordSize(arch);
>
> - if (emulator &&
> - (guest->arch.defaultInfo.emulator = strdup(emulator)) == NULL)
> - goto no_memory;
> - if (loader &&
> - (guest->arch.defaultInfo.loader = strdup(loader)) == NULL)
> + if (VIR_STRDUP(guest->arch.defaultInfo.emulator, emulator) < 0 ||
> + VIR_STRDUP(guest->arch.defaultInfo.loader, loader) < 0)
> goto no_memory;
another local double-oom. Cleaning now would be nice, but I guess I can
live with the promise of later cleanup of 'no_memory' labels.
> @@ -448,14 +445,9 @@ virCapabilitiesAddGuestDomain(virCapsGuestPtr guest,
> if (VIR_ALLOC(dom) < 0)
> goto no_memory;
>
> - if ((dom->type = strdup(hvtype)) == NULL)
> - goto no_memory;
> -
> - if (emulator &&
> - (dom->info.emulator = strdup(emulator)) == NULL)
> - goto no_memory;
> - if (loader &&
> - (dom->info.loader = strdup(loader)) == NULL)
> + if (VIR_STRDUP(dom->type, hvtype) < 0 ||
> + VIR_STRDUP(dom->info.emulator, emulator) < 0 ||
> + VIR_STRDUP(dom->info.loader, loader) < 0)
> goto no_memory;
More double-oom
>
> if (VIR_RESIZE_N(guest->arch.domains, guest->arch.ndomains_max,
> @@ -497,7 +489,7 @@ virCapabilitiesAddGuestFeature(virCapsGuestPtr guest,
> if (VIR_ALLOC(feature) < 0)
> goto no_memory;
>
> - if ((feature->name = strdup(name)) == NULL)
> + if (VIR_STRDUP(feature->name, name) < 0)
> goto no_memory;
and again
> feature->defaultOn = defaultOn;
> feature->toggle = toggle;
> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
> index 6aaee75..1144cba 100644
> --- a/src/conf/cpu_conf.c
> +++ b/src/conf/cpu_conf.c
> @@ -99,10 +99,10 @@ virCPUDefCopyModel(virCPUDefPtr dst,
> {
> unsigned int i;
>
> - if ((src->model && !(dst->model = strdup(src->model)))
> - || (src->vendor && !(dst->vendor = strdup(src->vendor)))
> - || (src->vendor_id && !(dst->vendor_id = strdup(src->vendor_id)))
> - || VIR_ALLOC_N(dst->features, src->nfeatures) < 0)
> + if (VIR_STRDUP(dst->model, src->model) < 0 ||
> + VIR_STRDUP(dst->vendor, src->vendor) < 0 ||
> + VIR_STRDUP(dst->vendor_id, src->vendor_id) < 0 ||
> + VIR_ALLOC_N(dst->features, src->nfeatures) < 0)
> goto no_memory;
Here, you have double-oom, but you also have a VIR_ALLOC_N. I can
definitely agree to leaving this one as-is, since we are going to audit
VIR_ALLOC_N later and can clean up the double-oom then (the earlier uses
have no VIR_ALLOC_N nearby, but if your goal is to get rid of no_memory
labels in general, you'd still get them on a later pass).
> +++ b/src/conf/domain_conf.c
> @@ -17352,12 +17303,9 @@ virDomainGraphicsListenSetAddress(virDomainGraphicsDefPtr def,
> return 0;
> }
>
> - listenInfo->address = (len == -1) ? strdup(address) : strndup(address, len);
> - if (!listenInfo->address) {
> - virReportOOMError();
> + if ((len == -1 && VIR_STRDUP(listenInfo->address, address) < 0) ||
> + (len != -1 && VIR_STRNDUP(listenInfo->address, address, len) < 0))
Might look nicer as:
if (VIR_STRNDUP(listenInfo->address, address,
len == -1 ? strlen(address) : len) < 0)
> @@ -17394,12 +17342,9 @@ virDomainGraphicsListenSetNetwork(virDomainGraphicsDefPtr def,
> return 0;
> }
>
> - listenInfo->network = (len == -1) ? strdup(network) : strndup(network, len);
> - if (!listenInfo->network) {
> - virReportOOMError();
> + if ((len == -1 && VIR_STRDUP(listenInfo->network, network) < 0) ||
> + (len != -1 && VIR_STRNDUP(listenInfo->network, network, len) < 0))
Another site where calling just VIR_STRNDUP looks nicer.
> @@ -17734,7 +17679,7 @@ virDomainDefGenSecurityLabelDef(const char *model)
> virSecurityLabelDefPtr seclabel = NULL;
>
> if (VIR_ALLOC(seclabel) < 0 ||
> - (model && !(seclabel->model = strdup(model)))) {
> + VIR_STRDUP(seclabel->model, model) < 0) {
> virReportOOMError();
double-oom, but mixed with VIR_ALLOC so okay for now
> +++ b/src/conf/domain_event.c
> @@ -791,9 +791,9 @@ static virDomainEventPtr virDomainEventIOErrorNewFromDomImpl(int event,
>
> if (ev) {
> ev->data.ioError.action = action;
> - if (!(ev->data.ioError.srcPath = strdup(srcPath)) ||
> - !(ev->data.ioError.devAlias = strdup(devAlias)) ||
> - (reason && !(ev->data.ioError.reason = strdup(reason)))) {
> + if (VIR_STRDUP(ev->data.ioError.srcPath, srcPath) < 0 ||
> + VIR_STRDUP(ev->data.ioError.devAlias, devAlias) < 0 ||
> + VIR_STRDUP(ev->data.ioError.reason, reason) < 0) {
> virDomainEventFree(ev);
> ev = NULL;
silent->noisy, but this is a case where we are trying to send an
asynchronous event, and merely discard it on OOM. This change now
leaves an error in the thread-local storage, to be used in the next
unrelated synchronous command that uses the same worker thread. Are we
SURE this is right? That is, if ALL libvirt API clear out the local
error on entry, then checking if the local error is set on exit, then
littering the thread-local error at other times won't change the
behavior of synchronous API. But I think you got lucky: looking at
src/libvirt.c, we are pretty consistent on virResetLastError() at the
head of all API.
> }
> @@ -815,9 +815,9 @@ static virDomainEventPtr virDomainEventIOErrorNewFromObjImpl(int event,
>
> if (ev) {
> ev->data.ioError.action = action;
> - if (!(ev->data.ioError.srcPath = strdup(srcPath)) ||
> - !(ev->data.ioError.devAlias = strdup(devAlias)) ||
> - (reason && !(ev->data.ioError.reason = strdup(reason)))) {
> + if (VIR_STRDUP(ev->data.ioError.srcPath, srcPath) < 0 ||
> + VIR_STRDUP(ev->data.ioError.devAlias, devAlias) < 0 ||
> + VIR_STRDUP(ev->data.ioError.reason, reason) < 0) {
> virDomainEventFree(ev);
> ev = NULL;
Same comment on silent-noisy now polluting thread-local storage.
> }
> @@ -882,7 +882,7 @@ virDomainEventPtr virDomainEventGraphicsNewFromDom(virDomainPtr dom,
>
> if (ev) {
> ev->data.graphics.phase = phase;
> - if (!(ev->data.graphics.authScheme = strdup(authScheme))) {
> + if (VIR_STRDUP(ev->data.graphics.authScheme, authScheme) < 0) {
> virDomainEventFree(ev);
> return NULL;
and again
> }
> @@ -907,7 +907,7 @@ virDomainEventPtr virDomainEventGraphicsNewFromObj(virDomainObjPtr obj,
>
> if (ev) {
> ev->data.graphics.phase = phase;
> - if (!(ev->data.graphics.authScheme = strdup(authScheme))) {
> + if (VIR_STRDUP(ev->data.graphics.authScheme, authScheme) < 0) {
> virDomainEventFree(ev);
> return NULL;
and again
> }
> @@ -928,8 +928,7 @@ virDomainEventBlockJobNew(int id, const char *name, unsigned char *uuid,
> id, name, uuid);
>
> if (ev) {
> - if (!(ev->data.blockJob.path = strdup(path))) {
> - virReportOOMError();
> + if (VIR_STRDUP(ev->data.blockJob.path, path) < 0) {
> virDomainEventFree(ev);
> return NULL;
but THIS virDomainEvent*New was already reporting. Okay, you win - we
SHOULD be reporting everywhere, even if we end up discarding the error
object instead of sending it across rpc because we hit oom.
A couple of suggestions, but no major errors, and I know you are
planning on doing some of the double-oom cleanup in a later patch.
ACK.
--
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