[PATCH v4 0/6] block: improve error reporting for unsupported O_DIRECT

[PATCH v4 0/6] block: improve error reporting for unsupported O_DIRECT

[Daniel P. Berrangé]

v1: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg00269.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg00589.html
v3: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg07098.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 v4:

 - Use assert() for programmer mistakes
 - Split second patch into three distinct parts
 - Misc typos
 - Improve commit message

Improved in v3:

 - Re-arrange the patches series, so that the conversion to Error
   takes place first, then the improve O_DIRECT reporting
 - Rename existing method to qemu_open_old
 - Use a pair of new methods qemu_open + qemu_create to improve
   arg checking

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é (6):
  util: rename qemu_open() to qemu_open_old()
  util: refactor qemu_open_old to split off variadic args handling
  util: add Error object for qemu_open_internal error reporting
  util: introduce qemu_open and qemu_create with error reporting
  util: give a specific error message when O_DIRECT doesn't work
  block/fileb: switch to use qemu_open/qemu_create for improved errors

 accel/kvm/kvm-all.c            |  2 +-
 backends/rng-random.c          |  2 +-
 backends/tpm/tpm_passthrough.c |  8 ++--
 block/file-posix.c             | 16 +++----
 block/file-win32.c             |  5 +-
 block/vvfat.c                  |  5 +-
 chardev/char-fd.c              |  2 +-
 chardev/char-pipe.c            |  6 +--
 chardev/char.c                 |  2 +-
 dump/dump.c                    |  2 +-
 hw/s390x/s390-skeys.c          |  2 +-
 hw/usb/host-libusb.c           |  2 +-
 hw/vfio/common.c               |  4 +-
 include/qemu/osdep.h           |  8 +++-
 io/channel-file.c              |  2 +-
 net/vhost-vdpa.c               |  2 +-
 os-posix.c                     |  2 +-
 qga/channel-posix.c            |  4 +-
 qga/commands-posix.c           |  6 +--
 target/arm/kvm.c               |  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 +--
 ui/console.c                   |  2 +-
 util/osdep.c                   | 83 ++++++++++++++++++++++++++++------
 util/oslib-posix.c             |  2 +-
 32 files changed, 136 insertions(+), 77 deletions(-)

-- 
2.26.2

[PATCH v4 1/6] util: rename qemu_open() to qemu_open_old()

[Daniel P. Berrangé]

We want to introduce a new version of qemu_open() that uses an Error
object for reporting problems and make this it the preferred interface.
Rename the existing method to release the namespace for the new impl.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 accel/kvm/kvm-all.c            |  2 +-
 backends/rng-random.c          |  2 +-
 backends/tpm/tpm_passthrough.c |  8 ++++----
 block/file-posix.c             | 14 +++++++-------
 block/file-win32.c             |  5 +++--
 block/vvfat.c                  |  5 +++--
 chardev/char-fd.c              |  2 +-
 chardev/char-pipe.c            |  6 +++---
 chardev/char.c                 |  2 +-
 dump/dump.c                    |  2 +-
 hw/s390x/s390-skeys.c          |  2 +-
 hw/usb/host-libusb.c           |  2 +-
 hw/vfio/common.c               |  4 ++--
 include/qemu/osdep.h           |  2 +-
 io/channel-file.c              |  2 +-
 net/vhost-vdpa.c               |  2 +-
 os-posix.c                     |  2 +-
 qga/channel-posix.c            |  4 ++--
 qga/commands-posix.c           |  6 +++---
 target/arm/kvm.c               |  2 +-
 ui/console.c                   |  2 +-
 util/osdep.c                   |  2 +-
 util/oslib-posix.c             |  2 +-
 23 files changed, 42 insertions(+), 40 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 63ef6af9a1..ad8b315b35 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2013,7 +2013,7 @@ static int kvm_init(MachineState *ms)
 #endif
     QLIST_INIT(&s->kvm_parked_vcpus);
     s->vmfd = -1;
