Re: [Qemu-devel] [PATCH] block: clarify error message for qmp-eject

[Fam Zheng]

On Tue, 05/17 20:42, John Snow wrote:
> If you use HMP's eject but the CDROM tray is locked, you may get a
> confusing error message informing you that the "tray isn't open."
> 
> As this is the point of eject, we can do a little better and help
> clarify that the tray was locked and that it (might) open up later,
> so try again.
> 
> It's not ideal, but it makes the semantics of the (legacy) eject
> command more understandable to end users when they try to use it.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  blockdev.c | 36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 1892b8e..feb8484 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2290,16 +2290,26 @@ exit:
>      block_job_txn_unref(block_job_txn);
>  }
>  
> +static int do_open_tray(const char *device, bool has_force, bool force,
> +                        Error **errp);
> +
>  void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
>  {
>      Error *local_err = NULL;
> +    int rc;
>  
> -    qmp_blockdev_open_tray(device, has_force, force, &local_err);
> +    rc = do_open_tray(device, has_force, force, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> +    if (rc == -EINPROGRESS) {
> +        error_setg(errp, "Device '%s' is locked and force was not specified, 
> "
> +                   "wait for tray to open and try again", device);
> +        return;
> +    }
> +
>      qmp_x_blockdev_remove_medium(device, errp);
>  }
>  
> @@ -2327,8 +2337,8 @@ void qmp_block_passwd(bool has_device, const char 
> *device,
>      aio_context_release(aio_context);
>  }
>  
> -void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
> -                            Error **errp)
> +static int do_open_tray(const char *device, bool has_force, bool force,
> +                        Error **errp)

Personally I feel the has_force and force could be merged as one parameter.

>  {
>      BlockBackend *blk;
>      bool locked;
> @@ -2341,21 +2351,21 @@ void qmp_blockdev_open_tray(const char *device, bool 
> has_force, bool force,
>      if (!blk) {
>          error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>                    "Device '%s' not found", device);
> -        return;
> +        return -ENODEV;
>      }
>  
>      if (!blk_dev_has_removable_media(blk)) {
>          error_setg(errp, "Device '%s' is not removable", device);
> -        return;
> +        return -ENOTSUP;
>      }
>  
>      if (!blk_dev_has_tray(blk)) {
>          /* Ignore this command on tray-less devices */
> -        return;
> +        return -ENOSYS;

I'm not sure how acceptable it is to leave errp untouched while setting ret
code to non-zero. Markus?

Fam

>      }
>  
>      if (blk_dev_is_tray_open(blk)) {
> -        return;
> +        return 0;
>      }
>  
>      locked = blk_dev_is_medium_locked(blk);
> @@ -2366,6 +2376,18 @@ void qmp_blockdev_open_tray(const char *device, bool 
> has_force, bool force,
>      if (!locked || force) {
>          blk_dev_change_media_cb(blk, false);
>      }
> +
> +    if (locked && !force) {
> +        return -EINPROGRESS;
> +    }
> +
> +    return 0;
> +}
> +
> +void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
> +                            Error **errp)
> +{
> +    do_open_tray(device, has_force, force, errp);
>  }
>  
>  void qmp_blockdev_close_tray(const char *device, Error **errp)
> -- 
> 2.4.11
> 
>