Re: [Xen-devel] [PATCH RFC 7/9] libxl: introduce specific error codes in libxl_device_disk_add

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

On 24 Jun 2015, at 16:28, Ian Jackson <ian.jackson@xxxxxxxxxxxxx> wrote:
> 
> Rob Hoes writes ("[PATCH RFC 7/9] libxl: introduce specific error codes in 
> libxl_device_disk_add"):
>> Signed-off-by: Rob Hoes <rob.hoes@xxxxxxxxxx>
> ...
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index f622981..2f56c6e 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -2008,9 +2008,16 @@ static int libxl__device_nextid(libxl__gc *gc, 
>> uint32_t domid, char *device)
>> static int libxl__resolve_domid(libxl__gc *gc, const char *name,
>>                                 uint32_t *domid)
>> {
>> +    int rc;
>>     if (!name)
>>         return 0;
>> -    return libxl_domain_qualifier_to_domid(CTX, name, domid);
>> +
>> +    rc = libxl_domain_qualifier_to_domid(CTX, name, domid);
>> +
>> +    if (rc < 0)
>> +        return ERROR_DOMAIN_NOTFOUND;
>> +    else
>> +        return rc;
> 
> This doesn't seem right.  You're smashing all the errors to domain not
> found.  Surely libxl_domain_qualifier_to_domid should return the right
> error to start with.

The idea was to translate a lower-level error into one that makes sense in the 
context of the higher-level function. When talking about this offline, I think 
that we concluded that this may be the right thing to do in certain cases, but 
then we should explicitly translate specific error codes rather than doing it 
by âif (rc < 0)â.

However, in this specific case, I should have probably changed 
libxl_name_to_domid instead, to return ERROR_DOMAIN_NOTFOUND rather than 
ERROR_INVAL.

> 
> The rest looks plausible (modulo my quibbles about names in my earlier
> mails).
> 
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel