[PATCH v2 0/3] block: improve error reporting for unsupported O_DIRECT

[PATCH v2 0/3] block: improve error reporting for unsupported O_DIRECT

[Daniel P. Berrangé]

v1: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg00269.html

See patch commit messages for rationale

Ideally we would convert other callers of qemu_open to use
qemu_open_err, and eventually remove qemu_open, renaming
qemu_open_err back to qemu_open.  Given soft freeze is just
days away though, I'm hoping this series is simple enough
to get into this release, leaving bigger cleanup for later.

Improved in v2:

 - Mention that qemu_open_err is preferred over qemu_open
 - Get rid of obsolete error_report call
 - Simplify O_DIRECT handling
 - Fixup iotests for changed error message text

Daniel P. Berrangé (3):
  util: validate whether O_DIRECT is supported after failure
  util: support detailed error reporting for qemu_open
  block: switch to use qemu_open_err for improved errors

 block/file-posix.c            | 10 ++---
 include/qemu/osdep.h          |  2 +
 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 +--
 util/osdep.c                  | 69 +++++++++++++++++++++++++++++------
 12 files changed, 85 insertions(+), 38 deletions(-)

-- 
2.26.2

[PATCH v2 1/3] util: validate whether O_DIRECT is supported after failure

[Daniel P. Berrangé]

Currently we suggest that a filesystem may not support O_DIRECT after
seeing an EINVAL. Other things can cause EINVAL though, so it is better
to do an explicit check, and then report a definitive error message.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 util/osdep.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/util/osdep.c b/util/osdep.c
index 4829c07ff6..e2b7507ee2 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -332,9 +332,11 @@ int qemu_open(const char *name, int flags, ...)
     }
 
 #ifdef O_CLOEXEC
-    ret = open(name, flags | O_CLOEXEC, mode);
-#else
+    flags |= O_CLOEXEC;
+#endif
     ret = open(name, flags, mode);
+
+#ifndef O_CLOEXEC
     if (ret >= 0) {
         qemu_set_cloexec(ret);
     }
@@ -342,8 +344,13 @@ int qemu_open(const char *name, int flags, ...)
 
 #ifdef O_DIRECT
     if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
-        error_report("file system may not support O_DIRECT");
-        errno = EINVAL; /* in case it was clobbered */
+        int newflags = flags & ~O_DIRECT;
+        ret = open(name, newflags, mode);
+        if (ret != -1) {
+            close(ret);
+            error_report("file system does not support O_DIRECT");
+            errno = EINVAL;
+        }
     }
 #endif /* O_DIRECT */
 
-- 
2.26.2

[PATCH v2 2/3] util: support detailed error reporting for qemu_open

[Daniel P. Berrangé]

Create a "qemu_open_err" method which does the same as "qemu_open",
but with a "Error **errp" for error reporting. There should be no
behavioural difference for existing callers at this stage.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qemu/osdep.h |  2 ++
 util/osdep.c         | 66 +++++++++++++++++++++++++++++++++++---------
 2 files changed, 55 insertions(+), 13 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 0d26a1b9bd..8506a978fa 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -483,6 +483,8 @@ int qemu_madvise(void *addr, size_t len, int advice);
 int qemu_mprotect_rwx(void *addr, size_t size);
 int qemu_mprotect_none(void *addr, size_t size);
 
+/* This is preferred over qemu_open for its improved error reporting */
+int qemu_open_err(const char *name, int flags, Error **errp, ...);
 int qemu_open(const char *name, int flags, ...);
 int qemu_close(int fd);
 int qemu_unlink(const char *name);
diff --git a/util/osdep.c b/util/osdep.c
index e2b7507ee2..3de8bee463 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -22,6 +22,7 @@
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 
 /* Needed early for CONFIG_BSD etc. */
 
@@ -282,7 +283,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
 /*
  * Opens a file with FD_CLOEXEC set
  */
-int qemu_open(const char *name, int flags, ...)
+static int qemu_openv(const char *name, int flags, Error **errp, va_list ap)
 {
     int ret;
     int mode = 0;
@@ -297,24 +298,31 @@ int qemu_open(const char *name, int flags, ...)
 
         fdset_id = qemu_parse_fdset(fdset_id_str);
         if (fdset_id == -1) {
+            error_setg(errp, "Could not parse fdset %s", name);
             errno = EINVAL;
             return -1;
         }
 
         fd = monitor_fdset_get_fd(fdset_id, flags);
         if (fd < 0) {
+            error_setg_errno(errp, -fd, "Could not acquire FD for %s flags %x",
+                             name, flags);
             errno = -fd;
             return -1;
         }
 
         dupfd = qemu_dup_flags(fd, flags);
         if (dupfd == -1) {
+            error_setg_errno(errp, errno, "Could not dup FD for %s flags %x",
+                             name, flags);
             return -1;
         }
 
         ret = monitor_fdset_dup_fd_add(fdset_id, dupfd);
         if (ret == -1) {
             close(dupfd);
+            error_setg(errp, "Could not save FD for %s flags %x",
+                       name, flags);
             errno = EINVAL;
             return -1;
         }
@@ -324,11 +332,7 @@ int qemu_open(const char *name, int flags, ...)
 #endif
 
     if (flags & O_CREAT) {
-        va_list ap;
-
-        va_start(ap, flags);
         mode = va_arg(ap, int);
-        va_end(ap);
     }
 
 #ifdef O_CLOEXEC
@@ -342,21 +346,57 @@ int qemu_open(const char *name, int flags, ...)
     }
 #endif
 
+    if (ret == -1) {
+        const char *action = "open";
+        if (flags & O_CREAT) {
+            action = "create";
+        }
 #ifdef O_DIRECT
-    if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
-        int newflags = flags & ~O_DIRECT;
-        ret = open(name, newflags, mode);
-        if (ret != -1) {
-            close(ret);
-            error_report("file system does not support O_DIRECT");
-            errno = EINVAL;
+        if (errno == EINVAL && (flags & O_DIRECT)) {
+            int newflags = flags & ~O_DIRECT;
+            ret = open(name, newflags, mode);
+            if (ret != -1) {
+                close(ret);
+                error_setg(errp, "Could not %s '%s' flags 0x%x: "
+                           "filesystem does not support O_DIRECT",
+                           action, name, flags);
+                errno = EINVAL;
+                return -1;
+            }
         }
-    }
 #endif /* O_DIRECT */
+        error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
+                         action, name, flags);
+    }
+
 
     return ret;
 }
 
