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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

> On 24 Jun 2015, at 16:26, Ian Jackson <ian.jackson@xxxxxxxxxxxxx> wrote:
> 
> Rob Hoes writes ("[PATCH RFC 8/9] libxl: introduce specific error codes in 
> libxl_device_cdrom_insert"):
>> Signed-off-by: Rob Hoes <rob.hoes@xxxxxxxxxx>
> 
>>     if (libxl_get_stubdom_id(ctx, domid) != 0) {
>>         LOG(ERROR, "cdrom-insert doesn't work for stub domains");
>> -        rc = ERROR_INVAL;
>> +        rc = ERROR_STUBDOM;
> 
> ERROR_NOTIMPLEMENTED_STUBDOM ?
> 
>>         goto out;
>>     }
>> 
>>     dm_ver = libxl__device_model_version_running(gc, domid);
>>     if (dm_ver == -1) {
>>         LOG(ERROR, "cannot determine device model version");
>> -        rc = ERROR_FAIL;
>> +        rc = ERROR_DM_VERSION_UNDETERMINED;
> 
> Is this an internal problem ?

In general, by ERROR_<something>_UNDETERMINED, I meant that the user did not 
specify <something> and expected libxl to figure it out or fill in a sensible 
default (e.g. based on some other params that _were_ specified, or some other 
state in the system), but libxl was unable to do so.

This as opposed to ERROR_INVAL_<parameter>, where there is something wrong with 
the <parameter> that was given to libxl by the caller.

Perhaps, for consistency, I should have made them 
ERROR_UNDETERMINED_<something> and ERROR_INVAL_<parameter> 
(ERROR_<condition>_<thing>), or the other way around: 
ERROR_<something>_UNDETERMINED and ERROR_<parameter>_INVAL 
(ERROR_<thing>_<condition>).

In this case, it probably means that QEMU was either not running (seems 
unlikely), an unknown version of it was running, or some state got messed up 
such that QEMU became unrecognisable.

> 
> 
>>     if (i == num) {
>>         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual device not found");
>> -        rc = ERROR_FAIL;
>> +        rc = ERROR_DISK_VDEV_NOT_FOUND;
> 
> ERROR_VDEV_NOTFOUND ?  Could be used for block-detach, nic-detach.

Though being more specific would do no harm?

> 
>> @@ -2941,7 +2941,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, 
>> libxl_device_disk *disk,
>>         {
>>             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Internal error: %s does not 
>> exist",
>>                        libxl__sprintf(gc, "%s/frontend", path));
>> -            rc = ERROR_FAIL;
>> +            rc = ERROR_INTERNAL;
> 
> Right, this is "libxl systemwide state seems corrupted".
> 
> ERROR_INTERNAL_BADSTATE_LIBXL ?
> 
>> @@ -299,7 +299,7 @@ int libxl__device_disk_set_backend(libxl__gc *gc, 
>> libxl_device_disk *disk) {
>>     }
>>     if (!ok) {
>>         LOG(ERROR, "no suitable backend for disk %s", disk->vdev);
>> -        return ERROR_INVAL;
>> +        return ERROR_DISK_BACKEND_UNDETERMINED;
> 
> ERROR_INVALID_DISKâ ?

This follows the reasoning I gave above: the user did not specify the disk 
backend, and libxl was unable to fill in a suitable default. This is different 
from ERROR_INVAL_DISK_BACKEND.

> 
>> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
>> index 9aa7e2e..c687e86 100644
>> --- a/tools/libxl/libxl_qmp.c
>> +++ b/tools/libxl/libxl_qmp.c
>> @@ -817,9 +817,11 @@ static int qmp_run_command(libxl__gc *gc, int domid,
>> 
>>     qmp = libxl__qmp_initialize(gc, domid);
>>     if (!qmp)
>> -        return ERROR_FAIL;
>> +        return ERROR_QMP_INIT;
> 
> This is a bit like mutex initialisation, isn't it ?  It might mean the
> process is out of fds.  Perhaps it would be best to have a function
> which converts an errno value.
> 
> 
>>     rc = qmp_synchronous_send(qmp, cmd, args, callback, opaque, 
>> qmp->timeout);
>> +    if (rc < 0)
>> +        rc = ERROR_QMP_SEND;
> 
> We need to distinguish various this probably by errno value, I think.
> 
> At least some of the results are going to be
> 
> ERROR_DEVICEMODEL_{CRASHED,MAD,HUNG}
> 
> In general it is not sufficient to know "what operation was libxl
> trying to do" and more interesting to know "what might cause this
> errorâ.

Yes, I agree. Itâs similar to the xenstore case.

> 
>> +    # Internal error; not actionable by the caller other than by doing 
>> something like
>> +    # a retry/reboot (perhaps a libxl bug)
>> +    (ENUM_PREV, "INTERNAL"),
> 
> The message is good.  But please wrap everything to 70 columns.  (That
> goes for your 0/N and commit messages too.)
> 
> 
> Ian.

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