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