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

[Max Reitz]

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

signature.asc
Description: OpenPGP digital signature