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