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

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2625 bytes --]

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 <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");

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;
>      }
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[Halil Pasic]

[-- Attachment #1: Type: text/plain, Size: 3114 bytes --]



On 07/18/2016 04:41 PM, Max Reitz wrote:
> 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 <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");
> 
> 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


Thanks for the comment!

Have no strong opinion here. AFAIU the errno argument is only used to
generate a message so there should be no consistency issue, and it would
be more consistent with the win32. How about moving both (posix and
win32) before the conditional statements readjusting the return value
and use errno and err directly?

Cheers,
Halil

> 
>>          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;
>>      }
>>  
>>
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3729 bytes --]

On 18.07.2016 17:48, Halil Pasic wrote:
> 
> 
> On 07/18/2016 04:41 PM, Max Reitz wrote:
>> 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 <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");
>>
>> 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
> 
> 
> Thanks for the comment!
> 
> Have no strong opinion here. AFAIU the errno argument is only used to
> generate a message so there should be no consistency issue, and it would
> be more consistent with the win32. How about moving both (posix and
> win32) before the conditional statements readjusting the return value
> and use errno and err directly?

Regarding win32, the issue is that we don't get an errno value but a
Windows-specific error value from GetLastError(). I don't think
error_setg_errno() understands those values. Therefore, for win32 we
don't have much choice but to use the "preprocessed" errno value.

I don't really see a consistency issue. It's just a human-readable error
message and I think we should be as specific as we can be; it's just
that it depends on the OS how much that is.

Max

> 
> Cheers,
> Halil
> 
>>
>>>          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;
>>>      }
>>>  
>>>
>>
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[Halil Pasic]

[-- Attachment #1: Type: text/plain, Size: 4444 bytes --]



On 07/18/2016 05:57 PM, Max Reitz wrote:
> On 18.07.2016 17:48, Halil Pasic wrote:
>>
>>
>> On 07/18/2016 04:41 PM, Max Reitz wrote:
>>> 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 <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");
>>>
>>> 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
>>
>>
>> Thanks for the comment!
>>
>> Have no strong opinion here. AFAIU the errno argument is only used to
>> generate a message so there should be no consistency issue, and it would
>> be more consistent with the win32. How about moving both (posix and
>> win32) before the conditional statements readjusting the return value
>> and use errno and err directly?
> 
> Regarding win32, the issue is that we don't get an errno value but a
> Windows-specific error value from GetLastError(). I don't think
> error_setg_errno() understands those values. Therefore, for win32 we

Of course you are right regarding the nature of the error code for win32.
Was not aware of that :/, so the win32 part was completely broken. 

> don't have much choice but to use the "preprocessed" errno value.
> 

We could use error_setg_win32 with the return value of GetLastError().
It basically uses 
https://developer.gnome.org/glib/stable/glib-Windows-Compatibility-Functions.html#g-win32-error-message
to get a message string from the error code. Would that be OK with you?

> I don't really see a consistency issue. It's just a human-readable error
> message and I think we should be as specific as we can be; it's just
> that it depends on the OS how much that is.
>

I agree. Wanted to say the same regarding consistency ;). Sorry if it did
not came across.

Thanks again for the catches!

Halil
 
> Max
> 
>>
>> Cheers,
>> Halil
>>
>>>
>>>>          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;
>>>>      }
>>>>  
>>>>
>>>
>>>
>>
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4706 bytes --]

