[libvirt] [PATCH] introducing <source> <name> (for logical storage pools)

[Jim Meyering]

Daniel Veillard <veillard at redhat.com> wrote:

> On Tue, Sep 02, 2008 at 11:34:46AM -0400, David Lively wrote:
>> Thanks Daniel.  I just merged in your changes.  You seem to be missing a
>> small incremental change (checking the strdup return value for NULL),
>> attached.
>
>   yes, mea-culpa ! That what happens when looking a too old mail,
> but i wanted to be sure your patches would not get lost so i kept them
> as 'unread'  in my mailer, problem is that i didn't realized they were
> deprecated. Jim pointed that missing piece,
>
>> diff --git a/src/storage_conf.c b/src/storage_conf.c
>> index 2f6093b..37a2040 100644
>> --- a/src/storage_conf.c
>> +++ b/src/storage_conf.c
>> @@ -331,6 +331,8 @@ virStoragePoolDefParseDoc(virConnectPtr conn,
>>          if (ret->source.name == NULL) {
>>              /* source name defaults to pool name */
>>              ret->source.name = strdup(ret->name);
>> +            if (ret->source.name == NULL)
>> +                virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("pool name"));
>>          }
>>      }
>>
>
>   Hum, I'm just wondering, shouldn't we go to cleanup too on strdup
>   error instead of continuing there ?

You're probably right.
However, technically, it looks like having a NULL source.name there
is tolerable, since all derefs (at least in that file) first check
for non-NULL.  But if a small strdup like that fails, I don't see much
point in trying to continue.

If that's the intent, then it deserves a comment explaining why this
failure case is different from most(all?) of the others in the vicinity.