+int qemu_open_err(const char *name, int flags, Error **errp, ...)
+{
+    va_list ap;
+    int rv;
+
+    va_start(ap, errp);
+    rv = qemu_openv(name, flags, errp, ap);
+    va_end(ap);
+
+    return rv;
+}
+
+int qemu_open(const char *name, int flags, ...)
+{
+    va_list ap;
+    int rv;
+
+    va_start(ap, flags);
+    rv = qemu_openv(name, flags, NULL, ap);
+    va_end(ap);
+
+    return rv;
+}
+
 int qemu_close(int fd)
 {
     int64_t fdset_id;
-- 
2.26.2

[PATCH v2 3/3] block: switch to use qemu_open_err for improved errors

[Daniel P. Berrangé]

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é <berrange@redhat.com>
---
 block/file-posix.c            | 10 ++++------
 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 +++---
 10 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 3ab8f5a0fa..2865b789fb 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -574,11 +574,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(filename, s->open_flags, 0644);
+    fd = qemu_open_err(filename, s->open_flags, errp, 0644);
     ret = fd < 0 ? -errno : 0;
 
     if (ret < 0) {
-        error_setg_file_open(errp, -ret, filename);
         if (ret == -EROFS) {
             ret = -EACCES;
         }
@@ -970,9 +969,8 @@ static int raw_reconfigure_getfd(BlockDriverState *bs, int flags,
         ret = raw_normalize_devicepath(&normalized_filename, errp);
         if (ret >= 0) {
             assert(!(*open_flags & O_CREAT));
-            fd = qemu_open(normalized_filename, *open_flags);
+            fd = qemu_open_err(normalized_filename, *open_flags, errp);
             if (fd == -1) {
-                error_setg_errno(errp, errno, "Could not reopen file");
                 return -1;
             }
         }
@@ -2324,10 +2322,10 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp)
     }
 
     /* Create file */
-    fd = qemu_open(file_opts->filename, O_RDWR | O_CREAT | O_BINARY, 0644);
+    fd = qemu_open_err(file_opts->filename, O_RDWR | O_CREAT | O_BINARY,
+                       errp, 0644);
     if (fd < 0) {
         result = -errno;
-        error_setg_errno(errp, -result, "Could not create file");
         goto out;
     }
 
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 554c5ca90a..1a80eac0ce 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -363,7 +363,7 @@ Testing: -drive file=foo:bar
 QEMU_PROG: -drive file=foo:bar: Unknown protocol 'foo'
 
 Testing: -drive file.filename=foo:bar
-QEMU_PROG: -drive file.filename=foo:bar: Could not open 'foo:bar': No such file or directory
+QEMU_PROG: -drive file.filename=foo:bar: Could not open 'foo:bar' flags 0x08000: No such file or directory
 
 Testing: -hda file:TEST_DIR/t.qcow2
 QEMU X.Y.Z monitor - type 'help' for more information
@@ -374,7 +374,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) quit
 
 Testing: -drive file.filename=file:TEST_DIR/t.qcow2
-QEMU_PROG: -drive file.filename=file:TEST_DIR/t.qcow2: Could not open 'file:TEST_DIR/t.qcow2': No such file or directory
+QEMU_PROG: -drive file.filename=file:TEST_DIR/t.qcow2: Could not open 'file:TEST_DIR/t.qcow2' flags 0x80000: No such file or directory
 
 
 === Snapshot mode ===
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index 0ea80d35f0..bf8f96287b 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -463,7 +463,7 @@ Testing: -drive file=foo:bar
 QEMU_PROG: -drive file=foo:bar: Unknown protocol 'foo'
 
 Testing: -drive file.filename=foo:bar
-QEMU_PROG: -drive file.filename=foo:bar: Could not open 'foo:bar': No such file or directory
+QEMU_PROG: -drive file.filename=foo:bar: Could not open 'foo:bar' flags 0x80000: No such file or directory
 
 Testing: -hda file:TEST_DIR/t.qcow2
 QEMU X.Y.Z monitor - type 'help' for more information
@@ -474,7 +474,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) quit
 
 Testing: -drive file.filename=file:TEST_DIR/t.qcow2
-QEMU_PROG: -drive file.filename=file:TEST_DIR/t.qcow2: Could not open 'file:TEST_DIR/t.qcow2': No such file or directory
+QEMU_PROG: -drive file.filename=file:TEST_DIR/t.qcow2: Could not open 'file:TEST_DIR/t.qcow2' flags 0x80000: No such file or directory
 
 
 === Snapshot mode ===
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index 2f03cf045c..b4bc1f3112 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -533,7 +533,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 qemu-img: data-file can only be set for images that use an external data file
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 data_file=TEST_DIR/t.IMGFMT.data
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'foo': No such file or directory
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'foo' flags 0x80000: No such file or directory
 image: TEST_DIR/t.IMGFMT
 file format: IMGFMT
 virtual size: 64 MiB (67108864 bytes)
diff --git a/tests/qemu-iotests/069.out b/tests/qemu-iotests/069.out
index c78e8c2b72..60da51f7da 100644
--- a/tests/qemu-iotests/069.out
+++ b/tests/qemu-iotests/069.out
@@ -4,5 +4,5 @@ QA output created by 069
 
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=131072
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 backing_file=TEST_DIR/t.IMGFMT.base
-qemu-io: can't open device TEST_DIR/t.IMGFMT: Could not open backing file: Could not open 'TEST_DIR/t.IMGFMT.base': No such file or directory
+qemu-io: can't open device TEST_DIR/t.IMGFMT: Could not open backing file: Could not open 'TEST_DIR/t.IMGFMT.base' flags 0x80000: No such file or directory
 *** done
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index 529a1214e1..7e57235183 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -528,10 +528,10 @@ Supported options:
   size=<size>            - Virtual disk size
 
 Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
