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

[Cole Robinson]

On 04/20/2015 09:53 AM, Michal Privoznik wrote:
> On 18.04.2015 03:45, 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:
>>   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 ab4f2bf..8458f5b 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";
> 
> Even though there are no other calls with VIR_ERR_OS_TYPE, I'd feel more
> safe with:
> 
> if (info == NULL)
>   errmsg = _("invalid or missing OS type");
> else
>   errmsg = "%s";
> 
> I find it more future proof.

Thanks for the review. I just the dropped virerror.c diff, rather than add a
new string that needs translation yet isn't used anywhere.

Thanks,
Cole