Re: [libvirt] [PATCH v3 03/34] Adapt to VIR_STRDUP and VIR_STRNDUP in src/conf/*
On 03.05.2013 23:22, Eric Blake wrote:
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.
I don't quite agree. In the next serie I will be:
1) turning VIR_ALLOC* and virAsprintf into OOM reporting
2) removing virReportOOMError from everywhere
These two steps will effectively cut off double OOM error reporting and
move the place where error is reported closer to it's actual origin.
So in this concrete example, callers of virCapabilitiesAddHostFeature are:
libxl_conf.c:80: libxlBuildCapabilities: does call virReportOOMError (in
fact, its parent does)
src/test/test_driver.c:175: testBuildCapabilities: does call
virReportOOMError
xen_hypervisor.c:2293: xenHypervisorBuildCapabilities does call
virReportOOMError (in fact, its parent does)
After my patches, these wouldn't call virReportOOMError at all. I agree,
that the future patches I am talking about will require more inspection
if callee reports OOM error on every path, but I would not bother with
it for now. I mean, this chunk of code we have here will report OOM
error at no cost - compared to your suggestion where I'd have to
manually go through every possible callers. Twice.
Moreover, I don't think moving from silent -> reporting is no bad at
all. The worst case scenario is, users will see one more line in the
libvirtd log. The best case scenario is - all forgotten places where we
currently don't catch OOM errors, or do it in bad way (remember
make_nonnull_domain?) will now report the correct error. Again, at no price.
What I really fear about here is, that I don't change semantics. But
that's what review is for, isn't it :)
Michal