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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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 ?


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

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

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

> +    # 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