On 05/03/2013 08:53 AM, Michal Privoznik wrote:
> ---
> src/conf/capabilities.c | 30 +++++--------
> src/conf/cpu_conf.c | 19 ++++----
> src/conf/domain_conf.c | 105 ++++++++++++++------------------------------
> src/conf/domain_event.c | 39 ++++++++--------
> src/conf/node_device_conf.c | 29 ++++++------
> src/conf/nwfilter_conf.c | 17 ++-----
> src/conf/nwfilter_params.c | 30 ++++---------
> src/conf/snapshot_conf.c | 11 ++---
> src/conf/storage_conf.c | 13 ++----
> src/conf/virchrdev.c | 12 ++---
> 10 files changed, 107 insertions(+), 198 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;
This function was previously silent on error; which means its callers
are now reporting an error where they used to be silent (probably GOOD
in most cases) or reporting doubled errors (annoying, and worth fixing
now while we are looking at it). I'd feel more comfortable if we:
1. fixed this function to report errors on ALL negative exit paths
(although for this function, the only other error path is via
VIR_ALLOC_N, and I already said I would tolerate delaying that fix that
to a later pass, so that we aren't adding churn with virReportOOMError()
in this patch just to pull it back out later)
2. check all callers as part of this patch to make sure they:
a. are okay with the error being reported
b. quit reporting double OOM on failure
For this function, all callers in libxl_conf.c, test_driver.c,
xen_hypervisor.c were reporting OOM, and need touchup.
It's going to be easier to audit that we are avoiding double-error
reports if we fix all call sites any time we change function semantics
now, compared to blindly converting to VIR_STRDUP now then trying to
audit who changed later.
> @@ -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;
Same comments about changed semantics. I've done the audit; this time:
esx_driver.c [hmm, that one wasn't even checking for errors, but we are
okay letting it report errors, so no change needed], qemu_capabilities.c
[another case of going from silent to error report being good],
xen_hypervisor.c, vmx2xmltest.c [tests are less important to fix; it
changed from silent to reporting, which is okay], xml2vmxtest.c [another
test].
So, of the list of five files with callers, I'd expect at least
xen_hypervisor.c to be patched.
> @@ -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;
Another case of changing from silent to error-reporting. Callers:
libxl_conf.c, xen_hypervisor.c, testutilsqemu.c [testsuite file, so less
important; was previously silent but new reporting behavior is okay],
testutilsxen.c [another test file]
Looks like libxl_conf.c and xen_hypervisor.c are the two files needing a
patch to avoid 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;
The label is now incorrectly named. And looking at that label:
no_memory:
virCapabilitiesFreeGuest(guest);
return NULL;
this is another case of changing from silent to error-reporting. Of the
callers, I determined that at least: esx_driver.c and lxc_conf.c switch
from silent to reporting (good, no change needed), and at least
openvz_conf.c and phyp_driver.c now have double-OOM (fix them); I
stopped auditing callers here.
>
> 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 ((emulator && VIR_STRDUP(guest->arch.defaultInfo.emulator, emulator) < 0) ||
> + (loader && VIR_STRDUP(guest->arch.defaultInfo.loader, loader) < 0))
> goto no_memory;
Ah, we really DO have a case where my proposal in 1/34 of allowing a
NULL source would make sense.
Again, no_memory is now labeled incorrectly
> @@ -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 ||
> + (emulator && VIR_STRDUP(dom->info.emulator, emulator) < 0) ||
> + (loader && VIR_STRDUP(dom->info.loader, loader) < 0))
> goto no_memory;
Label name is now wrong; and looking at the label, it is another case of
silent->reporting conversion; all callers need audit.
> @@ -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;
wrong label name, another silent->reporting caller audit.
Hmm, it might mean splitting this patch into smaller pieces, focusing on
one file plus all its fallout, rather than doing one directory but not
tracking any fallout. As such, I'm sending my email now in case it
helps you start v4 of a split on just capabilities.c, while I review the
rest of this patch in another email.
--
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