On 18.07.2016 19:04, Halil Pasic wrote:
> 
> 
> On 07/18/2016 05:57 PM, Max Reitz wrote:
>> On 18.07.2016 17:48, Halil Pasic wrote:
>>>
>>>
>>> On 07/18/2016 04:41 PM, Max Reitz wrote:
>>>> 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 <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");
>>>>
>>>> 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
>>>
>>>
>>> Thanks for the comment!
>>>
>>> Have no strong opinion here. AFAIU the errno argument is only used to
>>> generate a message so there should be no consistency issue, and it would
>>> be more consistent with the win32. How about moving both (posix and
>>> win32) before the conditional statements readjusting the return value
>>> and use errno and err directly?
>>
>> Regarding win32, the issue is that we don't get an errno value but a
>> Windows-specific error value from GetLastError(). I don't think
>> error_setg_errno() understands those values. Therefore, for win32 we
> 
> Of course you are right regarding the nature of the error code for win32.
> Was not aware of that :/, so the win32 part was completely broken. 
> 
>> don't have much choice but to use the "preprocessed" errno value.
>>
> 
> We could use error_setg_win32 with the return value of GetLastError().
> It basically uses 
> https://developer.gnome.org/glib/stable/glib-Windows-Compatibility-Functions.html#g-win32-error-message
> to get a message string from the error code. Would that be OK with you?

Oh, I didn't even know we had that. That's nice. Yes, please go ahead!

Max

>> I don't really see a consistency issue. It's just a human-readable error
>> message and I think we should be as specific as we can be; it's just
>> that it depends on the OS how much that is.
>>
> 
> I agree. Wanted to say the same regarding consistency ;). Sorry if it did
> not came across.
> 
> Thanks again for the catches!
> 
> Halil
>  
>> Max
>>
>>>
>>> Cheers,
>>> Halil
>>>
>>>>
>>>>>          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;
>>>>>      }
>>>>>  
>>>>>
>>>>
>>>>
>>>
>>
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