-qemu-img: Could not open 'TEST_DIR/t.qcow2.base': Could not open backing file: Could not open 'TEST_DIR/t.qcow2,help': No such file or directory
+qemu-img: Could not open 'TEST_DIR/t.qcow2.base': Could not open backing file: Could not open 'TEST_DIR/t.qcow2,help' flags 0x80000: No such file or directory
 
 Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
-qemu-img: Could not open 'TEST_DIR/t.qcow2.base': Could not open backing file: Could not open 'TEST_DIR/t.qcow2,?': No such file or directory
+qemu-img: Could not open 'TEST_DIR/t.qcow2.base': Could not open backing file: Could not open 'TEST_DIR/t.qcow2,?' flags 0x80000: No such file or directory
 
 Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2, -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
 qemu-img: Invalid option list: backing_file=TEST_DIR/t.qcow2,
diff --git a/tests/qemu-iotests/111.out b/tests/qemu-iotests/111.out
index 5279c462fc..f25f6bf573 100644
--- a/tests/qemu-iotests/111.out
+++ b/tests/qemu-iotests/111.out
@@ -1,4 +1,4 @@
 QA output created by 111
-qemu-img: TEST_DIR/t.IMGFMT: Could not open 'TEST_DIR/t.IMGFMT.inexistent': No such file or directory
+qemu-img: TEST_DIR/t.IMGFMT: Could not open 'TEST_DIR/t.IMGFMT.inexistent' flags 0x80000: No such file or directory
 Could not open backing image to determine size.
 *** done
diff --git a/tests/qemu-iotests/226.out b/tests/qemu-iotests/226.out
index 42be973ff2..353fc4c799 100644
--- a/tests/qemu-iotests/226.out
+++ b/tests/qemu-iotests/226.out
@@ -6,7 +6,7 @@ QA output created by 226
 qemu-io: can't open: A regular file was expected by the 'file' driver, but something else was given
 qemu-io: warning: Opening a character device as a file using the 'file' driver is deprecated
 == Testing RW ==
-qemu-io: can't open: Could not open 'TEST_DIR/t.IMGFMT': Is a directory
+qemu-io: can't open: Could not open 'TEST_DIR/t.IMGFMT' flags 0x80002: Is a directory
 qemu-io: warning: Opening a character device as a file using the 'file' driver is deprecated
 
 === Testing with driver:host_device ===
@@ -14,13 +14,13 @@ qemu-io: warning: Opening a character device as a file using the 'file' driver i
 == Testing RO ==
 qemu-io: can't open: 'host_device' driver expects either a character or block device
 == Testing RW ==
-qemu-io: can't open: Could not open 'TEST_DIR/t.IMGFMT': Is a directory
+qemu-io: can't open: Could not open 'TEST_DIR/t.IMGFMT' flags 0x80002: Is a directory
 
 === Testing with driver:host_cdrom ===
 
 == Testing RO ==
 qemu-io: can't open: 'host_cdrom' driver expects either a character or block device
 == Testing RW ==
-qemu-io: can't open: Could not open 'TEST_DIR/t.IMGFMT': Is a directory
+qemu-io: can't open: Could not open 'TEST_DIR/t.IMGFMT' flags 0x80802: Is a directory
 
 *** done
diff --git a/tests/qemu-iotests/232.out b/tests/qemu-iotests/232.out
index 3bd1a920af..bfa3f20172 100644
--- a/tests/qemu-iotests/232.out
+++ b/tests/qemu-iotests/232.out
@@ -21,11 +21,11 @@ NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
 NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
 NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
 
