[PATCH v5 0/8] block: improve error reporting for unsupported O_DIRECT

[PATCH v5 0/8] 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
v4: https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg05334.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 v5:

 - Drop reporting of flags in failed open call
 - Split O_CLOEXEC handling off into separate helper
 - Refactor monitor FD set APIs to simplify their use

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é (8):
  monitor: simplify functions for getting a dup'd fdset entry
  util: split off a helper for dealing with O_CLOEXEC flag
  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/file: 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/monitor/monitor.h      |   3 +-
 include/qemu/osdep.h           |   9 ++-
 io/channel-file.c              |   2 +-
 monitor/misc.c                 |  58 ++++++++----------
 net/vhost-vdpa.c               |   2 +-
 os-posix.c                     |   2 +-
 qga/channel-posix.c            |   4 +-
 qga/commands-posix.c           |   6 +-
 stubs/fdset.c                  |   8 +--
 target/arm/kvm.c               |   2 +-
 ui/console.c                   |   2 +-
 util/osdep.c                   | 104 +++++++++++++++++++++++----------
 util/oslib-posix.c             |   2 +-
 26 files changed, 148 insertions(+), 114 deletions(-)

-- 
2.26.2

[PATCH v5 1/8] monitor: simplify functions for getting a dup'd fdset entry

[Daniel P. Berrangé]

Currently code has to call monitor_fdset_get_fd, then dup
the return fd, and then add the duplicate FD back into the
fdset. This dance is overly verbose for the caller and
introduces extra failure modes which can be avoided by
folding all the logic into monitor_fdset_dup_fd_add and
removing monitor_fdset_get_fd entirely.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/monitor/monitor.h |  3 +-
 include/qemu/osdep.h      |  1 +
 monitor/misc.c            | 58 +++++++++++++++++----------------------
 stubs/fdset.c             |  8 ++----
 util/osdep.c              | 19 ++-----------
 5 files changed, 32 insertions(+), 57 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 1018d754a6..c0170773d4 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -43,8 +43,7 @@ int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func,
 AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
                                 bool has_opaque, const char *opaque,
                                 Error **errp);
-int monitor_fdset_get_fd(int64_t fdset_id, int flags);
-int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
+int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags);
 void monitor_fdset_dup_fd_remove(int dup_fd);
 int64_t monitor_fdset_dup_fd_find(int dup_fd);
 
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 412962d91a..66ee5bc45d 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -501,6 +501,7 @@ int qemu_open(const char *name, int flags, ...);
 int qemu_close(int fd);
 int qemu_unlink(const char *name);
 #ifndef _WIN32
+int qemu_dup_flags(int fd, int flags);
 int qemu_dup(int fd);
 #endif
 int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
diff --git a/monitor/misc.c b/monitor/misc.c
index e847b58a8c..98e389e4a8 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -1547,69 +1547,61 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
     return fdinfo;
 }
 
-int monitor_fdset_get_fd(int64_t fdset_id, int flags)
+
+int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags)
 {
 #ifdef _WIN32
     return -ENOENT;
 #else
     MonFdset *mon_fdset;
-    MonFdsetFd *mon_fdset_fd;
-    int mon_fd_flags;
-    int ret;
 
     qemu_mutex_lock(&mon_fdsets_lock);
     QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
+        MonFdsetFd *mon_fdset_fd;
+        MonFdsetFd *mon_fdset_fd_dup;
+        int fd = -1;
+        int dup_fd;
+        int mon_fd_flags;
+
         if (mon_fdset->id != fdset_id) {
             continue;
         }
+
         QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
             mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL);
             if (mon_fd_flags == -1) {
-                ret = -errno;
-                goto out;
+                qemu_mutex_unlock(&mon_fdsets_lock);
+                return -1;
             }
 
             if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
-                ret = mon_fdset_fd->fd;
-                goto out;
+                fd = mon_fdset_fd->fd;
+                break;
             }
         }
-        ret = -EACCES;
-        goto out;
-    }
-    ret = -ENOENT;
 