[Qemu-devel] [PATCH v2 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.

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 c979ac3..786f068 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -485,6 +485,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     s->fd = -1;
     fd = qemu_open(filename, s->open_flags, 0644);
     if (fd < 0) {
+        error_setg_errno(errp, errno, "Could not open file");
         ret = -errno;
         if (ret == -EROFS) {
             ret = -EACCES;
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 62edb1a..6f074f4 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -337,6 +337,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 v2 1/1] block: improve error handling in raw_open

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2903 bytes --]

On 26.07.2016 13:34, 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.
> 
> 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 c979ac3..786f068 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -485,6 +485,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>      s->fd = -1;
>      fd = qemu_open(filename, s->open_flags, 0644);
>      if (fd < 0) {
> +        error_setg_errno(errp, errno, "Could not open file");

We don't guarantee that error_setg_errno() does not modify errno. (In
practice it should not, but we don't guarantee that.)

Therefore, the common pattern is to save the errno value before calling
this function, i.e. to put the function call...

>          ret = -errno;

...here.

With that fixed, the patch should be good.

Max

>          if (ret == -EROFS) {
>              ret = -EACCES;
> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index 62edb1a..6f074f4 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -337,6 +337,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 {
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[Halil Pasic]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]



On 07/26/2016 05:42 PM, Max Reitz wrote:
>> +++ b/block/raw-posix.c
>> > @@ -485,6 +485,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>> >      s->fd = -1;
>> >      fd = qemu_open(filename, s->open_flags, 0644);
>> >      if (fd < 0) {
>> > +        error_setg_errno(errp, errno, "Could not open file");
> We don't guarantee that error_setg_errno() does not modify errno. (In
> practice it should not, but we don't guarantee that.)
> 


Thank you very much for your review. I have double checked, and I
remembered correctly: error_setg_errno saves and restores the original
errno, so that is why I assumed it does not. 

> Therefore, the common pattern is to save the errno value before calling
> this function, i.e. to put the function call...
> 
>> >          ret = -errno;
> ...here.
> 
> With that fixed, the patch should be good.
> 
> Max
> 
 
But now that you have this pointed out, I understand, it is better to
have no function calls between failure and saving the errno. I will post
a v3 and keep this in mind for the future. Sorry for the extra work.

Frankly, I'm a bit uncomfortable with asking (do not want to be pushy),
but do you have an opinion on the 'error_report_err' issue (pointed
out in the cover letter part)?

Cheers,
Halil


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]

On 26.07.2016 19:18, Halil Pasic wrote:
> 
> 
> On 07/26/2016 05:42 PM, Max Reitz wrote:
>>> +++ b/block/raw-posix.c
>>>> @@ -485,6 +485,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>>>>      s->fd = -1;
>>>>      fd = qemu_open(filename, s->open_flags, 0644);
>>>>      if (fd < 0) {
>>>> +        error_setg_errno(errp, errno, "Could not open file");
>> We don't guarantee that error_setg_errno() does not modify errno. (In
>> practice it should not, but we don't guarantee that.)
>>
> 
> 
> Thank you very much for your review. I have double checked, and I
> remembered correctly: error_setg_errno saves and restores the original
> errno, so that is why I assumed it does not. 
> 
>> Therefore, the common pattern is to save the errno value before calling
>> this function, i.e. to put the function call...
>>
>>>>          ret = -errno;
>> ...here.
>>
>> With that fixed, the patch should be good.
>>
>> Max
>>
>  
> But now that you have this pointed out, I understand, it is better to
> have no function calls between failure and saving the errno. I will post
> a v3 and keep this in mind for the future. Sorry for the extra work.
> 
> Frankly, I'm a bit uncomfortable with asking (do not want to be pushy),
> but do you have an opinion on the 'error_report_err' issue (pointed
> out in the cover letter part)?

You are using drive_add with QMP? As far as I know, one can only do so
with human-monitor-command which returns the error string like so:

{"return": "Could not open file: No such file or directory\r\n"}

Apart from that, as for QMP, in theory you're supposed to use
blockdev-add and device_add, I think (although blockdev-add is still
considered experimental...). And blockdev-add will return the error
string like so:

{"error": {"class": "GenericError", "desc": "Could not open file: No
such file or directory"}}

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1426 bytes --]

On 26.07.2016 19:18, Halil Pasic wrote:
> 
> 
> On 07/26/2016 05:42 PM, Max Reitz wrote:
>>> +++ b/block/raw-posix.c
>>>> @@ -485,6 +485,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>>>>      s->fd = -1;
>>>>      fd = qemu_open(filename, s->open_flags, 0644);
>>>>      if (fd < 0) {
>>>> +        error_setg_errno(errp, errno, "Could not open file");
>> We don't guarantee that error_setg_errno() does not modify errno. (In
>> practice it should not, but we don't guarantee that.)
>>
> 
> 
> Thank you very much for your review. I have double checked, and I
> remembered correctly: error_setg_errno saves and restores the original
> errno, so that is why I assumed it does not. 

Oh, and about this: Yes, I remember, this was introduced after we had
noticed that we had some old code that assumed that error_setg() would
not modify errno. We had to decide between making error_setg*() save and
restore errno (which we deemed a bit ugly) and fixing all of that old
code (which we deemed hard). So we want for the former, but I don't
think we actually guarantee that behavior (because you should never rely
on any function not to modify errno).

(The difference between us saving/restoring errno in practice and
guaranteeing that feature is the lack of documentation thereof, i.e.,
the comment for error_setg() in include/qapi/error.h doesn't mention
this :-))

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[Sascha Silbe]

Dear Max,

Max Reitz <mreitz@redhat.com> writes:

> We don't guarantee that error_setg_errno() does not modify errno. (In
> practice it should not, but we don't guarantee that.)

I agree it's a good general policy to _not_ rely on the callee
explicitly saving and restoring errno. Especially since C11 pretty much
allows any "library function" (including e.g. va_start()) to clobber
errno...

In the case of error_setg_errno() it would be helpful to mention that in
the API docs. The current implementation goes out of its way to preserve
errno, so callers might come to rely on it.

So how about:

 /*
  * Just like error_setg(), with @os_error info added to the message.
  * If @os_error is non-zero, ": " + strerror(os_error) is appended to
  * the human-readable error message.
+ *
+ * Reminder: errno may get clobbered by almost any function call. If
+ * you need the value of errno for another purpose, save it before
+ * invoking error_setg_errno() (or any other function for that
+ * matter).
  */
 #define error_setg_errno(errp, os_error, fmt, ...)                      \
     error_setg_errno_internal((errp), __FILE__, __LINE__, __func__,     \
                               (os_error), (fmt), ## __VA_ARGS__)

(I can prepare a proper patch if you agree with the above.)

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1466 bytes --]

On 26.07.2016 20:03, Sascha Silbe wrote:
> Dear Max,
> 
> Max Reitz <mreitz@redhat.com> writes:
> 
>> We don't guarantee that error_setg_errno() does not modify errno. (In
>> practice it should not, but we don't guarantee that.)
> 
> I agree it's a good general policy to _not_ rely on the callee
> explicitly saving and restoring errno. Especially since C11 pretty much
> allows any "library function" (including e.g. va_start()) to clobber
> errno...
> 
> In the case of error_setg_errno() it would be helpful to mention that in
> the API docs. The current implementation goes out of its way to preserve
> errno, so callers might come to rely on it.
> 
> So how about:
> 
>  /*
>   * Just like error_setg(), with @os_error info added to the message.
>   * If @os_error is non-zero, ": " + strerror(os_error) is appended to
>   * the human-readable error message.
> + *
> + * Reminder: errno may get clobbered by almost any function call. If
> + * you need the value of errno for another purpose, save it before
> + * invoking error_setg_errno() (or any other function for that
> + * matter).
>   */
>  #define error_setg_errno(errp, os_error, fmt, ...)                      \
>      error_setg_errno_internal((errp), __FILE__, __LINE__, __func__,     \
>                                (os_error), (fmt), ## __VA_ARGS__)
> 
> (I can prepare a proper patch if you agree with the above.)

Thanks, that sounds good to me.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[John Snow]

On 07/26/2016 07:34 AM, 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)
> 

In the future, please configure your patch sending utility to send V2
patches as a new top-level thread, and not as a reply-to your V1.
Patches are easier to miss for maintainers when they show up as replies
to previously-nacked patchsets.

Thanks, and sorry for the pedanticism.
--js

> ---
> 
> 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.
> 
> 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 c979ac3..786f068 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -485,6 +485,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>      s->fd = -1;
>      fd = qemu_open(filename, s->open_flags, 0644);
>      if (fd < 0) {
> +        error_setg_errno(errp, errno, "Could not open file");
>          ret = -errno;
>          if (ret == -EROFS) {
>              ret = -EACCES;
> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index 62edb1a..6f074f4 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -337,6 +337,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 {
> 

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

[Sascha Silbe]

Dear Max,

Max Reitz <mreitz@redhat.com> writes:

>> So how about:
>> 
>>  /*
>>   * Just like error_setg(), with @os_error info added to the message.
>>   * If @os_error is non-zero, ": " + strerror(os_error) is appended to
>>   * the human-readable error message.
>> + *
>> + * Reminder: errno may get clobbered by almost any function call. If
>> + * you need the value of errno for another purpose, save it before
>> + * invoking error_setg_errno() (or any other function for that
>> + * matter).
>>   */
>>  #define error_setg_errno(errp, os_error, fmt, ...)                      \
>>      error_setg_errno_internal((errp), __FILE__, __LINE__, __func__,     \
>>                                (os_error), (fmt), ## __VA_ARGS__)
>> 
>> (I can prepare a proper patch if you agree with the above.)
>
> Thanks, that sounds good to me.

Great, sent out as a patch [1].

Sascha

[1] mid:1469558699-23314-1-git-send-email-silbe@linux.vnet.ibm.com
    "[PATCH] error: error_setg_errno(): errno may be clobbered" by
    Sascha Silbe <silbe@linux.vnet.ibm.com>, sent on 2016-07-26.
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

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

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 26.07.2016 19:18, Halil Pasic wrote:
>> 
>> 
>> On 07/26/2016 05:42 PM, Max Reitz wrote:
>>>> +++ b/block/raw-posix.c
>>>>> @@ -485,6 +485,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>>>>>      s->fd = -1;
>>>>>      fd = qemu_open(filename, s->open_flags, 0644);
>>>>>      if (fd < 0) {
>>>>> +        error_setg_errno(errp, errno, "Could not open file");
>>> We don't guarantee that error_setg_errno() does not modify errno. (In
>>> practice it should not, but we don't guarantee that.)
>>>
>> 
>> 
>> Thank you very much for your review. I have double checked, and I
>> remembered correctly: error_setg_errno saves and restores the original
>> errno, so that is why I assumed it does not. 
>
> Oh, and about this: Yes, I remember, this was introduced after we had
> noticed that we had some old code that assumed that error_setg() would
> not modify errno. We had to decide between making error_setg*() save and
> restore errno (which we deemed a bit ugly) and fixing all of that old
> code (which we deemed hard). So we want for the former, but I don't
> think we actually guarantee that behavior (because you should never rely
> on any function not to modify errno).

Since we rely on this behavior, we should definitely spell it out in the
function contract.

> (The difference between us saving/restoring errno in practice and
> guaranteeing that feature is the lack of documentation thereof, i.e.,
> the comment for error_setg() in include/qapi/error.h doesn't mention
> this :-))

Let's fix it.

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

[Halil Pasic]

[-- Attachment #1: Type: text/plain, Size: 2591 bytes --]



On 07/26/2016 07:47 PM, Max Reitz wrote:
>> Frankly, I'm a bit uncomfortable with asking (do not want to be pushy),
>> > but do you have an opinion on the 'error_report_err' issue (pointed
>> > out in the cover letter part)?
> You are using drive_add with QMP? As far as I know, one can only do so
> with human-monitor-command which returns the error string like so:
> 
> {"return": "Could not open file: No such file or directory\r\n"}
> 

Yes, libvirt uses human-monitor-command, but are you sure the error
message is propagated back as described above?

Here is the call chain I'm talking about:

#0  error_report_err (err=0x1329a9d0) at /mnt/devel/root/git/qemu/util/error.c:228
#1  0x00000000100de4fe in drive_new (all_opts=all_opts@entry=0x12044010, block_default_type=<optimized out>) at /mnt/devel/root/git/qemu/blockdev.c:1134
#2  0x00000000100e47be in add_init_drive (optstr=<optimized out>) at /mnt/devel/root/git/qemu/device-hotplug.c:46
#3  hmp_drive_add (mon=0x3ffe3f7d188, qdict=0x120b3af0) at /mnt/devel/root/git/qemu/device-hotplug.c:66
#4  0x0000000010052092 in handle_hmp_command (mon=mon@entry=0x3ffe3f7d188, cmdline=0x110399ba "dummy file=/dev/sg924,if=none,id=drive-hostdev912", cmdline@entry=0x110399b0 "drive_add dummy file=/dev/sg924,if=none,id=drive-hostdev912")
    at /mnt/devel/root/git/qemu/monitor.c:2929
#5  0x0000000010052176 in qmp_human_monitor_command (command_line=0x110399b0 "drive_add dummy file=/dev/sg924,if=none,id=drive-hostdev912", has_cpu_index=<optimized out>, cpu_index=0, errp=errp@entry=0x3ffe3f7d310)
    at /mnt/devel/root/git/qemu/monitor.c:668
#6  0x00000000100f6faa in qmp_marshal_human_monitor_command (args=<optimized out>, ret=0x3ffe3f7d400, errp=0x3ffe3f7d3f8) at qmp-marshal.c:1777
[...]

Now if you examine #1 drive_new(all_opts,block_default_type) you see,
there is no errp argument and if you examine the code you see that the
error from blockdev_init which gets propagated properly to this point
gets "handled" by error_report_err (in QMP context! so does not much
good on this code path). AFAIU this can not work. Or am I wrong?


> Apart from that, as for QMP, in theory you're supposed to use
> blockdev-add and device_add, I think (although blockdev-add is still
> considered experimental...). And blockdev-add will return the error
> string like so:
> 
> {"error": {"class": "GenericError", "desc": "Could not open file: No
> such file or directory"}}
> 

I guess libvirt will eventually pick up blockdev-add so the problem
should eventually go away.

Cheers,
Halil




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 26.07.2016 19:18, Halil Pasic wrote:
[...]
>> Frankly, I'm a bit uncomfortable with asking (do not want to be pushy),
>> but do you have an opinion on the 'error_report_err' issue (pointed
>> out in the cover letter part)?
>
> You are using drive_add with QMP? As far as I know, one can only do so
> with human-monitor-command which returns the error string like so:
>
> {"return": "Could not open file: No such file or directory\r\n"}
>
> Apart from that, as for QMP, in theory you're supposed to use
> blockdev-add

In theory, we're supposed to have completed development of blockdev-add
into a useful, supported interface...

>              and device_add, I think (although blockdev-add is still
> considered experimental...).

... but we haven't, and that's why libvirt isn't using it, and you
shouldn't be using it, either.

>                              And blockdev-add will return the error
> string like so:
>
> {"error": {"class": "GenericError", "desc": "Could not open file: No
> such file or directory"}}

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1604 bytes --]

On 27.07.2016 14:59, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 26.07.2016 19:18, Halil Pasic wrote:
> [...]
>>> Frankly, I'm a bit uncomfortable with asking (do not want to be pushy),
>>> but do you have an opinion on the 'error_report_err' issue (pointed
>>> out in the cover letter part)?
>>
>> You are using drive_add with QMP? As far as I know, one can only do so
>> with human-monitor-command which returns the error string like so:
>>
>> {"return": "Could not open file: No such file or directory\r\n"}
>>
>> Apart from that, as for QMP, in theory you're supposed to use
>> blockdev-add
> 
> In theory, we're supposed to have completed development of blockdev-add
> into a useful, supported interface...
> 
>>              and device_add, I think (although blockdev-add is still
>> considered experimental...).
> 
> ... but we haven't, and that's why libvirt isn't using it, and you
> shouldn't be using it, either.

Yes, you're right, of course. Anyway, I just tried to point out that
drive_add is not a QMP command (whereas blockdev-add is), so its error
reporting is rather awkward (in contrast to QMP's blockdev-add).

So the fact that drive_add's error reporting isn't very nice is not
because of drive_add itself but because of the fact that it's an HMP
command. The corresponding QMP command thus works just fine.

Max

>>                              And blockdev-add will return the error
>> string like so:
>>
>> {"error": {"class": "GenericError", "desc": "Could not open file: No
>> such file or directory"}}



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3594 bytes --]

On 27.07.2016 14:40, Halil Pasic wrote:
> 
> 
> On 07/26/2016 07:47 PM, Max Reitz wrote:
>>> Frankly, I'm a bit uncomfortable with asking (do not want to be pushy),
>>>> but do you have an opinion on the 'error_report_err' issue (pointed
>>>> out in the cover letter part)?
>> You are using drive_add with QMP? As far as I know, one can only do so
>> with human-monitor-command which returns the error string like so:
>>
>> {"return": "Could not open file: No such file or directory\r\n"}
>>
> 
> Yes, libvirt uses human-monitor-command, but are you sure the error
> message is propagated back as described above?
> 
> Here is the call chain I'm talking about:
> 
> #0  error_report_err (err=0x1329a9d0) at /mnt/devel/root/git/qemu/util/error.c:228
> #1  0x00000000100de4fe in drive_new (all_opts=all_opts@entry=0x12044010, block_default_type=<optimized out>) at /mnt/devel/root/git/qemu/blockdev.c:1134
> #2  0x00000000100e47be in add_init_drive (optstr=<optimized out>) at /mnt/devel/root/git/qemu/device-hotplug.c:46
> #3  hmp_drive_add (mon=0x3ffe3f7d188, qdict=0x120b3af0) at /mnt/devel/root/git/qemu/device-hotplug.c:66
> #4  0x0000000010052092 in handle_hmp_command (mon=mon@entry=0x3ffe3f7d188, cmdline=0x110399ba "dummy file=/dev/sg924,if=none,id=drive-hostdev912", cmdline@entry=0x110399b0 "drive_add dummy file=/dev/sg924,if=none,id=drive-hostdev912")
>     at /mnt/devel/root/git/qemu/monitor.c:2929
> #5  0x0000000010052176 in qmp_human_monitor_command (command_line=0x110399b0 "drive_add dummy file=/dev/sg924,if=none,id=drive-hostdev912", has_cpu_index=<optimized out>, cpu_index=0, errp=errp@entry=0x3ffe3f7d310)
>     at /mnt/devel/root/git/qemu/monitor.c:668
> #6  0x00000000100f6faa in qmp_marshal_human_monitor_command (args=<optimized out>, ret=0x3ffe3f7d400, errp=0x3ffe3f7d3f8) at qmp-marshal.c:1777
> [...]
> 
> Now if you examine #1 drive_new(all_opts,block_default_type) you see,
> there is no errp argument and if you examine the code you see that the
> error from blockdev_init which gets propagated properly to this point
> gets "handled" by error_report_err (in QMP context! so does not much
> good on this code path). AFAIU this can not work. Or am I wrong?

drive_add is an HMP command. There's no other way for it to emit errors.

Strictly speaking, HMP commands are not supposed to be used by
management applications like libvirt (the "H" stands for "human", after
all). QMP's "human-monitor-command" is just a workaround because there
are some HMP commands for which we do not have fully working QMP
replacements yet. One such example is indeed drive_add, because as
Markus correctly pointed out, blockdev-add is still considered experimental.

So we're still in the awkward spot of only having a legacy command
(drive_add) and an experimental command (blockdev-add); and we have been
in that spot for quite a while now (more than two years, I think). I
think we're getting rather close to getting blockdev-add stable, but
then again I'm afraid that might be something we've been thinking for
the past two years.

Max

>> Apart from that, as for QMP, in theory you're supposed to use
>> blockdev-add and device_add, I think (although blockdev-add is still
>> considered experimental...). And blockdev-add will return the error
>> string like so:
>>
>> {"error": {"class": "GenericError", "desc": "Could not open file: No
>> such file or directory"}}
>>
> 
> I guess libvirt will eventually pick up blockdev-add so the problem
> should eventually go away.
> 
> Cheers,
> Halil
> 
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[Halil Pasic]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]



On 07/27/2016 04:37 PM, Max Reitz wrote:
>> Now if you examine #1 drive_new(all_opts,block_default_type) you see,
>> > there is no errp argument and if you examine the code you see that the
>> > error from blockdev_init which gets propagated properly to this point
>> > gets "handled" by error_report_err (in QMP context! so does not much
>> > good on this code path). AFAIU this can not work. Or am I wrong?
> drive_add is an HMP command. There's no other way for it to emit errors.
> 
> Strictly speaking, HMP commands are not supposed to be used by
> management applications like libvirt (the "H" stands for "human", after
> all). QMP's "human-monitor-command" is just a workaround because there
> are some HMP commands for which we do not have fully working QMP
> replacements yet. One such example is indeed drive_add, because as
> Markus correctly pointed out, blockdev-add is still considered experimental.
> 
> So we're still in the awkward spot of only having a legacy command
> (drive_add) and an experimental command (blockdev-add); and we have been
> in that spot for quite a while now (more than two years, I think). I
> think we're getting rather close to getting blockdev-add stable, but
> then again I'm afraid that might be something we've been thinking for
> the past two years.
> 
> Max
> 

My primary concern was the function drive_new which does not
propagate/report errors adequately under certain conditions, and
although there are multiple usages of drive_new after looking into them
I'm getting convinced that they are OK -- besides the one pointed
out here.

I read your comment as: "We are already working on this. It is best for
you to ignore the problem.". Is my reading correct? If it is I
can live with that.

Thanks for the explanations.

Cheers,
Halil


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]