Re: [PATCH v3 4/4] block: switch to use qemu_open/qemu_create for improved errors
Daniel P. Berrangé writes:
> Currently at startup if using cache=none on a filesystem lacking
> O_DIRECT such as tmpfs, at startup QEMU prints
>
> qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: file system may not
> support O_DIRECT
> qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Could not open
> '/tmp/foo.img': Invalid argument
>
> while at QMP level the hint is missing, so QEMU reports just
>
> "error": {
> "class": "GenericError",
> "desc": "Could not open '/tmp/foo.img': Invalid argument"
> }
>
> which is close to useless for the end user trying to figure out what
> they did wrong.
>
> With this change at startup QEMU prints
>
> qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Unable to open
> '/tmp/foo.img' flags 0x4000: filesystem does not support O_DIRECT
>
> while at the QMP level QEMU reports a massively more informative
>
> "error": {
> "class": "GenericError",
> "desc": "Unable to open '/tmp/foo.img' flags 0x4002: filesystem does not
> support O_DIRECT"
> }
>
> Signed-off-by: Daniel P. Berrangé
Subject promises slightly more than the patch actually provides:
block/vvfat.c:1355:int fd = qemu_open(mapping->path, O_RDONLY |
O_BINARY | O_LARGEFILE);
block/vvfat.c:2516:fd = qemu_open(mapping->path, O_RDWR | O_CREAT |
O_BINARY, 0666);
If you'd rather not touch block/vvfat.c (I understand), consider
tweaking the subject to say
block/file: ...
Re: [PATCH v3 4/4] block: switch to use qemu_open/qemu_create for improved errors
On 7/24/20 3:25 PM, Daniel P. Berrangé wrote:
> Currently at startup if using cache=none on a filesystem lacking
> O_DIRECT such as tmpfs, at startup QEMU prints
>
> qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: file system may not
> support O_DIRECT
> qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Could not open
> '/tmp/foo.img': Invalid argument
>
> while at QMP level the hint is missing, so QEMU reports just
>
> "error": {
> "class": "GenericError",
> "desc": "Could not open '/tmp/foo.img': Invalid argument"
> }
>
> which is close to useless for the end user trying to figure out what
> they did wrong.
>
> With this change at startup QEMU prints
>
> qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Unable to open
> '/tmp/foo.img' flags 0x4000: filesystem does not support O_DIRECT
>
> while at the QMP level QEMU reports a massively more informative
>
> "error": {
> "class": "GenericError",
> "desc": "Unable to open '/tmp/foo.img' flags 0x4002: filesystem does not
> support O_DIRECT"
> }
>
> Signed-off-by: Daniel P. Berrangé
> ---
> block/file-posix.c| 18 +++---
> block/file-win32.c| 6 ++
> tests/qemu-iotests/051.out| 4 ++--
> tests/qemu-iotests/051.pc.out | 4 ++--
> tests/qemu-iotests/061.out| 2 +-
> tests/qemu-iotests/069.out| 2 +-
> tests/qemu-iotests/082.out| 4 ++--
> tests/qemu-iotests/111.out| 2 +-
> tests/qemu-iotests/226.out| 6 +++---
> tests/qemu-iotests/232.out| 12 ++--
> tests/qemu-iotests/244.out| 6 +++---
> 11 files changed, 30 insertions(+), 36 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index bac2566f10..c63926d592 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -630,11 +630,10 @@ static int raw_open_common(BlockDriverState *bs, QDict
> *options,
> raw_parse_flags(bdrv_flags, &s->open_flags, false);
>
> s->fd = -1;
> -fd = qemu_open_old(filename, s->open_flags, 0644);
> +fd = qemu_open(filename, s->open_flags, errp);
> ret = fd < 0 ? -errno : 0;
>
> if (ret < 0) {
> -error_setg_file_open(errp, -ret, filename);
> if (ret == -EROFS) {
> ret = -EACCES;
> }
> @@ -1032,15 +1031,13 @@ static int raw_reconfigure_getfd(BlockDriverState
> *bs, int flags,
> }
> }
>
> -/* If we cannot use fcntl, or fcntl failed, fall back to qemu_open_old()
> */
> +/* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
> if (fd == -1) {
> const char *normalized_filename = bs->filename;
> ret = raw_normalize_devicepath(&normalized_filename, errp);
> if (ret >= 0) {
> -assert(!(*open_flags & O_CREAT));
> -fd = qemu_open_old(normalized_filename, *open_flags);
> +fd = qemu_open(normalized_filename, *open_flags, errp);
> if (fd == -1) {
> -error_setg_errno(errp, errno, "Could not reopen file");
> return -1;
> }
> }
> @@ -2411,10 +2408,9 @@ raw_co_create(BlockdevCreateOptions *options, Error
> **errp)
> }
>
> /* Create file */
> -fd = qemu_open_old(file_opts->filename, O_RDWR | O_CREAT | O_BINARY,
> 0644);
> +fd = qemu_create(file_opts->filename, O_RDWR | O_BINARY, 0644, errp);
> if (fd < 0) {
> result = -errno;
> -error_setg_errno(errp, -result, "Could not create file");
> goto out;
> }
[...]
Nice :)
I haven't checked the iotest errors; assuming a CI will take care of it:
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v3 4/4] block: switch to use qemu_open/qemu_create for improved errors
On Fri, Jul 24, 2020 at 09:10:00AM -0500, Eric Blake wrote:
> On 7/24/20 8:25 AM, Daniel P. Berrangé wrote:
> > Currently at startup if using cache=none on a filesystem lacking
> > O_DIRECT such as tmpfs, at startup QEMU prints
> >
> > qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: file system may
> > not support O_DIRECT
> > qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Could not open
> > '/tmp/foo.img': Invalid argument
>
> Are we trying to get this in 5.1?
It is probably verging on too late to justify for the rc
>
> >
> > while at QMP level the hint is missing, so QEMU reports just
> >
> >"error": {
> >"class": "GenericError",
> >"desc": "Could not open '/tmp/foo.img': Invalid argument"
> >}
> >
> > which is close to useless for the end user trying to figure out what
> > they did wrong.
> >
> > With this change at startup QEMU prints
> >
> > qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Unable to open
> > '/tmp/foo.img' flags 0x4000: filesystem does not support O_DIRECT
> >
> > while at the QMP level QEMU reports a massively more informative
> >
> >"error": {
> > "class": "GenericError",
> > "desc": "Unable to open '/tmp/foo.img' flags 0x4002: filesystem does
> > not support O_DIRECT"
> >}
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
>
> > @@ -3335,7 +3331,7 @@ static bool setup_cdrom(char *bsd_path, Error **errp)
> > for (index = 0; index < num_of_test_partitions; index++) {
> > snprintf(test_partition, sizeof(test_partition), "%ss%d",
> > bsd_path,
> >index);
> > -fd = qemu_open_old(test_partition, O_RDONLY | O_BINARY |
> > O_LARGEFILE);
> > +fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE,
> > NULL);
>
> Should qemu_open() always be setting O_BINARY|O_LARGEFILE, without us having
> to worry about them at each caller? But that's a separate cleanup.
Hmm, I think both of these are dead code.
IIUC, O_BINARY is a no-op on any platform except Windows, and this is
file-posix.c, and O_LARGEFILE is a no-op, if you have _FILE_OFFSET_BITS=64,
which we hardcode.
> Reviewed-by: Eric Blake
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3 4/4] block: switch to use qemu_open/qemu_create for improved errors
On 7/24/20 8:25 AM, Daniel P. Berrangé wrote:
Currently at startup if using cache=none on a filesystem lacking
O_DIRECT such as tmpfs, at startup QEMU prints
qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: file system may not
support O_DIRECT
qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Could not open
'/tmp/foo.img': Invalid argument
Are we trying to get this in 5.1?
while at QMP level the hint is missing, so QEMU reports just
"error": {
"class": "GenericError",
"desc": "Could not open '/tmp/foo.img': Invalid argument"
}
which is close to useless for the end user trying to figure out what
they did wrong.
With this change at startup QEMU prints
qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Unable to open
'/tmp/foo.img' flags 0x4000: filesystem does not support O_DIRECT
while at the QMP level QEMU reports a massively more informative
"error": {
"class": "GenericError",
"desc": "Unable to open '/tmp/foo.img' flags 0x4002: filesystem does not
support O_DIRECT"
}
Signed-off-by: Daniel P. Berrangé
---
@@ -3335,7 +3331,7 @@ static bool setup_cdrom(char *bsd_path, Error **errp)
for (index = 0; index < num_of_test_partitions; index++) {
snprintf(test_partition, sizeof(test_partition), "%ss%d", bsd_path,
index);
-fd = qemu_open_old(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
+fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE,
NULL);
Should qemu_open() always be setting O_BINARY|O_LARGEFILE, without us
having to worry about them at each caller? But that's a separate cleanup.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[PATCH v3 4/4] block: switch to use qemu_open/qemu_create for improved errors
Currently at startup if using cache=none on a filesystem lacking
O_DIRECT such as tmpfs, at startup QEMU prints
qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: file system may not
support O_DIRECT
qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Could not open
'/tmp/foo.img': Invalid argument
while at QMP level the hint is missing, so QEMU reports just
"error": {
"class": "GenericError",
"desc": "Could not open '/tmp/foo.img': Invalid argument"
}
which is close to useless for the end user trying to figure out what
they did wrong.
With this change at startup QEMU prints
qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Unable to open
'/tmp/foo.img' flags 0x4000: filesystem does not support O_DIRECT
while at the QMP level QEMU reports a massively more informative
"error": {
"class": "GenericError",
"desc": "Unable to open '/tmp/foo.img' flags 0x4002: filesystem does not
support O_DIRECT"
}
Signed-off-by: Daniel P. Berrangé
---
block/file-posix.c| 18 +++---
block/file-win32.c| 6 ++
tests/qemu-iotests/051.out| 4 ++--
tests/qemu-iotests/051.pc.out | 4 ++--
tests/qemu-iotests/061.out| 2 +-
tests/qemu-iotests/069.out| 2 +-
tests/qemu-iotests/082.out| 4 ++--
tests/qemu-iotests/111.out| 2 +-
tests/qemu-iotests/226.out| 6 +++---
tests/qemu-iotests/232.out| 12 ++--
tests/qemu-iotests/244.out| 6 +++---
11 files changed, 30 insertions(+), 36 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index bac2566f10..c63926d592 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -630,11 +630,10 @@ static int raw_open_common(BlockDriverState *bs, QDict
*options,
raw_parse_flags(bdrv_flags, &s->open_flags, false);
s->fd = -1;
-fd = qemu_open_old(filename, s->open_flags, 0644);
+fd = qemu_open(filename, s->open_flags, errp);
ret = fd < 0 ? -errno : 0;
if (ret < 0) {
-error_setg_file_open(errp, -ret, filename);
if (ret == -EROFS) {
ret = -EACCES;
}
@@ -1032,15 +1031,13 @@ static int raw_reconfigure_getfd(BlockDriverState *bs,
int flags,
}
}
-/* If we cannot use fcntl, or fcntl failed, fall back to qemu_open_old() */
+/* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
if (fd == -1) {
const char *normalized_filename = bs->filename;
ret = raw_normalize_devicepath(&normalized_filename, errp);
if (ret >= 0) {
-assert(!(*open_flags & O_CREAT));
-fd = qemu_open_old(normalized_filename, *open_flags);
+fd = qemu_open(normalized_filename, *open_flags, errp);
if (fd == -1) {
-error_setg_errno(errp, errno, "Could not reopen file");
return -1;
}
}
@@ -2411,10 +2408,9 @@ raw_co_create(BlockdevCreateOptions *options, Error
**errp)
}
/* Create file */
-fd = qemu_open_old(file_opts->filename, O_RDWR | O_CREAT | O_BINARY, 0644);
+fd = qemu_create(file_opts->filename, O_RDWR | O_BINARY, 0644, errp);
if (fd < 0) {
result = -errno;
-error_setg_errno(errp, -result, "Could not create file");
goto out;
}
@@ -3335,7 +3331,7 @@ static bool setup_cdrom(char *bsd_path, Error **errp)
for (index = 0; index < num_of_test_partitions; index++) {
snprintf(test_partition, sizeof(test_partition), "%ss%d", bsd_path,
index);
-fd = qemu_open_old(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
+fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE,
NULL);
if (fd >= 0) {
partition_found = true;
qemu_close(fd);
@@ -3653,7 +3649,7 @@ static int cdrom_probe_device(const char *filename)
int prio = 0;
struct stat st;
-fd = qemu_open_old(filename, O_RDONLY | O_NONBLOCK);
+fd = qemu_open(filename, O_RDONLY | O_NONBLOCK, NULL);
if (fd < 0) {
goto out;
}
@@ -3787,7 +3783,7 @@ static int cdrom_reopen(BlockDriverState *bs)
*/
if (s->fd >= 0)
qemu_close(s->fd);
-fd = qemu_open_old(bs->filename, s->open_flags, 0644);
+fd = qemu_open(bs->filename, s->open_flags, NULL);
if (fd < 0) {
s->fd = -1;
return -EIO;
diff --git a/block/file-win32.c b/block/file-win32.c
index 8c1845830e..1a31f8a5ba 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -576,11 +576,9 @@ static int raw_co_create(BlockdevCreateOptions *options,
Error **errp)
return -EINVAL;
}
-fd = qemu_open_old(file_opts->filename,
- O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
- 0644);
+fd = qemu_create(file_opts->filename, O_WRONLY | O_TRUNC | O_BINARY,
+ 0644, errp);
if (fd < 0) {
-error_setg_errno(errp, errno, "Could not create file");
