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 <pa...@linux.vnet.ibm.com>
>>>> Reviewed-by: Cornelia Huck <cornelia.h...@de.ibm.com>
>>>> Reviewed-by: Sascha Silbe <si...@linux.vnet.ibm.com>
>>>> Tested-by: Marc Hartmayer <mhart...@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 weOf 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; >>>> } >>>> >>>> >>> >>> >> > >
signature.asc
Description: OpenPGP digital signature