-out:
-    qemu_mutex_unlock(&mon_fdsets_lock);
-    return ret;
-#endif
-}
-
-int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
-{
-    MonFdset *mon_fdset;
-    MonFdsetFd *mon_fdset_fd_dup;
-
-    qemu_mutex_lock(&mon_fdsets_lock);
-    QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
-        if (mon_fdset->id != fdset_id) {
-            continue;
+        if (fd == -1) {
+            errno = EINVAL;
+            return -1;
         }
-        QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
-            if (mon_fdset_fd_dup->fd == dup_fd) {
-                goto err;
-            }
+
+        dup_fd = qemu_dup_flags(fd, flags);
+        if (dup_fd == -1) {
+            qemu_mutex_unlock(&mon_fdsets_lock);
+            return -1;
         }
+
         mon_fdset_fd_dup = g_malloc0(sizeof(*mon_fdset_fd_dup));
         mon_fdset_fd_dup->fd = dup_fd;
         QLIST_INSERT_HEAD(&mon_fdset->dup_fds, mon_fdset_fd_dup, next);
         qemu_mutex_unlock(&mon_fdsets_lock);
-        return 0;
+        return dup_fd;
     }
 
-err:
     qemu_mutex_unlock(&mon_fdsets_lock);
+    errno = ENOENT;
     return -1;
+#endif
 }
 
 static int64_t monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
diff --git a/stubs/fdset.c b/stubs/fdset.c
index 67dd5e1d34..56b3663d58 100644
--- a/stubs/fdset.c
+++ b/stubs/fdset.c
@@ -1,8 +1,9 @@
 #include "qemu/osdep.h"
 #include "monitor/monitor.h"
 
-int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
+int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags)
 {
+    errno = ENOSYS;
     return -1;
 }
 
@@ -11,11 +12,6 @@ int64_t monitor_fdset_dup_fd_find(int dup_fd)
     return -1;
 }
 
-int monitor_fdset_get_fd(int64_t fdset_id, int flags)
-{
-    return -ENOENT;
-}
-
 void monitor_fdset_dup_fd_remove(int dupfd)
 {
 }
diff --git a/util/osdep.c b/util/osdep.c
index 4829c07ff6..3d94b4d732 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -122,7 +122,7 @@ static int fcntl_op_getlk = -1;
 /*
  * Dups an fd and sets the flags
  */
-static int qemu_dup_flags(int fd, int flags)
+int qemu_dup_flags(int fd, int flags)
 {
     int ret;
     int serrno;
@@ -293,7 +293,7 @@ int qemu_open(const char *name, int flags, ...)
     /* Attempt dup of fd from fd set */
     if (strstart(name, "/dev/fdset/", &fdset_id_str)) {
         int64_t fdset_id;
-        int fd, dupfd;
+        int dupfd;
 
         fdset_id = qemu_parse_fdset(fdset_id_str);
         if (fdset_id == -1) {
@@ -301,24 +301,11 @@ int qemu_open(const char *name, int flags, ...)
             return -1;
         }
 
-        fd = monitor_fdset_get_fd(fdset_id, flags);
-        if (fd < 0) {
-            errno = -fd;
-            return -1;
-        }
-
-        dupfd = qemu_dup_flags(fd, flags);
+        dupfd = monitor_fdset_dup_fd_add(fdset_id, flags);
         if (dupfd == -1) {
             return -1;
         }
 
-        ret = monitor_fdset_dup_fd_add(fdset_id, dupfd);
-        if (ret == -1) {
-            close(dupfd);
-            errno = EINVAL;
-            return -1;
-        }
-
         return dupfd;
     }
 #endif
-- 
2.26.2

[PATCH v5 2/8] util: split off a helper for dealing with O_CLOEXEC flag

[Daniel P. Berrangé]

We're going to have multiple callers to open() from qemu_open()
soon. Readability would thus benefit from having a helper for
dealing with O_CLOEXEC.

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

diff --git a/util/osdep.c b/util/osdep.c
index 3d94b4d732..0d8fa9f016 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -279,6 +279,20 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
 }
 #endif
 
