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 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;
>>> }
>>>
>>>
>>
>>
>
signature.asc
Description: OpenPGP digital signature
