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

[Martin Kletzander]

On 11/20/2012 03:27 PM, Peter Krempa wrote:
> On 11/20/12 14:55, 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;
>> +    }
> 
> Some of the call paths of this function the callers do their own error
> reporting (see src/qemu/qemu_command.c) based on the return value. This
> should be consolidated.
> 
> In case of this function I'd probably rather go for adding the error
> message on the appropriate places as the inability to parse the disk ID
> is ignored on some calls (maybe that should be fixed in the first place
> and than the error reporting is okay here.)
> 

You're right, I went through the code just looking for the "un-errored"
negative return and haven't thought about that deeply enough.  I figured
when the error gets set on two places, it'll be simply shadowed.

Sending a v2 in a minute.

Martin