Re: [libvirt] [PATCH v4 09/13] Adapt to VIR_STRDUP and VIR_STRNDUP in src/util/*

On 05/20/2013 11:55 AM, Michal Privoznik wrote: > --- > 34 files changed, 357 insertions(+), 570 deletions(-) I've got my work cut out for me! > @@ -226,8 +228,11 @@ char *virBitmapFormat(virBitmapPtr bitmap) > return NULL; > > cur = virBitmapNextSetBit(bitmap, -1); > - if (cur < 0) > - return strdup(""); > + if (cur < 0) { > + char *ret; > + ignore_value(VIR_STRDUP(ret, "")); > + return ret; Hmm, I've seen this three-line pattern (declare temp var, strdup "" into it, use the var) in several patches now. I think it might help to have a new function in virstring.h whose job in life is to return a malloc'd copy of an empty string, as a one-liner, so that callers don't have to mess with a temp var. And notice that it's slightly more efficient to just zero-initialize a malloc'd array of 1, instead of going through strdup machinery, when we know the output will be an empty string. Maybe: /* Return a malloc'd empty string, or NULL after reporting OOM */ char * virStringEmpty(void) { char *ret; // assuming we fix VIR_ALLOC to report oom... ignore_value(VIR_ALLOC(ret)); return ret; } then THIS code could use the shorter: if (cur < 0) return virStringEmpty(); But if you decide to go that route, it's probably worth a separate cleanup pass, so this commit is not delayed. > +++ b/src/util/vircgroup.c > @@ -116,19 +116,13 @@ static int virCgroupCopyMounts(virCgroupPtr group, > if (!parent->controllers[i].mountPoint) > continue; > > - group->controllers[i].mountPoint = > - strdup(parent->controllers[i].mountPoint); > - > - if (!group->controllers[i].mountPoint) > + if (VIR_STRDUP(group->controllers[i].mountPoint, > + parent->controllers[i].mountPoint) < 0) > return -ENOMEM; double-oom, since this function was previously silent and callers already expected to do their own error reporting. This whole file has an unusual paradigm compared to most source files, it may be best to split this file into a separate patch (so that you aren't holding up the rest of src/util/*) and either use VIR_STRDUP_QUIET or start to tackle the bigger issue of tracing through callers to behave better when leaf functions report errors. > > - if (parent->controllers[i].linkPoint) { > - group->controllers[i].linkPoint = > - strdup(parent->controllers[i].linkPoint); > - > - if (!group->controllers[i].linkPoint) > - return -ENOMEM; > - } > + if (VIR_STRDUP(group->controllers[i].linkPoint, > + parent->controllers[i].linkPoint) < 0) > + return -ENOMEM; again, double-oom > @@ -177,7 +171,7 @@ static int virCgroupDetectMounts(virCgroupPtr group) > struct stat sb; > char *tmp2; > > - if (!(group->controllers[i].mountPoint = strdup(entry.mnt_dir))) > + if (VIR_STRDUP(group->controllers[i].mountPoint, entry.mnt_dir) < 0) > goto no_memory; no_memory label is redudant; VIR_STRDUP guarantees that 'errno == ENOMEM' if it returns -1, as do VIR_ALLOC and virAsprintf. (Hmm, maybe we should enhance './configure --enable-test-oom' to specifically test that; although it may be a surprising amount of work to get that to happen). This is a silent->noisy change, and again an instance where the cgroup callers are doing their own error reporting; and the no_memory label is a bit awkward because it is not doing its own OOM reporting. Just deleting the no_memory label, and using 'goto error' will make the code a bit less confusing. And it reiterates my thought that src/util/vircgroup.c is enough of an oddball to warrant being split into its own patch. So with that, I'll quit pointing out silent->noisy changes in this file, and just point out other problems.