-QEMU_PROG: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none,read-only=off,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+QEMU_PROG: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none,read-only=off,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT' flags 0x80002: Permission denied
 NODE_NAME: TEST_DIR/t.IMGFMT (file)
 NODE_NAME: TEST_DIR/t.IMGFMT (file)
 
-QEMU_PROG: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+QEMU_PROG: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT' flags 0x80002: Permission denied
 NODE_NAME: TEST_DIR/t.IMGFMT (file)
 NODE_NAME: TEST_DIR/t.IMGFMT (file)
 
@@ -49,11 +49,11 @@ node0: TEST_DIR/t.IMGFMT (file, read-only)
 node0: TEST_DIR/t.IMGFMT (file, read-only)
 node0: TEST_DIR/t.IMGFMT (file, read-only)
 
-QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,read-only=off,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,read-only=off,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT' flags 0x80002: Permission denied
 node0: TEST_DIR/t.IMGFMT (file)
-QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,read-only=off: Could not open 'TEST_DIR/t.IMGFMT' flags 0x80002: Permission denied
 
-QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT' flags 0x80002: Permission denied
 node0: TEST_DIR/t.IMGFMT (file)
-QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0: Could not open 'TEST_DIR/t.IMGFMT' flags 0x80002: Permission denied
 *** done
diff --git a/tests/qemu-iotests/244.out b/tests/qemu-iotests/244.out
index dbab7359a9..34daec97f2 100644
--- a/tests/qemu-iotests/244.out
+++ b/tests/qemu-iotests/244.out
@@ -9,7 +9,7 @@ read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-qemu-io: can't open device TEST_DIR/t.qcow2: Could not open 'inexistent': No such file or directory
+qemu-io: can't open device TEST_DIR/t.qcow2: Could not open 'inexistent' flags 0x80002: No such file or directory
 no file open, try 'help open'
 
 Data file required, but without data file name in the image:
@@ -17,14 +17,14 @@ qemu-io: can't open device TEST_DIR/t.qcow2: 'data-file' is required for this im
 no file open, try 'help open'
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-qemu-io: can't open device TEST_DIR/t.qcow2: Could not open 'inexistent': No such file or directory
+qemu-io: can't open device TEST_DIR/t.qcow2: Could not open 'inexistent' flags 0x80002: No such file or directory
 no file open, try 'help open'
 
 Setting data-file for an image with internal data:
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 qemu-io: can't open device TEST_DIR/t.qcow2: 'data-file' can only be set for images with an external data file
 no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow2: Could not open 'inexistent': No such file or directory
+qemu-io: can't open device TEST_DIR/t.qcow2: Could not open 'inexistent' flags 0x80002: No such file or directory
 no file open, try 'help open'
 
 === Conflicting features ===
-- 
2.26.2

Re: [PATCH v2 1/3] util: validate whether O_DIRECT is supported after failure

[Philippe Mathieu-Daudé]

On 7/2/20 2:56 PM, Daniel P. Berrangé wrote:
> Currently we suggest that a filesystem may not support O_DIRECT after
> seeing an EINVAL. Other things can cause EINVAL though, so it is better
> to do an explicit check, and then report a definitive error message.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  util/osdep.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/util/osdep.c b/util/osdep.c
> index 4829c07ff6..e2b7507ee2 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -332,9 +332,11 @@ int qemu_open(const char *name, int flags, ...)
>      }
>  
>  #ifdef O_CLOEXEC
> -    ret = open(name, flags | O_CLOEXEC, mode);
> -#else
> +    flags |= O_CLOEXEC;
> +#endif
>      ret = open(name, flags, mode);
> +
> +#ifndef O_CLOEXEC
>      if (ret >= 0) {
>          qemu_set_cloexec(ret);
>      }
> @@ -342,8 +344,13 @@ int qemu_open(const char *name, int flags, ...)

Too bad the git-diff removed 2 lines of context to add a comment instead.

