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