[libvirt] [PATCH] virCapabilitiesDomainDataLookup: Produce saner error message

[Michal Privoznik]

On 15.06.2015 09:10, Erik Skultety wrote:
> 
> 
> On 06/12/2015 03:49 PM, Michal Privoznik wrote:
>> During a review, I've noticed this error message that was eventually
>> produced when I was trying to define a domain:
>>
>> error: invalid argument: could not find capabilities for arch=mips64el
>> domaintype=(null)
>>
>> Look at the (null). Why is it there? Well, during XML parsing, we try
>> to look up the default emulator for given OS type and possibly virt
>> type too. And this is the problem, because if we don't want to look up
>> by virt type, a -1 is passed to note this fact. Later, the code
>> handles -1 just right. Except for error message. When it is
>> constructed (in a very fabulous way I must say), the value is compared
>> to zero, not -1. And since we don't have any translation from -1 to a
>> virt type string, we just print (null).
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/conf/capabilities.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
>> index 6decde8..9c2c6b4 100644
>> --- a/src/conf/capabilities.c
>> +++ b/src/conf/capabilities.c
>> @@ -678,7 +678,7 @@ virCapabilitiesDomainDataLookupInternal(virCapsPtr caps,
>>                                virDomainOSTypeToString(ostype));
>>          if (arch)
>>              virBufferAsprintf(&buf, "arch=%s ", virArchToString(arch));
>> -        if (domaintype)
>> +        if (domaintype != -1)
> Well, this will work. However for some reason I don't quite like this
> way of handling it. We're trying to construct an error message and
> because we didn't want to search by virttype we special-case this. I was
> thinking more about introducing a new enum VIR_DOMAIN_VIRT_NONE which
> will serve exactly this purpose (see virCapsDomainDataCompare,
> comparison to arch :)). That way, we don't have to play with ternary in
> here as pass VIRT_NONE right away:
> ...if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type,
>                 def->os.arch, use_virttype ? def->virtType : -1,
>                 NULL, NULL)))...
> Since XML parsing requires a domain to have a valid virttype defined, we
> should be able to abuse the VIRT_NONE this way and stay a little
> consistent in this particular code snippet of constructing an error
> message. What do you think about it? If you think it's not worth it at
> all, just drop the idea and push this one, it's an ACK.

I think it is worth it. However, it would require slightly bigger change
since the rest of lookup functions expects -1 as an alias for 'not
specified'. However, I've started new wiki page (after example given by
qemu):

  http://wiki.libvirt.org/page/BiteSizedTasks

and put your idea there. Feel free to reword and adjust it. Maybe I
should send out more explicit e-mail to let others know about the page
too. Anyway, I've pushed this as-is. Thanks!

Michal