+static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
+{
+    int ret;
+#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;
+}
+
 /*
  * Opens a file with FD_CLOEXEC set
  */
@@ -318,14 +332,7 @@ int qemu_open(const char *name, int flags, ...)
         va_end(ap);
     }
 
-#ifdef O_CLOEXEC
-    ret = open(name, flags | O_CLOEXEC, mode);
-#else
-    ret = open(name, flags, mode);
-    if (ret >= 0) {
-        qemu_set_cloexec(ret);
-    }
-#endif
+    ret = qemu_open_cloexec(name, flags, mode);
 
 #ifdef O_DIRECT
     if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
-- 
2.26.2

[PATCH v5 3/8] 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 08604f787f..f2270c2bf7 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1146,7 +1146,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 66ee5bc45d..ae1234104c 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 bf98508b6d..0bfd8e2d3c 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 0d8fa9f016..7504c156e8 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -296,7 +296,7 @@ static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
 /*
  * 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 v5 4/8] 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 | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/util/osdep.c b/util/osdep.c
index 7504c156e8..dd34b58bb7 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. */
 
@@ -296,10 +297,10 @@ static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
 /*
  * 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;
@@ -324,15 +325,25 @@ int qemu_open_old(const char *name, int flags, ...)
     }
 #endif
 
-    if (flags & O_CREAT) {
-        va_list ap;
+    ret = qemu_open_cloexec(name, flags, mode);
+
+    return ret;
+}
+
 
-        va_start(ap, flags);
+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);
     }
+    va_end(ap);
 
-    ret = qemu_open_cloexec(name, flags, mode);
+    ret = qemu_open_internal(name, flags, mode);
 
 #ifdef O_DIRECT
     if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
-- 
2.26.2

[PATCH v5 5/8] 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 to callers that ask for it.

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

diff --git a/util/osdep.c b/util/osdep.c
index dd34b58bb7..28aa89adc9 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -298,7 +298,7 @@ static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
  * 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;
 
@@ -312,12 +312,15 @@ 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;
         }
 
         dupfd = monitor_fdset_dup_fd_add(fdset_id, flags);
         if (dupfd == -1) {
+            error_setg_errno(errp, errno, "Could not dup FD for %s flags %x",
+                             name, flags);
             return -1;
         }
 
@@ -327,6 +330,13 @@ qemu_open_internal(const char *name, int flags, mode_t mode)
 
     ret = qemu_open_cloexec(name, flags, mode);
 
+    if (ret == -1) {
+        const char *action = flags & O_CREAT ? "create" : "open";
+        error_setg_errno(errp, errno, "Could not %s '%s'",
+                         action, name);
+    }
+
+
     return ret;
 }
 
@@ -343,7 +353,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 v5 6/8] 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.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qemu/osdep.h |  6 ++++++
 util/osdep.c         | 16 ++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index ae1234104c..577d9e8315 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 28aa89adc9..c99f1e7db2 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -341,6 +341,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 v5 7/8] 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: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 util/osdep.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/util/osdep.c b/util/osdep.c
index c99f1e7db2..8ea7a807c1 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -332,11 +332,24 @@ 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
+        /* Give more helpful error message for 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': "
+                           "filesystem does not support O_DIRECT",
+                           action, name);
+                errno = EINVAL; /* restore first open()'s errno */
+                return -1;
+            }
+        }
+#endif /* O_DIRECT */
         error_setg_errno(errp, errno, "Could not %s '%s'",
                          action, name);
     }
 
-
     return ret;
 }
 
-- 
2.26.2

[PATCH v5 8/8] block/file: 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': 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': filesystem does not support O_DIRECT"
  }

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 block/file-posix.c | 18 +++++++-----------
 block/file-win32.c |  6 ++----
 2 files changed, 9 insertions(+), 15 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);
-- 
2.26.2

Re: [PATCH v5 1/8] monitor: simplify functions for getting a dup'd fdset entry

[Markus Armbruster]

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

