Re: [Qemu-devel] [Qemu-block] [PATCH 1/1] block: improve error handling in raw_open

[Max Reitz]

On 18.07.2016 14:30, Halil Pasic wrote:
> Make raw_open for POSIX more consistent in handling errors by setting
> the error object also when qemu_open fails. The error object was
> generally set in case of errors, but I guess this case was overlooked.
> Do the same for win32.
> 
> Signed-off-by: Halil Pasic <address@hidden>
> Reviewed-by: Cornelia Huck <address@hidden>
> Reviewed-by: Sascha Silbe <address@hidden>
> Tested-by: Marc Hartmayer <address@hidden> (POSIX only)
> 
> ---
> 
> Stumbled upon this (POSIX) while testing VMs with too many SCSI disks in
> respect to my nofile limit. When open hits the nofile limit while trying
> to hotplug yet another SCSI disk via libvirt we end up with no adequate
> error message (one stating too many files). Sadly this patch in not
> sufficient to fix this problem because drive_new (/qemu/blockdev.c)
> handles errors using error_report_err which is documented as not to be
> used in QMP context. Do not have a patch for that, because I'm unsure
> whats the best way to deal with it. My guess right now is to make sure
> we propagate errors at least until reaching code which is called  only
> QMP in context and handle communicating the error to the requester of
> the operation there. Any suggestions or ideas?
> 
> The win32 part was not tested, and the sole reason I touched it is
> to not introduce unnecessary divergence.
> ---
>  block/raw-posix.c | 1 +
>  block/raw-win32.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index c979ac3..4a7056e 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -489,6 +489,7 @@ static int raw_open_common(BlockDriverState *bs, QDict 
> *options,
>          if (ret == -EROFS) {
>              ret = -EACCES;
>          }
> +        error_setg_errno(errp, -ret, "Could not open file");

How about putting this above the "if (ret == -EROFS)" block? While other
parts of qemu may want to treat EROFS and EACCES in the same way, I
think it makes sense to distinguish both cases in messages meant for a
human user.

Max

>          goto fail;
>      }
>      s->fd = fd;
> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index 62edb1a..f324f4e 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -342,6 +342,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
> int flags,
>          } else {
>              ret = -EINVAL;
>          }
> +        error_setg_errno(errp, err, "Could not open file");
>          goto fail;
>      }
>  
>

signature.asc
Description: OpenPGP digital signature