[libvirt] [PATCH] virCapabilitiesDomainDataLookup: Produce saner error message
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@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) virBufferAsprintf(&buf, "domaintype=%s ", virDomainVirtTypeToString(domaintype)); if (emulator) -- 2.3.6
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@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
On 06/12/2015 03:49 PM, Michal Privoznik wrote: 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.
virBufferAsprintf(&buf, "domaintype=%s ", virDomainVirtTypeToString(domaintype)); if (emulator)
Erik
On 15.06.2015 09:10, Erik Skultety 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@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
On 06/12/2015 03:49 PM, Michal Privoznik wrote: 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
participants (2)
-
Erik Skultety -
Michal Privoznik