> Currently code has to call monitor_fdset_get_fd, then dup
> the return fd, and then add the duplicate FD back into the
> fdset. This dance is overly verbose for the caller and
> introduces extra failure modes which can be avoided by
> folding all the logic into monitor_fdset_dup_fd_add and
> removing monitor_fdset_get_fd entirely.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/monitor/monitor.h |  3 +-
>  include/qemu/osdep.h      |  1 +
>  monitor/misc.c            | 58 +++++++++++++++++----------------------
>  stubs/fdset.c             |  8 ++----
>  util/osdep.c              | 19 ++-----------
>  5 files changed, 32 insertions(+), 57 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 1018d754a6..c0170773d4 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -43,8 +43,7 @@ int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func,
>  AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>                                  bool has_opaque, const char *opaque,
>                                  Error **errp);
> -int monitor_fdset_get_fd(int64_t fdset_id, int flags);
> -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
> +int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags);
>  void monitor_fdset_dup_fd_remove(int dup_fd);
>  int64_t monitor_fdset_dup_fd_find(int dup_fd);
>  
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 412962d91a..66ee5bc45d 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -501,6 +501,7 @@ int qemu_open(const char *name, int flags, ...);
>  int qemu_close(int fd);
>  int qemu_unlink(const char *name);
>  #ifndef _WIN32
> +int qemu_dup_flags(int fd, int flags);
>  int qemu_dup(int fd);
>  #endif
>  int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> diff --git a/monitor/misc.c b/monitor/misc.c
> index e847b58a8c..98e389e4a8 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -1547,69 +1547,61 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>      return fdinfo;
>  }
>  
> -int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> +

Extra blank line.

