| Message ID | 1469532873-78542-1-git-send-email-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 { >
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
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
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
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
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
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 { >
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.
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.
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
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"}}
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"}}
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 > > >
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
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 {