| Message ID | 1468845016-43468-1-git-send-email-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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; > } > >
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; >> } >> >> > >
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; >>> } >>> >>> >> >> >
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; >>>> } >>>> >>>> >>> >>> >> > >
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; >>>>> } >>>>> >>>>> >>>> >>>> >>> >> >> >
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; }