> +int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags)
>  {
>  #ifdef _WIN32
>      return -ENOENT;
>  #else
>      MonFdset *mon_fdset;
> -    MonFdsetFd *mon_fdset_fd;
> -    int mon_fd_flags;
> -    int ret;
>  
>      qemu_mutex_lock(&mon_fdsets_lock);
>      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> +        MonFdsetFd *mon_fdset_fd;
> +        MonFdsetFd *mon_fdset_fd_dup;
> +        int fd = -1;
> +        int dup_fd;
> +        int mon_fd_flags;
> +
>          if (mon_fdset->id != fdset_id) {
>              continue;
>          }
> +
>          QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
>              mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL);
>              if (mon_fd_flags == -1) {
> -                ret = -errno;
> -                goto out;
> +                qemu_mutex_unlock(&mon_fdsets_lock);
> +                return -1;
>              }
>  
>              if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
> -                ret = mon_fdset_fd->fd;
> -                goto out;
> +                fd = mon_fdset_fd->fd;
> +                break;
>              }
>          }
> -        ret = -EACCES;
> -        goto out;
> -    }
> -    ret = -ENOENT;
>  
> -out:
> -    qemu_mutex_unlock(&mon_fdsets_lock);
> -    return ret;
> -#endif
> -}
> -
> -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> -{
> -    MonFdset *mon_fdset;
> -    MonFdsetFd *mon_fdset_fd_dup;
> -
> -    qemu_mutex_lock(&mon_fdsets_lock);
> -    QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> -        if (mon_fdset->id != fdset_id) {
> -            continue;
> +        if (fd == -1) {
> +            errno = EINVAL;
> +            return -1;

Missing qemu_mutex_unlock().

>          }

Old monitor_fdset_get_fd() returns -ENOENT when @fdset_id does not
exist, and -EACCES when it doesn't contain a file descriptor matching
@flags.

The new code seems to use EINVAL for the latter case.  Intentional?

> -        QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
> -            if (mon_fdset_fd_dup->fd == dup_fd) {
> -                goto err;
> -            }
> +
> +        dup_fd = qemu_dup_flags(fd, flags);
> +        if (dup_fd == -1) {
> +            qemu_mutex_unlock(&mon_fdsets_lock);
> +            return -1;
>          }
> +
>          mon_fdset_fd_dup = g_malloc0(sizeof(*mon_fdset_fd_dup));
>          mon_fdset_fd_dup->fd = dup_fd;
>          QLIST_INSERT_HEAD(&mon_fdset->dup_fds, mon_fdset_fd_dup, next);
>          qemu_mutex_unlock(&mon_fdsets_lock);
> -        return 0;
> +        return dup_fd;
>      }
>  
> -err:
>      qemu_mutex_unlock(&mon_fdsets_lock);
> +    errno = ENOENT;
>      return -1;
> +#endif
>  }
>  
>  static int64_t monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
> diff --git a/stubs/fdset.c b/stubs/fdset.c
> index 67dd5e1d34..56b3663d58 100644
> --- a/stubs/fdset.c
> +++ b/stubs/fdset.c
> @@ -1,8 +1,9 @@
>  #include "qemu/osdep.h"
>  #include "monitor/monitor.h"
>  
> -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> +int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags)
>  {
> +    errno = ENOSYS;
>      return -1;
>  }
>  
> @@ -11,11 +12,6 @@ int64_t monitor_fdset_dup_fd_find(int dup_fd)
>      return -1;
>  }
>  
> -int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> -{
> -    return -ENOENT;
> -}
> -
>  void monitor_fdset_dup_fd_remove(int dupfd)
>  {
>  }
> diff --git a/util/osdep.c b/util/osdep.c
> index 4829c07ff6..3d94b4d732 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -122,7 +122,7 @@ static int fcntl_op_getlk = -1;
>  /*
>   * Dups an fd and sets the flags
>   */
> -static int qemu_dup_flags(int fd, int flags)
> +int qemu_dup_flags(int fd, int flags)
>  {
>      int ret;
>      int serrno;
> @@ -293,7 +293,7 @@ int qemu_open(const char *name, int flags, ...)
>      /* Attempt dup of fd from fd set */
>      if (strstart(name, "/dev/fdset/", &fdset_id_str)) {
>          int64_t fdset_id;
> -        int fd, dupfd;
> +        int dupfd;
>  
>          fdset_id = qemu_parse_fdset(fdset_id_str);
>          if (fdset_id == -1) {
> @@ -301,24 +301,11 @@ int qemu_open(const char *name, int flags, ...)
>              return -1;
>          }
>  
> -        fd = monitor_fdset_get_fd(fdset_id, flags);
> -        if (fd < 0) {
> -            errno = -fd;
> -            return -1;
> -        }
> -
> -        dupfd = qemu_dup_flags(fd, flags);
> +        dupfd = monitor_fdset_dup_fd_add(fdset_id, flags);
>          if (dupfd == -1) {
>              return -1;
>          }
>  
> -        ret = monitor_fdset_dup_fd_add(fdset_id, dupfd);
> -        if (ret == -1) {
> -            close(dupfd);
> -            errno = EINVAL;
> -            return -1;
> -        }
> -
>          return dupfd;
>      }
>  #endif

Clear improvement, thanks!

Re: [PATCH v5 2/8] util: split off a helper for dealing with O_CLOEXEC flag

[Markus Armbruster]

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

> We're going to have multiple callers to open() from qemu_open()
> soon. Readability would thus benefit from having a helper for
> dealing with O_CLOEXEC.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

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

Re: [PATCH v5 3/8] util: rename qemu_open() to qemu_open_old()

[Markus Armbruster]

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

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

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

Re: [PATCH v5 4/8] 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.

Drop the second sentence, it is no longer true in this revision.

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  util/osdep.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/util/osdep.c b/util/osdep.c
> index 7504c156e8..dd34b58bb7 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -22,6 +22,7 @@
>   * THE SOFTWARE.
>   */
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"

This patch doesn't use anything from qapi/error.h as far as I can tell.
Does the hunk belong to another patch?

>  
>  /* Needed early for CONFIG_BSD etc. */
>  
> @@ -296,10 +297,10 @@ static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
>  /*
>   * 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;
> @@ -324,15 +325,25 @@ int qemu_open_old(const char *name, int flags, ...)
>      }
>  #endif
>  
> -    if (flags & O_CREAT) {
> -        va_list ap;
> +    ret = qemu_open_cloexec(name, flags, mode);
> +
> +    return ret;
> +}
> +
>  
> -        va_start(ap, flags);
> +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);
>      }
> +    va_end(ap);
>  
> -    ret = qemu_open_cloexec(name, flags, mode);
> +    ret = qemu_open_internal(name, flags, mode);
>  
>  #ifdef O_DIRECT
>      if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {

With the minor inaccuracies tidied up:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v5 5/8] 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 to callers that ask for it.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  util/osdep.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/util/osdep.c b/util/osdep.c
> index dd34b58bb7..28aa89adc9 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -298,7 +298,7 @@ static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
>   * 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;
>  
> @@ -312,12 +312,15 @@ 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;
>          }
>  
>          dupfd = monitor_fdset_dup_fd_add(fdset_id, flags);
>          if (dupfd == -1) {
> +            error_setg_errno(errp, errno, "Could not dup FD for %s flags %x",
> +                             name, flags);

You kept the reporting of flags here.  Intentional?

>              return -1;
>          }
>  
> @@ -327,6 +330,13 @@ qemu_open_internal(const char *name, int flags, mode_t mode)
>  
>      ret = qemu_open_cloexec(name, flags, mode);
>  
> +    if (ret == -1) {
> +        const char *action = flags & O_CREAT ? "create" : "open";
> +        error_setg_errno(errp, errno, "Could not %s '%s'",
> +                         action, name);
> +    }
> +
> +
>      return ret;
>  }

Much neater.  Thanks!

>  
> @@ -343,7 +353,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)) {

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

Re: [PATCH v5 1/8] monitor: simplify functions for getting a dup'd fdset entry

[Daniel P. Berrangé]

On Thu, Sep 03, 2020 at 10:52:40AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > Currently code has to call monitor_fdset_get_fd, then dup
> > the return fd, and then add the duplicate FD back into the
> > fdset. This dance is overly verbose for the caller and
> > introduces extra failure modes which can be avoided by
> > folding all the logic into monitor_fdset_dup_fd_add and
> > removing monitor_fdset_get_fd entirely.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  include/monitor/monitor.h |  3 +-
> >  include/qemu/osdep.h      |  1 +
> >  monitor/misc.c            | 58 +++++++++++++++++----------------------
> >  stubs/fdset.c             |  8 ++----
> >  util/osdep.c              | 19 ++-----------
> >  5 files changed, 32 insertions(+), 57 deletions(-)
> >

> > -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> > -{
> > -    MonFdset *mon_fdset;
> > -    MonFdsetFd *mon_fdset_fd_dup;
> > -
> > -    qemu_mutex_lock(&mon_fdsets_lock);
> > -    QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> > -        if (mon_fdset->id != fdset_id) {
> > -            continue;
> > +        if (fd == -1) {
> > +            errno = EINVAL;
> > +            return -1;
> 
> Missing qemu_mutex_unlock().
> 
> >          }
> 
> Old monitor_fdset_get_fd() returns -ENOENT when @fdset_id does not
> exist, and -EACCES when it doesn't contain a file descriptor matching
> @flags.
> 
> The new code seems to use EINVAL for the latter case.  Intentional?

No, its a mistake.


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 v5 5/8] util: add Error object for qemu_open_internal error reporting

[Daniel P. Berrangé]

On Thu, Sep 03, 2020 at 11:03:52AM +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 to callers that ask for it.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  util/osdep.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/util/osdep.c b/util/osdep.c
> > index dd34b58bb7..28aa89adc9 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -298,7 +298,7 @@ static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
> >   * 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;
> >  
> > @@ -312,12 +312,15 @@ 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;
> >          }
> >  
> >          dupfd = monitor_fdset_dup_fd_add(fdset_id, flags);
> >          if (dupfd == -1) {
> > +            error_setg_errno(errp, errno, "Could not dup FD for %s flags %x",
> > +                             name, flags);
> 
> You kept the reporting of flags here.  Intentional?

I think it is useful because one of the failure reasons for
monitor_fdset_dup_fd_add is mis-matching flags. So if we ever
get a bug report mentioning this error message it'd be useful
to have the flags present.

> > @@ -343,7 +353,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)) {
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

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