[libvirt] [PATCH] conf: Report sensible error for invalid disk name

[Martin Kletzander]

On 11/20/2012 03:45 PM, Daniel P. Berrange wrote:
> On Tue, Nov 20, 2012 at 02:55:28PM +0100, Martin Kletzander wrote:
>> The error "... but the cause is unknown" appeared for XMLs similar to
>> this:
>>
>>  <disk type='file' device='cdrom'>
>>    <driver name='qemu' type='raw'/>
>>    <source file='/dev/zero'/>
>>    <target dev='sr0'/>
>>  </disk>
>>
>> Notice unsupported disk type (for the driver), but also no address
>> specified. The first part is not a problem and we should not abort
>> immediately because of that, but the combination with the address
>> unknown was causing an unspecified error.
>> ---
>>  src/util/util.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/util/util.c b/src/util/util.c
>> index 75b18c1..d5b2c97 100644
>> --- a/src/util/util.c
>> +++ b/src/util/util.c
>> @@ -2189,8 +2189,12 @@ int virDiskNameToIndex(const char *name) {
>>          }
>>      }
>>
>> -    if (!ptr)
>> +    if (!ptr) {
>> +        virReportError(VIR_ERR_XML_ERROR,
>> +                       _("Unknown disk name '%s' and no address specified"),
>> +                       name);
>>          return -1;
>> +    }
>>
>>      for (i = 0; *ptr; i++) {
>>          idx = (idx + (i < 1 ? 0 : 1)) * 26;
> 
> IMHO, you should really be adding an ATTRIBUTE_NONNULL annotation and
> then fixing the callers not to invoke it if the disk name they have
> is Null.
> 

The pointer is not the function attribute, but ...

> Adding virReportError here is wrong, because a number of callers will
> already report errors.
> 

... you're right as Peter, v2 is on it's way.

Martin