In case it helps other reviewers, what was stripped out of the
context is this single line:

   #endif

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>  
>  #ifdef O_DIRECT
>      if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
> -        error_report("file system may not support O_DIRECT");
> -        errno = EINVAL; /* in case it was clobbered */
> +        int newflags = flags & ~O_DIRECT;
> +        ret = open(name, newflags, mode);
> +        if (ret != -1) {
> +            close(ret);
> +            error_report("file system does not support O_DIRECT");
> +            errno = EINVAL;
> +        }
>      }
>  #endif /* O_DIRECT */
>  
> 

Re: [PATCH v2 1/3] util: validate whether O_DIRECT is supported after failure

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> Currently we suggest that a filesystem may not support O_DIRECT after
> seeing an EINVAL. Other things can cause EINVAL though, so it is better
> to do an explicit check, and then report a definitive error message.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  util/osdep.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/util/osdep.c b/util/osdep.c
> index 4829c07ff6..e2b7507ee2 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -332,9 +332,11 @@ int qemu_open(const char *name, int flags, ...)
>      }
>  
>  #ifdef O_CLOEXEC
> -    ret = open(name, flags | O_CLOEXEC, mode);
> -#else
> +    flags |= O_CLOEXEC;
> +#endif
>      ret = open(name, flags, mode);
> +
> +#ifndef O_CLOEXEC
>      if (ret >= 0) {
>          qemu_set_cloexec(ret);
>      }

I'd prefer something like

   #ifdef O_CLOEXEC
       flags |= O_CLOEXEC;
       ret = open(name, flags, mode);
   #else
       ret = open(name, flags, mode);
       if (ret >= 0) {
           qemu_set_cloexec(ret);
       }
   #endif

Continues to duplicate open(), but spares me the effort to fuse two
#ifdef sections in my head to understand what is being done in each
case.

> @@ -342,8 +344,13 @@ int qemu_open(const char *name, int flags, ...)
>  
>  #ifdef O_DIRECT
>      if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
> -        error_report("file system may not support O_DIRECT");
> -        errno = EINVAL; /* in case it was clobbered */
> +        int newflags = flags & ~O_DIRECT;
> +        ret = open(name, newflags, mode);

I'd prefer the more concise

           ret = open(name, flags & ~O_DIRECT, mode);

> +        if (ret != -1) {
> +            close(ret);
> +            error_report("file system does not support O_DIRECT");
> +            errno = EINVAL;
> +        }
>      }
>  #endif /* O_DIRECT */

The function now reports to stderr in just one of many failure modes.
That's wrong.  Looks like the next patch fixes this defect.  I'd swap
the two.

Re: [PATCH v2 2/3] util: support detailed error reporting for qemu_open

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> Create a "qemu_open_err" method which does the same as "qemu_open",
> but with a "Error **errp" for error reporting. There should be no
> behavioural difference for existing callers at this stage.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/qemu/osdep.h |  2 ++
>  util/osdep.c         | 66 +++++++++++++++++++++++++++++++++++---------
>  2 files changed, 55 insertions(+), 13 deletions(-)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 0d26a1b9bd..8506a978fa 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -483,6 +483,8 @@ int qemu_madvise(void *addr, size_t len, int advice);
>  int qemu_mprotect_rwx(void *addr, size_t size);
>  int qemu_mprotect_none(void *addr, size_t size);
>  
> +/* This is preferred over qemu_open for its improved error reporting */
> +int qemu_open_err(const char *name, int flags, Error **errp, ...);
>  int qemu_open(const char *name, int flags, ...);
>  int qemu_close(int fd);
>  int qemu_unlink(const char *name);

I still think renaming qemu_open() to something like
qemu_open_with_useless_errors() is the way to go.