-    s->fd = qemu_open("/dev/kvm", O_RDWR);
+    s->fd = qemu_open_old("/dev/kvm", O_RDWR);
     if (s->fd == -1) {
         fprintf(stderr, "Could not access KVM kernel module: %m\n");
         ret = -errno;
diff --git a/backends/rng-random.c b/backends/rng-random.c
index 32998d8ee7..245b12ab24 100644
--- a/backends/rng-random.c
+++ b/backends/rng-random.c
@@ -75,7 +75,7 @@ static void rng_random_opened(RngBackend *b, Error **errp)
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "filename", "a valid filename");
     } else {
-        s->fd = qemu_open(s->filename, O_RDONLY | O_NONBLOCK);
+        s->fd = qemu_open_old(s->filename, O_RDONLY | O_NONBLOCK);
         if (s->fd == -1) {
             error_setg_file_open(errp, errno, s->filename);
         }
diff --git a/backends/tpm/tpm_passthrough.c b/backends/tpm/tpm_passthrough.c
index 7403807ec4..81e2d8f531 100644
--- a/backends/tpm/tpm_passthrough.c
+++ b/backends/tpm/tpm_passthrough.c
@@ -217,7 +217,7 @@ static int tpm_passthrough_open_sysfs_cancel(TPMPassthruState *tpm_pt)
     char path[PATH_MAX];
 
     if (tpm_pt->options->cancel_path) {
-        fd = qemu_open(tpm_pt->options->cancel_path, O_WRONLY);
+        fd = qemu_open_old(tpm_pt->options->cancel_path, O_WRONLY);
         if (fd < 0) {
             error_report("tpm_passthrough: Could not open TPM cancel path: %s",
                          strerror(errno));
@@ -235,11 +235,11 @@ static int tpm_passthrough_open_sysfs_cancel(TPMPassthruState *tpm_pt)
     dev++;
     if (snprintf(path, sizeof(path), "/sys/class/tpm/%s/device/cancel",
                  dev) < sizeof(path)) {
-        fd = qemu_open(path, O_WRONLY);
+        fd = qemu_open_old(path, O_WRONLY);
         if (fd < 0) {
             if (snprintf(path, sizeof(path), "/sys/class/misc/%s/device/cancel",
                          dev) < sizeof(path)) {
-                fd = qemu_open(path, O_WRONLY);
+                fd = qemu_open_old(path, O_WRONLY);
             }
         }
     }
@@ -271,7 +271,7 @@ tpm_passthrough_handle_device_opts(TPMPassthruState *tpm_pt, QemuOpts *opts)
     }
 
     tpm_pt->tpm_dev = value ? value : TPM_PASSTHROUGH_DEFAULT_DEVICE;
-    tpm_pt->tpm_fd = qemu_open(tpm_pt->tpm_dev, O_RDWR);
+    tpm_pt->tpm_fd = qemu_open_old(tpm_pt->tpm_dev, O_RDWR);
     if (tpm_pt->tpm_fd < 0) {
         error_report("Cannot access TPM device using '%s': %s",
                      tpm_pt->tpm_dev, strerror(errno));
diff --git a/block/file-posix.c b/block/file-posix.c
index 9a00d4190a..bac2566f10 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -630,7 +630,7 @@ 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_old(filename, s->open_flags, 0644);
     ret = fd < 0 ? -errno : 0;
 
     if (ret < 0) {
@@ -1032,13 +1032,13 @@ static int raw_reconfigure_getfd(BlockDriverState *bs, int flags,
         }
     }
 
-    /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
+    /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open_old() */
     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(normalized_filename, *open_flags);
+            fd = qemu_open_old(normalized_filename, *open_flags);
             if (fd == -1) {
                 error_setg_errno(errp, errno, "Could not reopen file");
                 return -1;
@@ -2411,7 +2411,7 @@ 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_old(file_opts->filename, O_RDWR | O_CREAT | O_BINARY, 0644);
     if (fd < 0) {
         result = -errno;
         error_setg_errno(errp, -result, "Could not create file");
@@ -3335,7 +3335,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(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
+        fd = qemu_open_old(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
         if (fd >= 0) {
             partition_found = true;
             qemu_close(fd);
@@ -3653,7 +3653,7 @@ static int cdrom_probe_device(const char *filename)
     int prio = 0;
     struct stat st;
 
-    fd = qemu_open(filename, O_RDONLY | O_NONBLOCK);
+    fd = qemu_open_old(filename, O_RDONLY | O_NONBLOCK);
     if (fd < 0) {
         goto out;
     }
@@ -3787,7 +3787,7 @@ static int cdrom_reopen(BlockDriverState *bs)
      */
     if (s->fd >= 0)
         qemu_close(s->fd);
-    fd = qemu_open(bs->filename, s->open_flags, 0644);
+    fd = qemu_open_old(bs->filename, s->open_flags, 0644);
     if (fd < 0) {
         s->fd = -1;
         return -EIO;
diff --git a/block/file-win32.c b/block/file-win32.c
index ab69bd811a..8c1845830e 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -576,8 +576,9 @@ static int raw_co_create(BlockdevCreateOptions *options, Error **errp)
         return -EINVAL;
     }
 
-    fd = qemu_open(file_opts->filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
-                   0644);
+    fd = qemu_open_old(file_opts->filename,
+                       O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
+                       0644);
     if (fd < 0) {
         error_setg_errno(errp, errno, "Could not create file");
         return -EIO;
diff --git a/block/vvfat.c b/block/vvfat.c
index 36b53c8757..5abb90e7c7 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1352,7 +1352,8 @@ static int open_file(BDRVVVFATState* s,mapping_t* mapping)
     if(!s->current_mapping ||
             strcmp(s->current_mapping->path,mapping->path)) {
         /* open file */
-        int fd = qemu_open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE);
+        int fd = qemu_open_old(mapping->path,
+                               O_RDONLY | O_BINARY | O_LARGEFILE);
         if(fd<0)
             return -1;
         vvfat_close_current_file(s);
@@ -2513,7 +2514,7 @@ static int commit_one_file(BDRVVVFATState* s,
     for (i = s->cluster_size; i < offset; i += s->cluster_size)
         c = modified_fat_get(s, c);
 
-    fd = qemu_open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
+    fd = qemu_open_old(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
     if (fd < 0) {
         fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path,
                 strerror(errno), errno);
diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index c2d8101106..1cd62f2779 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -119,7 +119,7 @@ int qmp_chardev_open_file_source(char *src, int flags, Error **errp)
 {
     int fd = -1;
 
-    TFR(fd = qemu_open(src, flags, 0666));
+    TFR(fd = qemu_open_old(src, flags, 0666));
     if (fd == -1) {
         error_setg_file_open(errp, errno, src);
     }
diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
index fd12c9e63b..7eca5d9a56 100644
--- a/chardev/char-pipe.c
+++ b/chardev/char-pipe.c
@@ -132,8 +132,8 @@ static void qemu_chr_open_pipe(Chardev *chr,
 
     filename_in = g_strdup_printf("%s.in", filename);
     filename_out = g_strdup_printf("%s.out", filename);
-    TFR(fd_in = qemu_open(filename_in, O_RDWR | O_BINARY));
-    TFR(fd_out = qemu_open(filename_out, O_RDWR | O_BINARY));
+    TFR(fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY));
+    TFR(fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY));
     g_free(filename_in);
     g_free(filename_out);
     if (fd_in < 0 || fd_out < 0) {
@@ -143,7 +143,7 @@ static void qemu_chr_open_pipe(Chardev *chr,
         if (fd_out >= 0) {
             close(fd_out);
         }
-        TFR(fd_in = fd_out = qemu_open(filename, O_RDWR | O_BINARY));
+        TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY));
         if (fd_in < 0) {
             error_setg_file_open(errp, errno, filename);
             return;
diff --git a/chardev/char.c b/chardev/char.c
index 77e7ec814f..6b85099c03 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -235,7 +235,7 @@ static void qemu_char_open(Chardev *chr, ChardevBackend *backend,
         } else {
             flags |= O_TRUNC;
         }
-        chr->logfd = qemu_open(common->logfile, flags, 0666);
+        chr->logfd = qemu_open_old(common->logfile, flags, 0666);
         if (chr->logfd < 0) {
             error_setg_errno(errp, errno,
                              "Unable to open logfile %s",
diff --git a/dump/dump.c b/dump/dump.c
index 383bc7876b..13fda440a4 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1994,7 +1994,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
 #endif
 
     if  (strstart(file, "file:", &p)) {
-        fd = qemu_open(p, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR);
+        fd = qemu_open_old(p, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR);
         if (fd < 0) {
             error_setg_file_open(errp, errno, p);
             return;
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index db2f49cb27..5cc559fe4c 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -125,7 +125,7 @@ void qmp_dump_skeys(const char *filename, Error **errp)
         return;
     }
 
-    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0600);
+    fd = qemu_open_old(filename, O_WRONLY | O_CREAT | O_TRUNC, 0600);
     if (fd < 0) {
         error_setg_file_open(errp, errno, filename);
         return;
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index c474551d84..14dd4de684 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1111,7 +1111,7 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
     if (s->hostdevice) {
         int fd;
         s->needs_autoscan = false;
-        fd = qemu_open(s->hostdevice, O_RDWR);
+        fd = qemu_open_old(s->hostdevice, O_RDWR);
         if (fd < 0) {
             error_setg_errno(errp, errno, "failed to open %s", s->hostdevice);
             return;
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 33357140b8..13471ae294 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1254,7 +1254,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
         }
     }
 
-    fd = qemu_open("/dev/vfio/vfio", O_RDWR);
+    fd = qemu_open_old("/dev/vfio/vfio", O_RDWR);
     if (fd < 0) {
         error_setg_errno(errp, errno, "failed to open /dev/vfio/vfio");
         ret = -errno;
@@ -1479,7 +1479,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
     group = g_malloc0(sizeof(*group));
 
     snprintf(path, sizeof(path), "/dev/vfio/%d", groupid);
-    group->fd = qemu_open(path, O_RDWR);
+    group->fd = qemu_open_old(path, O_RDWR);
     if (group->fd < 0) {
         error_setg_errno(errp, errno, "failed to open %s", path);
         goto free_group_exit;
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 20872e793e..18333e9006 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -497,7 +497,7 @@ 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);
 
-int qemu_open(const char *name, int flags, ...);
+int qemu_open_old(const char *name, int flags, ...);
 int qemu_close(int fd);
 int qemu_unlink(const char *name);
 #ifndef _WIN32
diff --git a/io/channel-file.c b/io/channel-file.c
index dac21f6012..2ed3b75e8b 100644
--- a/io/channel-file.c
+++ b/io/channel-file.c
@@ -51,7 +51,7 @@ qio_channel_file_new_path(const char *path,
 
     ioc = QIO_CHANNEL_FILE(object_new(TYPE_QIO_CHANNEL_FILE));
 
-    ioc->fd = qemu_open(path, flags, mode);
+    ioc->fd = qemu_open_old(path, flags, mode);
     if (ioc->fd < 0) {
         object_unref(OBJECT(ioc));
         error_setg_errno(errp, errno,
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index bc0e0d2d35..e2b3ba85bf 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -184,7 +184,7 @@ static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
     snprintf(nc->info_str, sizeof(nc->info_str), TYPE_VHOST_VDPA);
     nc->queue_index = 0;
     s = DO_UPCAST(VhostVDPAState, nc, nc);
-    vdpa_device_fd = qemu_open(vhostdev, O_RDWR);
+    vdpa_device_fd = qemu_open_old(vhostdev, O_RDWR);
     if (vdpa_device_fd == -1) {
         return -errno;
     }
diff --git a/os-posix.c b/os-posix.c
index 3572db3f44..1b927c7c04 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -297,7 +297,7 @@ void os_setup_post(void)
             error_report("not able to chdir to /: %s", strerror(errno));
             exit(1);
         }
-        TFR(fd = qemu_open("/dev/null", O_RDWR));
+        TFR(fd = qemu_open_old("/dev/null", O_RDWR));
         if (fd == -1) {
             exit(1);
         }
diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 8fc205ad21..0373975360 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -127,7 +127,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
     switch (c->method) {
     case GA_CHANNEL_VIRTIO_SERIAL: {
         assert(fd < 0);
-        fd = qemu_open(path, O_RDWR | O_NONBLOCK
+        fd = qemu_open_old(path, O_RDWR | O_NONBLOCK
 #ifndef CONFIG_SOLARIS
                            | O_ASYNC
 #endif
@@ -157,7 +157,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
         struct termios tio;
 
         assert(fd < 0);
-        fd = qemu_open(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
+        fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
         if (fd == -1) {
             g_critical("error opening channel: %s", strerror(errno));
             return false;
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1a62a3a70d..ffe0d24bf3 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1304,7 +1304,7 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
             }
         }
 
-        fd = qemu_open(mount->dirname, O_RDONLY);
+        fd = qemu_open_old(mount->dirname, O_RDONLY);
         if (fd == -1) {
             error_setg_errno(errp, errno, "failed to open %s", mount->dirname);
             goto error;
@@ -1371,7 +1371,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
 
     QTAILQ_FOREACH(mount, &mounts, next) {
         logged = false;
-        fd = qemu_open(mount->dirname, O_RDONLY);
+        fd = qemu_open_old(mount->dirname, O_RDONLY);
         if (fd == -1) {
             continue;
         }
@@ -1461,7 +1461,7 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
         list->next = response->paths;
         response->paths = list;
 
-        fd = qemu_open(mount->dirname, O_RDONLY);
+        fd = qemu_open_old(mount->dirname, O_RDONLY);
         if (fd == -1) {
             result->error = g_strdup_printf("failed to open: %s",
                                             strerror(errno));
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 8bb7318378..f944bfa0dc 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -71,7 +71,7 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try,
 {
     int ret = 0, kvmfd = -1, vmfd = -1, cpufd = -1;
 
-    kvmfd = qemu_open("/dev/kvm", O_RDWR);
+    kvmfd = qemu_open_old("/dev/kvm", O_RDWR);
     if (kvmfd < 0) {
         goto err;
     }
diff --git a/ui/console.c b/ui/console.c
index 0579be792f..02eca16bd7 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -372,7 +372,7 @@ void qmp_screendump(const char *filename, bool has_device, const char *device,
         return;
     }
 
-    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666);
+    fd = qemu_open_old(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666);
     if (fd == -1) {
         error_setg(errp, "failed to open file '%s': %s", filename,
                    strerror(errno));
diff --git a/util/osdep.c b/util/osdep.c
index 4829c07ff6..9df1b6adec 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -282,7 +282,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, ...)
+int qemu_open_old(const char *name, int flags, ...)
 {
     int ret;
     int mode = 0;
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index ad8001a4ad..f5f676f079 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -125,7 +125,7 @@ bool qemu_write_pidfile(const char *path, Error **errp)
             .l_len = 0,
         };
 
-        fd = qemu_open(path, O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR);
+        fd = qemu_open_old(path, O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR);
         if (fd == -1) {
             error_setg_errno(errp, errno, "Cannot open pid file");
             return false;
-- 
2.26.2

[PATCH v4 2/6] util: refactor qemu_open_old to split off variadic args handling

[Daniel P. Berrangé]

This simple refactoring prepares for future patches. The variadic args
handling is split from the main bulk of the open logic. The duplicated
calls to open() are removed in favour of updating the "flags" variable
to have O_CLOEXEC.

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

diff --git a/util/osdep.c b/util/osdep.c
index 9df1b6adec..9ff92551e7 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,10 +283,10 @@ 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_old(const char *name, int flags, ...)
+static int
+qemu_open_internal(const char *name, int flags, mode_t mode)
 {
     int ret;
-    int mode = 0;
 
 #ifndef _WIN32
     const char *fdset_id_str;
@@ -323,22 +324,35 @@ int qemu_open_old(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
-    ret = open(name, flags | O_CLOEXEC, mode);
-#else
+    flags |= O_CLOEXEC;
+#endif /* O_CLOEXEC */
+
     ret = open(name, flags, mode);
+
+#ifndef O_CLOEXEC
     if (ret >= 0) {
         qemu_set_cloexec(ret);
     }
-#endif
+#endif /* ! O_CLOEXEC */
+
+    return ret;
+}
+
+
+int qemu_open_old(const char *name, int flags, ...)
+{
+    va_list ap;
+    mode_t mode = 0;
+    int ret;
+
+    va_start(ap, flags);
+    if (flags & O_CREAT) {
+        mode = va_arg(ap, int);
+    }
+    va_end(ap);
+
+    ret = qemu_open_internal(name, flags, mode);
 
 #ifdef O_DIRECT
     if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
-- 
2.26.2

[PATCH v4 3/6] util: add Error object for qemu_open_internal error reporting

[Daniel P. Berrangé]

Instead of relying on the limited information from errno, we can now
also provide detailed error messages.

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

diff --git a/util/osdep.c b/util/osdep.c
index 9ff92551e7..9c7118d3cb 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -284,7 +284,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
  * Opens a file with FD_CLOEXEC set
  */
 static int
-qemu_open_internal(const char *name, int flags, mode_t mode)
+qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
 {
     int ret;
 
@@ -298,24 +298,31 @@ qemu_open_internal(const char *name, int flags, mode_t mode)
 
         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;
         }
@@ -336,6 +343,16 @@ qemu_open_internal(const char *name, int flags, mode_t mode)
     }
 #endif /* ! O_CLOEXEC */
 
+    if (ret == -1) {
+        const char *action = "open";
+        if (flags & O_CREAT) {
+            action = "create";
+        }
+        error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
+                         action, name, flags);
+    }
+
+
     return ret;
 }
 
@@ -352,7 +369,7 @@ int qemu_open_old(const char *name, int flags, ...)
     }
     va_end(ap);
 
-    ret = qemu_open_internal(name, flags, mode);
+    ret = qemu_open_internal(name, flags, mode, NULL);
 
 #ifdef O_DIRECT
     if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
-- 
2.26.2

[PATCH v4 4/6] util: introduce qemu_open and qemu_create with error reporting

[Daniel P. Berrangé]

qemu_open_old() works like open(): set errno and return -1 on failure.
It has even more failure modes, though.  Reporting the error clearly
to users is basically impossible for many of them.

Our standard cure for "errno is too coarse" is the Error object.
Introduce two new helper methods:

  int qemu_open(const char *name, int flags, Error **errp);
  int qemu_create(const char *name, int flags, mode_t mode, Error **errp);

Note that with this design we no longer require or even accept the
O_CREAT flag. Avoiding overloading the two distinct operations
means we can avoid variable arguments which would prevent 'errp' from
being the last argument. It also gives us a guarantee that the 'mode' is
given when creating files, avoiding a latent security bug.

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

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 18333e9006..13a821845b 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -497,7 +497,13 @@ 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);
 
+/*
+ * Don't introduce new usage of this function, prefer the following
+ * qemu_open/qemu_create that take an "Error **errp"
+ */
 int qemu_open_old(const char *name, int flags, ...);
+int qemu_open(const char *name, int flags, Error **errp);
+int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
 int qemu_close(int fd);
 int qemu_unlink(const char *name);
 #ifndef _WIN32
diff --git a/util/osdep.c b/util/osdep.c
index 9c7118d3cb..a4956fbf6b 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -344,10 +344,7 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
 #endif /* ! O_CLOEXEC */
 
     if (ret == -1) {
-        const char *action = "open";
-        if (flags & O_CREAT) {
-            action = "create";
-        }
+        const char *action = flags & O_CREAT ? "create" : "open";
         error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
                          action, name, flags);
     }
@@ -357,6 +354,22 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
 }
 
 
+int qemu_open(const char *name, int flags, Error **errp)
+{
+    assert(!(flags & O_CREAT));
+
+    return qemu_open_internal(name, flags, 0, errp);
+}
+
+
+int qemu_create(const char *name, int flags, mode_t mode, Error **errp)
+{
+    assert(!(flags & O_CREAT));
+
+    return qemu_open_internal(name, flags | O_CREAT, mode, errp);
+}
+
+
 int qemu_open_old(const char *name, int flags, ...)
 {
     va_list ap;
-- 
2.26.2

[PATCH v4 5/6] util: give a specific error message when O_DIRECT doesn't work

[Daniel P. Berrangé]

A common error scenario is to tell QEMU to use O_DIRECT in combination
with a filesystem that doesn't support it. To aid users to diagnosing
their mistake we want to provide a clear error message when this happens.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 util/osdep.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/util/osdep.c b/util/osdep.c
index a4956fbf6b..6c24985f7a 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -345,6 +345,19 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
 
     if (ret == -1) {
         const char *action = flags & O_CREAT ? "create" : "open";
+#ifdef O_DIRECT
+        if (errno == EINVAL && (flags & O_DIRECT)) {
+            ret = open(name, flags & ~O_DIRECT, 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; /* close() clobbered earlier errno */
+                return -1;
+            }
+        }
+#endif /* O_DIRECT */
         error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
                          action, name, flags);
     }
-- 
2.26.2

[PATCH v4 6/6] block/fileb: switch to use qemu_open/qemu_create 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"
  }

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 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");
         return -EIO;
     }
     set_sparse(fd);
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index de4771bcb3..242dcfe08d 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 f707471fb0..4c41b46986 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 b2d2dfed04..a533b0d416 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -538,7 +538,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 126b4d2d51..ffb95c965f 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 backing_fmt=IMGFMT
-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 1728aff1e0..5a718ac1b5 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -544,10 +544,10 @@ Supported options:
   size=<size>            - Virtual disk size
 
 Testing: convert -O qcow2 -o backing_fmt=qcow2,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_fmt=qcow2,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 ba034e5c58..90255ea653 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.
 *** 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 v4 2/6] util: refactor qemu_open_old to split off variadic args handling

[Markus Armbruster]

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

> This simple refactoring prepares for future patches. The variadic args
> handling is split from the main bulk of the open logic. The duplicated
> calls to open() are removed in favour of updating the "flags" variable
> to have O_CLOEXEC.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  util/osdep.c | 40 +++++++++++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/util/osdep.c b/util/osdep.c
> index 9df1b6adec..9ff92551e7 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,10 +283,10 @@ 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_old(const char *name, int flags, ...)
> +static int
> +qemu_open_internal(const char *name, int flags, mode_t mode)
>  {
>      int ret;
> -    int mode = 0;
>  
>  #ifndef _WIN32
>      const char *fdset_id_str;
> @@ -323,22 +324,35 @@ int qemu_open_old(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
> -    ret = open(name, flags | O_CLOEXEC, mode);
> -#else
> +    flags |= O_CLOEXEC;
> +#endif /* O_CLOEXEC */
> +
>      ret = open(name, flags, mode);
> +
> +#ifndef O_CLOEXEC
>      if (ret >= 0) {
>          qemu_set_cloexec(ret);
>      }
> -#endif
> +#endif /* ! O_CLOEXEC */

I dislike this part, to be honest.  I find

    #ifdef O_CLOEXEC
        flags |= O_CLOEXEC;
    #endif /* O_CLOEXEC */

        ret = open(name, flags, mode);

    #ifndef O_CLOEXEC
        if (ret >= 0) {
            qemu_set_cloexec(ret);
        }
    #endif /* ! O_CLOEXEC */

harder to read than

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

> +
> +    return ret;
> +}
> +
> +
> +int qemu_open_old(const char *name, int flags, ...)
> +{
> +    va_list ap;
> +    mode_t mode = 0;
> +    int ret;
> +
> +    va_start(ap, flags);
> +    if (flags & O_CREAT) {
> +        mode = va_arg(ap, int);
> +    }
> +    va_end(ap);
> +
> +    ret = qemu_open_internal(name, flags, mode);
>  
>  #ifdef O_DIRECT
>      if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {

Re: [PATCH v4 2/6] util: refactor qemu_open_old to split off variadic args handling

[Daniel P. Berrangé]

On Tue, Aug 25, 2020 at 04:56:40PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > This simple refactoring prepares for future patches. The variadic args
> > handling is split from the main bulk of the open logic. The duplicated
> > calls to open() are removed in favour of updating the "flags" variable
> > to have O_CLOEXEC.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  util/osdep.c | 40 +++++++++++++++++++++++++++-------------
> >  1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/util/osdep.c b/util/osdep.c
> > index 9df1b6adec..9ff92551e7 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,10 +283,10 @@ 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_old(const char *name, int flags, ...)
> > +static int
> > +qemu_open_internal(const char *name, int flags, mode_t mode)
> >  {
> >      int ret;
> > -    int mode = 0;
> >  
> >  #ifndef _WIN32
> >      const char *fdset_id_str;
> > @@ -323,22 +324,35 @@ int qemu_open_old(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
> > -    ret = open(name, flags | O_CLOEXEC, mode);
> > -#else
> > +    flags |= O_CLOEXEC;
> > +#endif /* O_CLOEXEC */
> > +
> >      ret = open(name, flags, mode);
> > +
> > +#ifndef O_CLOEXEC
> >      if (ret >= 0) {
> >          qemu_set_cloexec(ret);
> >      }
> > -#endif
> > +#endif /* ! O_CLOEXEC */
> 
> I dislike this part, to be honest.  I find
> 
>     #ifdef O_CLOEXEC
>         flags |= O_CLOEXEC;
>     #endif /* O_CLOEXEC */
> 
>         ret = open(name, flags, mode);
> 
>     #ifndef O_CLOEXEC
>         if (ret >= 0) {
>             qemu_set_cloexec(ret);
>         }
>     #endif /* ! O_CLOEXEC */
> 
> harder to read than
> 
>     #ifdef O_CLOEXEC
>         ret = open(name, flags | O_CLOEXEC, mode);
>     #else
>         ret = open(name, flags, mode);
>         if (ret >= 0) {
>             qemu_set_cloexec(ret);
>         }
>     #endif

We're re-using 'flags' variable again in a later patch and want to
have O_CLOEXEC present in it then too.

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 v4 3/6] util: add Error object for qemu_open_internal error reporting

[Markus Armbruster]

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

> Instead of relying on the limited information from errno, we can now
> also provide detailed error messages.

The more detailed error messages are currently always ignored, but the
next patches will fix that.

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  util/osdep.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/util/osdep.c b/util/osdep.c
> index 9ff92551e7..9c7118d3cb 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -284,7 +284,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
>   * Opens a file with FD_CLOEXEC set
>   */
>  static int
> -qemu_open_internal(const char *name, int flags, mode_t mode)
> +qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
>  {
>      int ret;
>  
> @@ -298,24 +298,31 @@ qemu_open_internal(const char *name, int flags, mode_t mode)
>  
>          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);

Can this happen?

>              errno = EINVAL;
>              return -1;
>          }
> @@ -336,6 +343,16 @@ qemu_open_internal(const char *name, int flags, mode_t mode)
>      }
>  #endif /* ! O_CLOEXEC */
>  
> +    if (ret == -1) {
> +        const char *action = "open";
> +        if (flags & O_CREAT) {
> +            action = "create";
> +        }
> +        error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
> +                         action, name, flags);

Not a good user experience:

    Could not open '/etc/shadow' flags 0x0: Permission denied

Better:

    Could not open '/etc/shadow' for reading: Permission denied

Are you sure flags other than the access mode (O_RDONLY, O_WRONLY,
O_RDWR) must be included in the error message?

If you must report flags in hexadecimal, then please reporting them more
consistently.  Right now you have

    for %s flags 0x%x
    '%s' flags %x

Perhaps '%s' with flags 0x%x

> +    }
> +
> +
>      return ret;
>  }
>  
> @@ -352,7 +369,7 @@ int qemu_open_old(const char *name, int flags, ...)
>      }
>      va_end(ap);
>  
> -    ret = qemu_open_internal(name, flags, mode);
> +    ret = qemu_open_internal(name, flags, mode, NULL);
>  
>  #ifdef O_DIRECT
>      if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {

Re: [PATCH v4 4/6] util: introduce qemu_open and qemu_create with error reporting

[Markus Armbruster]

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

> qemu_open_old() works like open(): set errno and return -1 on failure.
> It has even more failure modes, though.  Reporting the error clearly
> to users is basically impossible for many of them.
>
> Our standard cure for "errno is too coarse" is the Error object.
> Introduce two new helper methods:
>
>   int qemu_open(const char *name, int flags, Error **errp);
>   int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
>
> Note that with this design we no longer require or even accept the
> O_CREAT flag. Avoiding overloading the two distinct operations
> means we can avoid variable arguments which would prevent 'errp' from
> being the last argument. It also gives us a guarantee that the 'mode' is
> given when creating files, avoiding a latent security bug.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/qemu/osdep.h |  6 ++++++
>  util/osdep.c         | 21 +++++++++++++++++----
>  2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 18333e9006..13a821845b 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -497,7 +497,13 @@ 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);
>  
> +/*
> + * Don't introduce new usage of this function, prefer the following
> + * qemu_open/qemu_create that take an "Error **errp"
> + */
>  int qemu_open_old(const char *name, int flags, ...);
> +int qemu_open(const char *name, int flags, Error **errp);
> +int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
>  int qemu_close(int fd);
>  int qemu_unlink(const char *name);
>  #ifndef _WIN32
> diff --git a/util/osdep.c b/util/osdep.c
> index 9c7118d3cb..a4956fbf6b 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -344,10 +344,7 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
>  #endif /* ! O_CLOEXEC */
>  
>      if (ret == -1) {
> -        const char *action = "open";
> -        if (flags & O_CREAT) {
> -            action = "create";
> -        }
> +        const char *action = flags & O_CREAT ? "create" : "open";
>          error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
>                           action, name, flags);
>      }

Squash this hunk into PATCH 3, please.

> @@ -357,6 +354,22 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
>  }
>  
>  
> +int qemu_open(const char *name, int flags, Error **errp)
> +{
> +    assert(!(flags & O_CREAT));
> +
> +    return qemu_open_internal(name, flags, 0, errp);
> +}
> +
> +
> +int qemu_create(const char *name, int flags, mode_t mode, Error **errp)
> +{
> +    assert(!(flags & O_CREAT));
> +
> +    return qemu_open_internal(name, flags | O_CREAT, mode, errp);
> +}
> +
> +
>  int qemu_open_old(const char *name, int flags, ...)
>  {
>      va_list ap;

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

Re: [PATCH v4 5/6] util: give a specific error message when O_DIRECT doesn't work

[Markus Armbruster]

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

> A common error scenario is to tell QEMU to use O_DIRECT in combination
> with a filesystem that doesn't support it. To aid users to diagnosing
> their mistake we want to provide a clear error message when this happens.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  util/osdep.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/util/osdep.c b/util/osdep.c
> index a4956fbf6b..6c24985f7a 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -345,6 +345,19 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
>  
>      if (ret == -1) {
>          const char *action = flags & O_CREAT ? "create" : "open";
> +#ifdef O_DIRECT
> +        if (errno == EINVAL && (flags & O_DIRECT)) {
> +            ret = open(name, flags & ~O_DIRECT, 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; /* close() clobbered earlier errno */

More precise: close() may have clobbered

Sure open() can only fail with EINVAL here?

> +                return -1;
> +            }
> +        }
> +#endif /* O_DIRECT */
>          error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
>                           action, name, flags);
>      }

There is no qemu_set_cloexec().  Intentional?

Re: [PATCH v4 5/6] util: give a specific error message when O_DIRECT doesn't work

[Daniel P. Berrangé]

On Tue, Aug 25, 2020 at 05:19:53PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > A common error scenario is to tell QEMU to use O_DIRECT in combination
> > with a filesystem that doesn't support it. To aid users to diagnosing
> > their mistake we want to provide a clear error message when this happens.
> >
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  util/osdep.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/util/osdep.c b/util/osdep.c
> > index a4956fbf6b..6c24985f7a 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -345,6 +345,19 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
> >  
> >      if (ret == -1) {
> >          const char *action = flags & O_CREAT ? "create" : "open";
> > +#ifdef O_DIRECT
> > +        if (errno == EINVAL && (flags & O_DIRECT)) {
> > +            ret = open(name, flags & ~O_DIRECT, 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; /* close() clobbered earlier errno */
> 
> More precise: close() may have clobbered

Either open or close in fact.

> 
> Sure open() can only fail with EINVAL here?

We don't care about the errno from the open() call seen in this context,
we're restoring the EINVAL from the previous open() call above this patch
contxt, that we match on with  "if (errno == EINVAL && ...)" line.

> 
> > +                return -1;
> > +            }
> > +        }
> > +#endif /* O_DIRECT */
> >          error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
> >                           action, name, flags);
> >      }
> 
> There is no qemu_set_cloexec().  Intentional?

We've called close() immediately after open(). Adding qemu_set_cloexec()
doesnt make it any less racy on platforms lacking O_CLOSEXEC

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 v4 6/6] block/fileb: switch to use qemu_open/qemu_create 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"
>   }
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Both commit message and expected iotest results demonstrate the less
than helpful flags error reporting I pointed out in my review of PATCH
3.

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

Re: [PATCH v4 3/6] util: add Error object for qemu_open_internal error reporting

[Daniel P. Berrangé]

On Tue, Aug 25, 2020 at 05:14:21PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > Instead of relying on the limited information from errno, we can now
> > also provide detailed error messages.
> 
> The more detailed error messages are currently always ignored, but the
> next patches will fix that.
> 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  util/osdep.c | 21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/util/osdep.c b/util/osdep.c
> > index 9ff92551e7..9c7118d3cb 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -284,7 +284,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
> >   * Opens a file with FD_CLOEXEC set
> >   */
> >  static int
> > -qemu_open_internal(const char *name, int flags, mode_t mode)
> > +qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
> >  {
> >      int ret;
> >  
> > @@ -298,24 +298,31 @@ qemu_open_internal(const char *name, int flags, mode_t mode)
> >  
> >          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);
> 
> Can this happen?

Well there's code in monitor_fdset_dup_fd_add that can return -1.

> 
> >              errno = EINVAL;
> >              return -1;
> >          }
> > @@ -336,6 +343,16 @@ qemu_open_internal(const char *name, int flags, mode_t mode)
> >      }
> >  #endif /* ! O_CLOEXEC */
> >  
> > +    if (ret == -1) {
> > +        const char *action = "open";
> > +        if (flags & O_CREAT) {
> > +            action = "create";
> > +        }
> > +        error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
> > +                         action, name, flags);
> 
> Not a good user experience:
> 
>     Could not open '/etc/shadow' flags 0x0: Permission denied
> 
> Better:
> 
>     Could not open '/etc/shadow' for reading: Permission denied
> 
> Are you sure flags other than the access mode (O_RDONLY, O_WRONLY,
> O_RDWR) must be included in the error message?

It was the flags other than access mode that I was thinking were
more important to log. I'm ambivalent htough, so can drop the
flags if it is thought to be overkill.

> 
> If you must report flags in hexadecimal, then please reporting them more
> consistently.  Right now you have
> 
>     for %s flags 0x%x
>     '%s' flags %x
> 
> Perhaps '%s' with flags 0x%x
> 
> > +    }
> > +
> > +
> >      return ret;
> >  }
> >  
> > @@ -352,7 +369,7 @@ int qemu_open_old(const char *name, int flags, ...)
> >      }
> >      va_end(ap);
> >  
> > -    ret = qemu_open_internal(name, flags, mode);
> > +    ret = qemu_open_internal(name, flags, mode, NULL);
> >  
> >  #ifdef O_DIRECT
> >      if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
> 
> 

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 v4 3/6] util: add Error object for qemu_open_internal error reporting

[Markus Armbruster]

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

> On Tue, Aug 25, 2020 at 05:14:21PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > Instead of relying on the limited information from errno, we can now
>> > also provide detailed error messages.
>> 
>> The more detailed error messages are currently always ignored, but the
>> next patches will fix that.
>> 
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> >  util/osdep.c | 21 +++++++++++++++++++--
>> >  1 file changed, 19 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/util/osdep.c b/util/osdep.c
>> > index 9ff92551e7..9c7118d3cb 100644
>> > --- a/util/osdep.c
>> > +++ b/util/osdep.c
>> > @@ -284,7 +284,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
>> >   * Opens a file with FD_CLOEXEC set
>> >   */
>> >  static int
>> > -qemu_open_internal(const char *name, int flags, mode_t mode)
>> > +qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
>> >  {
>> >      int ret;
>> >  
>> > @@ -298,24 +298,31 @@ qemu_open_internal(const char *name, int flags, mode_t mode)
>> >  
>> >          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);
>> 
>> Can this happen?
>
> Well there's code in monitor_fdset_dup_fd_add that can return -1.

It fails when

* @fdset_id contains @dupfd

  @dupfd is a fresh file descriptor.  If @fdset_id already contains it,
  it's stale there.  That would be a programming error.  Recommend to
  assert.

* @fdset_id is not in @mon_fdsets

  monitor_fdset_get_fd() fails the same way.  monitor_fdset_dup_fd_add()
  can fail that way after monitor_fdset_get_fd() succeed only if the fd
  set went away between the two.  Could that happen?  Would it be safe?

  This is the only user of monitor_fdset_dup_fd_add().  Why not remove
  the awkward failure mode by making monitor_fdset_dup_fd_add() dup the
  fd and add?

>> >              errno = EINVAL;
>> >              return -1;
>> >          }
>> > @@ -336,6 +343,16 @@ qemu_open_internal(const char *name, int flags, mode_t mode)
>> >      }
>> >  #endif /* ! O_CLOEXEC */
>> >  
>> > +    if (ret == -1) {
>> > +        const char *action = "open";
>> > +        if (flags & O_CREAT) {
>> > +            action = "create";
>> > +        }
>> > +        error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
>> > +                         action, name, flags);
>> 
>> Not a good user experience:
>> 
>>     Could not open '/etc/shadow' flags 0x0: Permission denied
>> 
>> Better:
>> 
>>     Could not open '/etc/shadow' for reading: Permission denied
>> 
>> Are you sure flags other than the access mode (O_RDONLY, O_WRONLY,
>> O_RDWR) must be included in the error message?
>
> It was the flags other than access mode that I was thinking were
> more important to log. I'm ambivalent htough, so can drop the
> flags if it is thought to be overkill.

Hexadecimal flags are borderline useless even for developers: to make
sense of them, you have to grep -R /usr/include/.  For mere mortals,
they are confusing in addition to useless.

>> If you must report flags in hexadecimal, then please reporting them more
>> consistently.  Right now you have
>> 
>>     for %s flags 0x%x
>>     '%s' flags %x
>> 
>> Perhaps '%s' with flags 0x%x
>> 
>> > +    }
>> > +
>> > +
>> >      return ret;
>> >  }
>> >  
>> > @@ -352,7 +369,7 @@ int qemu_open_old(const char *name, int flags, ...)
>> >      }
>> >      va_end(ap);
>> >  
>> > -    ret = qemu_open_internal(name, flags, mode);
>> > +    ret = qemu_open_internal(name, flags, mode, NULL);
>> >  
>> >  #ifdef O_DIRECT
>> >      if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
>> 
>> 
>
> Regards,
> Daniel

Re: [PATCH v4 2/6] util: refactor qemu_open_old to split off variadic args handling

[Markus Armbruster]

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

> On Tue, Aug 25, 2020 at 04:56:40PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > This simple refactoring prepares for future patches. The variadic args
>> > handling is split from the main bulk of the open logic. The duplicated
>> > calls to open() are removed in favour of updating the "flags" variable
>> > to have O_CLOEXEC.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> >  util/osdep.c | 40 +++++++++++++++++++++++++++-------------
>> >  1 file changed, 27 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/util/osdep.c b/util/osdep.c
>> > index 9df1b6adec..9ff92551e7 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,10 +283,10 @@ 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_old(const char *name, int flags, ...)
>> > +static int
>> > +qemu_open_internal(const char *name, int flags, mode_t mode)
>> >  {
>> >      int ret;
>> > -    int mode = 0;
>> >  
>> >  #ifndef _WIN32
>> >      const char *fdset_id_str;
>> > @@ -323,22 +324,35 @@ int qemu_open_old(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
>> > -    ret = open(name, flags | O_CLOEXEC, mode);
>> > -#else
>> > +    flags |= O_CLOEXEC;
>> > +#endif /* O_CLOEXEC */
>> > +
>> >      ret = open(name, flags, mode);
>> > +
>> > +#ifndef O_CLOEXEC
>> >      if (ret >= 0) {
>> >          qemu_set_cloexec(ret);
>> >      }
>> > -#endif
>> > +#endif /* ! O_CLOEXEC */
>> 
>> I dislike this part, to be honest.  I find
>> 
>>     #ifdef O_CLOEXEC
>>         flags |= O_CLOEXEC;
>>     #endif /* O_CLOEXEC */
>> 
>>         ret = open(name, flags, mode);
>> 
>>     #ifndef O_CLOEXEC
>>         if (ret >= 0) {
>>             qemu_set_cloexec(ret);
>>         }
>>     #endif /* ! O_CLOEXEC */
>> 
>> harder to read than
>> 
>>     #ifdef O_CLOEXEC
>>         ret = open(name, flags | O_CLOEXEC, mode);
>>     #else
>>         ret = open(name, flags, mode);
>>         if (ret >= 0) {
>>             qemu_set_cloexec(ret);
>>         }
>>     #endif
>
> We're re-using 'flags' variable again in a later patch and want to
> have O_CLOEXEC present in it then too.

I guess you mean PATCH 5.  I'll get back the the issue there.

Re: [PATCH v4 5/6] util: give a specific error message when O_DIRECT doesn't work

[Markus Armbruster]

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

> On Tue, Aug 25, 2020 at 05:19:53PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > A common error scenario is to tell QEMU to use O_DIRECT in combination
>> > with a filesystem that doesn't support it. To aid users to diagnosing
>> > their mistake we want to provide a clear error message when this happens.
>> >
>> > Reviewed-by: Eric Blake <eblake@redhat.com>
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> >  util/osdep.c | 13 +++++++++++++
>> >  1 file changed, 13 insertions(+)
>> >
>> > diff --git a/util/osdep.c b/util/osdep.c
>> > index a4956fbf6b..6c24985f7a 100644
>> > --- a/util/osdep.c
>> > +++ b/util/osdep.c
>> > @@ -345,6 +345,19 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
>> >  
>> >      if (ret == -1) {
>> >          const char *action = flags & O_CREAT ? "create" : "open";
>> > +#ifdef O_DIRECT
>> > +        if (errno == EINVAL && (flags & O_DIRECT)) {

Suggest

                  /*
                   * Check whether open() failed due to use of O_DIRECT,
                   * and set a more helpful error then.
                   */

>> > +            ret = open(name, flags & ~O_DIRECT, 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; /* close() clobbered earlier errno */
>> 
>> More precise: close() may have clobbered
>
> Either open or close in fact.
>
>> 
>> Sure open() can only fail with EINVAL here?
>
> We don't care about the errno from the open() call seen in this context,
> we're restoring the EINVAL from the previous open() call above this patch
> contxt, that we match on with  "if (errno == EINVAL && ...)" line.

Ah, got it now.

I'd prefer

                errno = EINVAL; /* restore first open()'s errno */
>> 
>> > +                return -1;
>> > +            }
>> > +        }
>> > +#endif /* O_DIRECT */
>> >          error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
>> >                           action, name, flags);
>> >      }
>> 
>> There is no qemu_set_cloexec().  Intentional?
>
> We've called close() immediately after open(). Adding qemu_set_cloexec()
> doesnt make it any less racy on platforms lacking O_CLOSEXEC

You're right.  I misread the code.

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


Now back to my dislike of the #ifdeffery I voiced in reply to PATCH 2.

Code now:

    #ifdef O_CLOEXEC
        flags |= O_CLOEXEC;
    #endif /* O_CLOEXEC */

        ret = open(name, flags, mode);

    #ifndef O_CLOEXEC
        if (ret >= 0) {
            qemu_set_cloexec(ret);
        }
    #endif /* ! O_CLOEXEC */

        if (ret == -1) {
            const char *action = flags & O_CREAT ? "create" : "open";
    #ifdef O_DIRECT
            if (errno == EINVAL && (flags & O_DIRECT)) {
                ret = open(name, flags & ~O_DIRECT, mode);
                if (ret != -1) {
                    close(ret);
                    [O_DIRECT not supported error...]
                    errno = EINVAL; /* close() clobbered earlier errno */
                    return -1;
                }
            }
    #endif /* O_DIRECT */
            [generic error...]
        }

Compare:

    #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 /* ! O_CLOEXEC */

        if (ret == -1) {
            const char *action = flags & O_CREAT ? "create" : "open";
    #ifdef O_DIRECT
            if (errno == EINVAL && (flags & O_DIRECT)) {
                ret = open(name, flags & ~O_DIRECT, mode);
                if (ret != -1) {
                    close(ret);
                    [O_DIRECT not supported error...]
                    errno = EINVAL; /* close() clobbered earlier errno */
                    return -1;
                }
            }
    #endif /* O_DIRECT */
            [generic error...]
        }

I like this a bit better, but not enough to make a strong
recommendation, let alone demand.

Re: [PATCH v4 3/6] util: add Error object for qemu_open_internal error reporting

[Daniel P. Berrangé]

On Wed, Aug 26, 2020 at 01:03:19PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Aug 25, 2020 at 05:14:21PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > Instead of relying on the limited information from errno, we can now
> >> > also provide detailed error messages.
> >> 
> >> The more detailed error messages are currently always ignored, but the
> >> next patches will fix that.
> >> 
> >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >> > ---
> >> >  util/osdep.c | 21 +++++++++++++++++++--
> >> >  1 file changed, 19 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/util/osdep.c b/util/osdep.c
> >> > index 9ff92551e7..9c7118d3cb 100644
> >> > --- a/util/osdep.c
> >> > +++ b/util/osdep.c
> >> > @@ -284,7 +284,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
> >> >   * Opens a file with FD_CLOEXEC set
> >> >   */
> >> >  static int
> >> > -qemu_open_internal(const char *name, int flags, mode_t mode)
> >> > +qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
> >> >  {
> >> >      int ret;
> >> >  
> >> > @@ -298,24 +298,31 @@ qemu_open_internal(const char *name, int flags, mode_t mode)
> >> >  
> >> >          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);
> >> 
> >> Can this happen?
> >
> > Well there's code in monitor_fdset_dup_fd_add that can return -1.
> 
> It fails when
> 
> * @fdset_id contains @dupfd
> 
>   @dupfd is a fresh file descriptor.  If @fdset_id already contains it,
>   it's stale there.  That would be a programming error.  Recommend to
>   assert.
> 
> * @fdset_id is not in @mon_fdsets
> 
>   monitor_fdset_get_fd() fails the same way.  monitor_fdset_dup_fd_add()
>   can fail that way after monitor_fdset_get_fd() succeed only if the fd
>   set went away between the two.  Could that happen?  Would it be safe?
> 
>   This is the only user of monitor_fdset_dup_fd_add().  Why not remove
>   the awkward failure mode by making monitor_fdset_dup_fd_add() dup the
>   fd and add?

Once we push  the qemu_dup call into monitor_fdset_dup_fd_add, we
might as well go the whole way and merge monitor_fdset_get_fd
into it too. So I've done that, turning 3 calls into 1.

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 v4 5/6] util: give a specific error message when O_DIRECT doesn't work

[Daniel P. Berrangé]

On Wed, Aug 26, 2020 at 01:19:53PM +0200, Markus Armbruster wrote:

> Now back to my dislike of the #ifdeffery I voiced in reply to PATCH 2.
> 
> Code now:
> 
>     #ifdef O_CLOEXEC
>         flags |= O_CLOEXEC;
>     #endif /* O_CLOEXEC */
> 
>         ret = open(name, flags, mode);
> 
>     #ifndef O_CLOEXEC
>         if (ret >= 0) {
>             qemu_set_cloexec(ret);
>         }
>     #endif /* ! O_CLOEXEC */
> 
>         if (ret == -1) {
>             const char *action = flags & O_CREAT ? "create" : "open";
>     #ifdef O_DIRECT
>             if (errno == EINVAL && (flags & O_DIRECT)) {
>                 ret = open(name, flags & ~O_DIRECT, mode);
>                 if (ret != -1) {
>                     close(ret);
>                     [O_DIRECT not supported error...]
>                     errno = EINVAL; /* close() clobbered earlier errno */
>                     return -1;
>                 }
>             }
>     #endif /* O_DIRECT */
>             [generic error...]
>         }
> 
> Compare:
> 
>     #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 /* ! O_CLOEXEC */
> 
>         if (ret == -1) {
>             const char *action = flags & O_CREAT ? "create" : "open";
>     #ifdef O_DIRECT
>             if (errno == EINVAL && (flags & O_DIRECT)) {
>                 ret = open(name, flags & ~O_DIRECT, mode);
>                 if (ret != -1) {
>                     close(ret);
>                     [O_DIRECT not supported error...]
>                     errno = EINVAL; /* close() clobbered earlier errno */
>                     return -1;
>                 }
>             }
>     #endif /* O_DIRECT */
>             [generic error...]
>         }
> 
> I like this a bit better, but not enough to make a strong
> recommendation, let alone demand.

In v5 I've gone with neither approach, and instead spun off a helper
method qemu_open_cloexec which I think is clearer than both.

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 :|