[libvirt] [PATCH] domain: conf: Better errors on bad os <type> values

[Cole Robinson]

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.
>