> diff --git a/util/osdep.c b/util/osdep.c
> index e2b7507ee2..3de8bee463 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -22,6 +22,7 @@
>   * THE SOFTWARE.
>   */
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
>  
>  /* Needed early for CONFIG_BSD etc. */
>  
> @@ -282,7 +283,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
>  /*
>   * Opens a file with FD_CLOEXEC set
>   */
> -int qemu_open(const char *name, int flags, ...)
> +static int qemu_openv(const char *name, int flags, Error **errp, va_list ap)
>  {
>      int ret;
>      int mode = 0;
> @@ -297,24 +298,31 @@ int qemu_open(const char *name, int flags, ...)
>  
>          fdset_id = qemu_parse_fdset(fdset_id_str);
>          if (fdset_id == -1) {
> +            error_setg(errp, "Could not parse fdset %s", name);
>              errno = EINVAL;
>              return -1;
>          }
>  
>          fd = monitor_fdset_get_fd(fdset_id, flags);
>          if (fd < 0) {
> +            error_setg_errno(errp, -fd, "Could not acquire FD for %s flags %x",
> +                             name, flags);
>              errno = -fd;
>              return -1;
>          }
>  
>          dupfd = qemu_dup_flags(fd, flags);
>          if (dupfd == -1) {
> +            error_setg_errno(errp, errno, "Could not dup FD for %s flags %x",
> +                             name, flags);
>              return -1;
>          }
>  
>          ret = monitor_fdset_dup_fd_add(fdset_id, dupfd);
>          if (ret == -1) {
>              close(dupfd);
> +            error_setg(errp, "Could not save FD for %s flags %x",
> +                       name, flags);
>              errno = EINVAL;
>              return -1;
>          }
> @@ -324,11 +332,7 @@ int qemu_open(const char *name, int flags, ...)
>  #endif
>  
>      if (flags & O_CREAT) {
> -        va_list ap;
> -
> -        va_start(ap, flags);
>          mode = va_arg(ap, int);
> -        va_end(ap);
>      }
>  
>  #ifdef O_CLOEXEC
> @@ -342,21 +346,57 @@ int qemu_open(const char *name, int flags, ...)
>      }
>  #endif
>  
> +    if (ret == -1) {
> +        const char *action = "open";
> +        if (flags & O_CREAT) {
> +            action = "create";
> +        }

I'd prefer the more concise single assignment

           const char *action = flags & O_CREAT ? "create" : "open";

I'd also put it right at the beginning of the function.

>  #ifdef O_DIRECT
> -    if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
> -        int newflags = flags & ~O_DIRECT;
> -        ret = open(name, newflags, mode);
> -        if (ret != -1) {
> -            close(ret);
> -            error_report("file system does not support O_DIRECT");
> -            errno = EINVAL;
> +        if (errno == EINVAL && (flags & O_DIRECT)) {
> +            int newflags = flags & ~O_DIRECT;
> +            ret = open(name, newflags, mode);
> +            if (ret != -1) {
> +                close(ret);
> +                error_setg(errp, "Could not %s '%s' flags 0x%x: "
> +                           "filesystem does not support O_DIRECT",
> +                           action, name, flags);
> +                errno = EINVAL;
> +                return -1;
> +            }
>          }
> -    }
>  #endif /* O_DIRECT */
> +        error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
> +                         action, name, flags);
> +    }
> +

Swapping with PATCH 1 will reduce churn here.

>  
>      return ret;
>  }
>  
> +int qemu_open_err(const char *name, int flags, Error **errp, ...)
> +{
> +    va_list ap;
> +    int rv;
> +
> +    va_start(ap, errp);
> +    rv = qemu_openv(name, flags, errp, ap);
> +    va_end(ap);
> +
> +    return rv;
> +}
> +
> +int qemu_open(const char *name, int flags, ...)
> +{
> +    va_list ap;
> +    int rv;
> +
> +    va_start(ap, flags);
> +    rv = qemu_openv(name, flags, NULL, ap);
> +    va_end(ap);
> +
> +    return rv;
> +}
> +
>  int qemu_close(int fd)
>  {
>      int64_t fdset_id;

Preferably with suggested improvements:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 3/3] block: switch to use qemu_open_err for improved errors

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com>

Narrowing down the error's cause takes extra code, but the error message
improvement is worth it.

Reviewed-by: Markus Armbruster <armbru@redhat.com>