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