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

[Michal Privoznik]

On 23.05.2013 23:10, Eric Blake wrote:
> 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.

Okay, I've separated vircgroup.c into its specific commit and
s/VIR_STRDUP/VIR_STRNDUP/ within it. The whole file seems a bit awkward
to me to say the least. The less I have to touch it the better :)

Michal