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

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

[Halil Pasic]

Make raw_open for POSIX more consistent in handling errors by setting
the error object also when qemu_open fails. The error object was set
generally set in case of errors, but I guess this case was overlooked.
Do the same for win32.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Tested-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> (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.

v2 -> v3:
* first save errno then error_setg_errno
v1 -> v2:
* fixed win32 by the correct error_setg_*
* use the original errno consequently
---
 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 6ed7547..51e4ddb 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -436,6 +436,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     fd = qemu_open(filename, s->open_flags, 0644);
     if (fd < 0) {
         ret = -errno;
+        error_setg_errno(errp, errno, "Could not open file");
         if (ret == -EROFS) {
             ret = -EACCES;
         }
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 56f45fe..185dd9b 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -338,6 +338,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     if (s->hfile == INVALID_HANDLE_VALUE) {
         int err = GetLastError();
 
+        error_setg_win32(errp, err, "Could not open file");
         if (err == ERROR_ACCESS_DENIED) {
             ret = -EACCES;
         } else {
-- 
2.6.6

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

[Halil Pasic]

On 07/27/2016 05:28 PM, 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 set
> generally set in case of errors, but I guess this case was overlooked.
> Do the same for win32.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> Tested-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> (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.
> 
> v2 -> v3:
> * first save errno then error_setg_errno
> v1 -> v2:
> * fixed win32 by the correct error_setg_*
> * use the original errno consequently
> ---
>  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 6ed7547..51e4ddb 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -436,6 +436,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>      fd = qemu_open(filename, s->open_flags, 0644);
>      if (fd < 0) {
>          ret = -errno;
> +        error_setg_errno(errp, errno, "Could not open file");
>          if (ret == -EROFS) {
>              ret = -EACCES;
>          }
> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index 56f45fe..185dd9b 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -338,6 +338,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
>      if (s->hfile == INVALID_HANDLE_VALUE) {
>          int err = GetLastError();
> 
> +        error_setg_win32(errp, err, "Could not open file");
>          if (err == ERROR_ACCESS_DENIED) {
>              ret = -EACCES;
>          } else {
> 

Sorry its v3.

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

[Halil Pasic]

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 <pasic@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Tested-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> (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");
         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;
     }
 
-- 
2.6.6