[libvirt] [PATCH] domain: conf: Better errors on bad os <type> values
If no <os><type> was specified: before: unknown OS type no OS type after : xml error: an os <type> must be specified If an <os><type> is specified that's not in our capabiliities data: before: unknown OS type $type after : unsupported configuration: no support found for os <type> '$type' VIR_ERR_OS_TYPE is now unused (as it should be frankly) so drop its strings as well to save our translators some effort. --- src/conf/domain_conf.c | 9 +++++---- src/util/virerror.c | 5 +---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4d7e3c9..a145e11 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14638,8 +14638,8 @@ virDomainDefParseXML(xmlDocPtr xml, if (VIR_STRDUP(def->os.type, "xen") < 0) goto error; } else { - virReportError(VIR_ERR_OS_TYPE, - "%s", _("no OS type")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("an os <type> must be specified")); goto error; } } @@ -14656,8 +14656,9 @@ virDomainDefParseXML(xmlDocPtr xml, } if (!virCapabilitiesSupportsGuestOSType(caps, def->os.type)) { - virReportError(VIR_ERR_OS_TYPE, - "%s", def->os.type); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("no support found for os <type> '%s'"), + def->os.type); goto error; } diff --git a/src/util/virerror.c b/src/util/virerror.c index 73dae95..aab36ae 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -900,10 +900,7 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("failed Xen syscall %s"); break; case VIR_ERR_OS_TYPE: - if (info == NULL) - errmsg = _("unknown OS type"); - else - errmsg = _("unknown OS type %s"); + errmsg = "%s"; break; case VIR_ERR_NO_KERNEL: errmsg = _("missing kernel information"); -- 2.3.5
On 04/17/2015 07:06 AM, Cole Robinson wrote:
If no <os><type> was specified: before: unknown OS type no OS type after : xml error: an os <type> must be specified
If an <os><type> is specified that's not in our capabiliities data:
s/capabiliities/capabilities/
before: unknown OS type $type after : unsupported configuration: no support found for os <type> '$type'
VIR_ERR_OS_TYPE is now unused (as it should be frankly) so drop its strings as well to save our translators some effort.
NACK to that part - even if a newer libvirtd never sends the error, newer clients can still connect to older libvirtd and the new client must still be prepared to receive the error from the older server. We are stuck carrying the translation, even if we no longer generate it.
--- src/conf/domain_conf.c | 9 +++++---- src/util/virerror.c | 5 +---- 2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4d7e3c9..a145e11 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14638,8 +14638,8 @@ virDomainDefParseXML(xmlDocPtr xml, if (VIR_STRDUP(def->os.type, "xen") < 0) goto error; } else { - virReportError(VIR_ERR_OS_TYPE, - "%s", _("no OS type")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("an os <type> must be specified")); goto error; } } @@ -14656,8 +14656,9 @@ virDomainDefParseXML(xmlDocPtr xml, }
if (!virCapabilitiesSupportsGuestOSType(caps, def->os.type)) { - virReportError(VIR_ERR_OS_TYPE, - "%s", def->os.type); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("no support found for os <type> '%s'"), + def->os.type); goto error; }
ACK to these two hunks. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/17/2015 09:10 AM, Eric Blake wrote:
On 04/17/2015 07:06 AM, Cole Robinson wrote:
If no <os><type> was specified: before: unknown OS type no OS type after : xml error: an os <type> must be specified
If an <os><type> is specified that's not in our capabiliities data:
s/capabiliities/capabilities/
before: unknown OS type $type after : unsupported configuration: no support found for os <type> '$type'
VIR_ERR_OS_TYPE is now unused (as it should be frankly) so drop its strings as well to save our translators some effort.
NACK to that part - even if a newer libvirtd never sends the error, newer clients can still connect to older libvirtd and the new client must still be prepared to receive the error from the older server. We are stuck carrying the translation, even if we no longer generate it.
Thanks for the review. But AFAICT messages are always built server side, that is virErrorPtr->message always contains 'error-code-message + site-message'. So dropping these strings should be fine. We _are_ stuck with the error code though, that's public API Verified this by using f22 libvirtd (without this patch), and ./tools/virsh (with this patch), to define a VM with bogus type 'frob', and received the f22 'unknown OS type frob' - Cole
--- src/conf/domain_conf.c | 9 +++++---- src/util/virerror.c | 5 +---- 2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4d7e3c9..a145e11 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14638,8 +14638,8 @@ virDomainDefParseXML(xmlDocPtr xml, if (VIR_STRDUP(def->os.type, "xen") < 0) goto error; } else { - virReportError(VIR_ERR_OS_TYPE, - "%s", _("no OS type")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("an os <type> must be specified")); goto error; } } @@ -14656,8 +14656,9 @@ virDomainDefParseXML(xmlDocPtr xml, }
if (!virCapabilitiesSupportsGuestOSType(caps, def->os.type)) { - virReportError(VIR_ERR_OS_TYPE, - "%s", def->os.type); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("no support found for os <type> '%s'"), + def->os.type); goto error; }
ACK to these two hunks.
participants (2)
-
Cole Robinson -
Eric Blake