[libvirt] [PATCH v3 03/34] Adapt to VIR_STRDUP and VIR_STRNDUP in src/conf/*

[Michal Privoznik]

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