From 60efffa41b34cc299fed65b67e2c2641846d592b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Thu, 27 Aug 2020 13:27:00 +0100 Subject: monitor: simplify functions for getting a dup'd fdset entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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é --- 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 1018d75..c017077 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 412962d..66ee5bc 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 e847b58..0b1b9b1 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) { + qemu_mutex_unlock(&mon_fdsets_lock); + errno = EACCES; + 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 67dd5e1..56b3663 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 4829c07..3d94b4d 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 -- cgit v1.1 From c2069ff624eda7f94bc118d7b5825bce29d7ca94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Wed, 2 Sep 2020 16:58:06 +0100 Subject: util: split off a helper for dealing with O_CLOEXEC flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Reviewed-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- util/osdep.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/util/osdep.c b/util/osdep.c index 3d94b4d..0d8fa9f 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)) { -- cgit v1.1 From 448058aa99aaf30e7b8978508e575284a19fcfc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Tue, 21 Jul 2020 13:25:21 +0100 Subject: util: rename qemu_open() to qemu_open_old() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- 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/usb/u2f-passthru.c | 4 ++-- 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 +- 24 files changed, 44 insertions(+), 42 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 63ef6af..ad8b315 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 32998d8..245b12a 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 10722e0..6d6294e 100644 --- a/backends/tpm/tpm_passthrough.c +++ b/backends/tpm/tpm_passthrough.c @@ -218,7 +218,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)); @@ -236,11 +236,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); } } } @@ -272,7 +272,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 9a00d41..bac2566 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 e2900c3..b28603c 100644 --- a/block/file-win32.c +++ b/block/file-win32.c @@ -596,8 +596,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 36b53c8..5abb90e 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 c2d8101..1cd62f2 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 fd12c9e..7eca5d9 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 77e7ec8..6b85099 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 383bc78..13fda44 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 db2f49c..5cc559f 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 43c9350..8b02bee 100644 --- a/hw/usb/host-libusb.c +++ b/hw/usb/host-libusb.c @@ -1147,7 +1147,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/usb/u2f-passthru.c b/hw/usb/u2f-passthru.c index e9c8aa4..ae00e93 100644 --- a/hw/usb/u2f-passthru.c +++ b/hw/usb/u2f-passthru.c @@ -383,7 +383,7 @@ static int u2f_passthru_open_from_device(struct udev_device *device) { const char *devnode = udev_device_get_devnode(device); - int fd = qemu_open(devnode, O_RDWR); + int fd = qemu_open_old(devnode, O_RDWR); if (fd < 0) { return -1; } else if (!u2f_passthru_is_u2f_device(fd)) { @@ -482,7 +482,7 @@ static void u2f_passthru_realize(U2FKeyState *base, Error **errp) return; #endif } else { - fd = qemu_open(key->hidraw, O_RDWR); + fd = qemu_open_old(key->hidraw, O_RDWR); if (fd < 0) { error_setg(errp, "%s: Failed to open %s", TYPE_U2F_PASSTHRU, key->hidraw); diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 3335714..13471ae 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 66ee5bc..ae12341 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 dac21f6..2ed3b75 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 bc0e0d2..e2b3ba8 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 bf98508..0bfd8e2 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 8fc205a..0373975 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 af5a58a..3bffee9 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -1365,7 +1365,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; @@ -1432,7 +1432,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; } @@ -1522,7 +1522,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 2eae733..0dcb9bf 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 f8d7643..7592c3c 100644 --- a/ui/console.c +++ b/ui/console.c @@ -373,7 +373,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 0d8fa9f..7504c15 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 ad8001a..f5f676f 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; -- cgit v1.1 From bf93d2ade9008235fe2fbfd683458509ce53d6f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Wed, 1 Jul 2020 16:30:35 +0100 Subject: util: refactor qemu_open_old to split off variadic args handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This simple refactoring prepares for future patches. The variadic args handling is split from the main bulk of the open logic. Reviewed-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- util/osdep.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/util/osdep.c b/util/osdep.c index 7504c15..11531e8 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -296,10 +296,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 +324,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)) { -- cgit v1.1 From ebb3d49cb25b1f1d1b3e870ffdf7a05e02d28624 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Fri, 21 Aug 2020 17:45:02 +0100 Subject: util: add Error object for qemu_open_internal error reporting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of relying on the limited information from errno, we can now also provide detailed error messages to callers that ask for it. Reviewed-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- util/osdep.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/util/osdep.c b/util/osdep.c index 11531e8..28aa89a 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. */ @@ -297,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; @@ -311,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; } @@ -326,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; } @@ -342,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)) { -- cgit v1.1 From c490af57cb451bf39707f4cc73650e81c5797221 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Wed, 1 Jul 2020 16:30:35 +0100 Subject: util: introduce qemu_open and qemu_create with error reporting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Daniel P. Berrangé --- 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 ae12341..577d9e8 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 28aa89a..c99f1e7 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; -- cgit v1.1 From 661b3e81a336570dfbf4c224595921d490ac792f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Tue, 21 Jul 2020 16:17:35 +0100 Subject: util: give a specific error message when O_DIRECT doesn't work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Eric Blake Signed-off-by: Daniel P. Berrangé --- util/osdep.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/util/osdep.c b/util/osdep.c index c99f1e7..8ea7a80 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; } -- cgit v1.1 From b18a24a9f889bcf722754046130507d744a1b0b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Wed, 1 Jul 2020 15:22:43 +0100 Subject: block/file: switch to use qemu_open/qemu_create for improved errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- 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 bac2566..c63926d 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 b28603c..2642088 100644 --- a/block/file-win32.c +++ b/block/file-win32.c @@ -596,11 +596,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); -- cgit v1.1