* [PATCH v6 1/8] monitor: simplify functions for getting a dup'd fdset entry
2020-09-03 15:22 [PATCH v6 0/8] block: improve error reporting for unsupported O_DIRECT Daniel P. Berrangé
@ 2020-09-03 15:22 ` Daniel P. Berrangé
2020-09-03 16:51 ` Richard Henderson
2020-09-04 7:33 ` Markus Armbruster
2020-09-03 15:22 ` [PATCH v6 2/8] util: split off a helper for dealing with O_CLOEXEC flag Daniel P. Berrangé
` (7 subsequent siblings)
8 siblings, 2 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-09-03 15:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Daniel P. Berrangé, qemu-block,
Markus Armbruster, Max Reitz, Philippe Mathieu-Daudé
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..0b1b9b196c 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 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v6 1/8] monitor: simplify functions for getting a dup'd fdset entry
2020-09-03 15:22 ` [PATCH v6 1/8] monitor: simplify functions for getting a dup'd fdset entry Daniel P. Berrangé
@ 2020-09-03 16:51 ` Richard Henderson
2020-09-04 7:33 ` Markus Armbruster
1 sibling, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2020-09-03 16:51 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Kevin Wolf, Philippe Mathieu-Daudé, Markus Armbruster,
qemu-block, Max Reitz
On 9/3/20 8:22 AM, Daniel P. Berrangé wrote:
> 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(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v6 1/8] monitor: simplify functions for getting a dup'd fdset entry
2020-09-03 15:22 ` [PATCH v6 1/8] monitor: simplify functions for getting a dup'd fdset entry Daniel P. Berrangé
2020-09-03 16:51 ` Richard Henderson
@ 2020-09-04 7:33 ` Markus Armbruster
1 sibling, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2020-09-04 7:33 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Philippe Mathieu-Daudé, Max Reitz
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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 2/8] util: split off a helper for dealing with O_CLOEXEC flag
2020-09-03 15:22 [PATCH v6 0/8] block: improve error reporting for unsupported O_DIRECT Daniel P. Berrangé
2020-09-03 15:22 ` [PATCH v6 1/8] monitor: simplify functions for getting a dup'd fdset entry Daniel P. Berrangé
@ 2020-09-03 15:22 ` Daniel P. Berrangé
2020-09-03 16:51 ` Richard Henderson
2020-09-03 15:22 ` [PATCH v6 3/8] util: rename qemu_open() to qemu_open_old() Daniel P. Berrangé
` (6 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-09-03 15:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Daniel P. Berrangé, qemu-block,
Markus Armbruster, Max Reitz, Philippe Mathieu-Daudé
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 <armbru@redhat.com>
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v6 2/8] util: split off a helper for dealing with O_CLOEXEC flag
2020-09-03 15:22 ` [PATCH v6 2/8] util: split off a helper for dealing with O_CLOEXEC flag Daniel P. Berrangé
@ 2020-09-03 16:51 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2020-09-03 16:51 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Kevin Wolf, Philippe Mathieu-Daudé, Markus Armbruster,
qemu-block, Max Reitz
On 9/3/20 8:22 AM, Daniel P. Berrangé wrote:
> 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 <armbru@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> util/osdep.c | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 3/8] util: rename qemu_open() to qemu_open_old()
2020-09-03 15:22 [PATCH v6 0/8] block: improve error reporting for unsupported O_DIRECT Daniel P. Berrangé
2020-09-03 15:22 ` [PATCH v6 1/8] monitor: simplify functions for getting a dup'd fdset entry Daniel P. Berrangé
2020-09-03 15:22 ` [PATCH v6 2/8] util: split off a helper for dealing with O_CLOEXEC flag Daniel P. Berrangé
@ 2020-09-03 15:22 ` Daniel P. Berrangé
2020-09-03 16:51 ` Richard Henderson
2020-09-03 15:22 ` [PATCH v6 4/8] util: refactor qemu_open_old to split off variadic args handling Daniel P. Berrangé
` (5 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-09-03 15:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Daniel P. Berrangé, qemu-block,
Philippe Mathieu-Daudé, Markus Armbruster, Max Reitz,
Philippe Mathieu-Daudé
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>
Reviewed-by: Markus Armbruster <armbru@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/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 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/usb/u2f-passthru.c b/hw/usb/u2f-passthru.c
index e9c8aa4595..ae00e93f35 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 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v6 3/8] util: rename qemu_open() to qemu_open_old()
2020-09-03 15:22 ` [PATCH v6 3/8] util: rename qemu_open() to qemu_open_old() Daniel P. Berrangé
@ 2020-09-03 16:51 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2020-09-03 16:51 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Kevin Wolf, qemu-block, Philippe Mathieu-Daudé,
Markus Armbruster, Max Reitz, Philippe Mathieu-Daudé
On 9/3/20 8:22 AM, Daniel P. Berrangé wrote:
> 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>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 4/8] util: refactor qemu_open_old to split off variadic args handling
2020-09-03 15:22 [PATCH v6 0/8] block: improve error reporting for unsupported O_DIRECT Daniel P. Berrangé
` (2 preceding siblings ...)
2020-09-03 15:22 ` [PATCH v6 3/8] util: rename qemu_open() to qemu_open_old() Daniel P. Berrangé
@ 2020-09-03 15:22 ` Daniel P. Berrangé
2020-09-03 16:52 ` Richard Henderson
2020-09-03 15:22 ` [PATCH v6 5/8] util: add Error object for qemu_open_internal error reporting Daniel P. Berrangé
` (4 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-09-03 15:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Daniel P. Berrangé, qemu-block,
Markus Armbruster, Max Reitz, Philippe Mathieu-Daudé
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 <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
util/osdep.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/util/osdep.c b/util/osdep.c
index 7504c156e8..11531e8f59 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)) {
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v6 5/8] util: add Error object for qemu_open_internal error reporting
2020-09-03 15:22 [PATCH v6 0/8] block: improve error reporting for unsupported O_DIRECT Daniel P. Berrangé
` (3 preceding siblings ...)
2020-09-03 15:22 ` [PATCH v6 4/8] util: refactor qemu_open_old to split off variadic args handling Daniel P. Berrangé
@ 2020-09-03 15:22 ` Daniel P. Berrangé
2020-09-03 16:53 ` Richard Henderson
2020-09-03 15:22 ` [PATCH v6 6/8] util: introduce qemu_open and qemu_create with " Daniel P. Berrangé
` (3 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-09-03 15:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Daniel P. Berrangé, qemu-block,
Markus Armbruster, Max Reitz, Philippe Mathieu-Daudé
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 <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
util/osdep.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/util/osdep.c b/util/osdep.c
index 11531e8f59..28aa89adc9 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)) {
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v6 5/8] util: add Error object for qemu_open_internal error reporting
2020-09-03 15:22 ` [PATCH v6 5/8] util: add Error object for qemu_open_internal error reporting Daniel P. Berrangé
@ 2020-09-03 16:53 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2020-09-03 16:53 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Kevin Wolf, Philippe Mathieu-Daudé, Markus Armbruster,
qemu-block, Max Reitz
On 9/3/20 8:22 AM, Daniel P. Berrangé wrote:
> 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 <armbru@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> util/osdep.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 6/8] util: introduce qemu_open and qemu_create with error reporting
2020-09-03 15:22 [PATCH v6 0/8] block: improve error reporting for unsupported O_DIRECT Daniel P. Berrangé
` (4 preceding siblings ...)
2020-09-03 15:22 ` [PATCH v6 5/8] util: add Error object for qemu_open_internal error reporting Daniel P. Berrangé
@ 2020-09-03 15:22 ` Daniel P. Berrangé
2020-09-03 16:54 ` Richard Henderson
2020-09-03 15:22 ` [PATCH v6 7/8] util: give a specific error message when O_DIRECT doesn't work Daniel P. Berrangé
` (2 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-09-03 15:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Daniel P. Berrangé, qemu-block,
Markus Armbruster, Max Reitz, Philippe Mathieu-Daudé
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v6 6/8] util: introduce qemu_open and qemu_create with error reporting
2020-09-03 15:22 ` [PATCH v6 6/8] util: introduce qemu_open and qemu_create with " Daniel P. Berrangé
@ 2020-09-03 16:54 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2020-09-03 16:54 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Kevin Wolf, Philippe Mathieu-Daudé, Markus Armbruster,
qemu-block, Max Reitz
On 9/3/20 8:22 AM, Daniel P. Berrangé wrote:
> 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>
> ---
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 7/8] util: give a specific error message when O_DIRECT doesn't work
2020-09-03 15:22 [PATCH v6 0/8] block: improve error reporting for unsupported O_DIRECT Daniel P. Berrangé
` (5 preceding siblings ...)
2020-09-03 15:22 ` [PATCH v6 6/8] util: introduce qemu_open and qemu_create with " Daniel P. Berrangé
@ 2020-09-03 15:22 ` Daniel P. Berrangé
2020-09-03 16:55 ` Richard Henderson
2020-09-03 15:22 ` [PATCH v6 8/8] block/file: switch to use qemu_open/qemu_create for improved errors Daniel P. Berrangé
2020-09-10 13:07 ` [PATCH v6 0/8] block: improve error reporting for unsupported O_DIRECT Daniel P. Berrangé
8 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-09-03 15:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Daniel P. Berrangé, qemu-block,
Markus Armbruster, Max Reitz, Philippe Mathieu-Daudé
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v6 7/8] util: give a specific error message when O_DIRECT doesn't work
2020-09-03 15:22 ` [PATCH v6 7/8] util: give a specific error message when O_DIRECT doesn't work Daniel P. Berrangé
@ 2020-09-03 16:55 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2020-09-03 16:55 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Kevin Wolf, Philippe Mathieu-Daudé, Markus Armbruster,
qemu-block, Max Reitz
On 9/3/20 8:22 AM, Daniel P. Berrangé wrote:
> 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(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 8/8] block/file: switch to use qemu_open/qemu_create for improved errors
2020-09-03 15:22 [PATCH v6 0/8] block: improve error reporting for unsupported O_DIRECT Daniel P. Berrangé
` (6 preceding siblings ...)
2020-09-03 15:22 ` [PATCH v6 7/8] util: give a specific error message when O_DIRECT doesn't work Daniel P. Berrangé
@ 2020-09-03 15:22 ` Daniel P. Berrangé
2020-09-03 16:56 ` Richard Henderson
2020-09-10 13:07 ` [PATCH v6 0/8] block: improve error reporting for unsupported O_DIRECT Daniel P. Berrangé
8 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-09-03 15:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Daniel P. Berrangé, qemu-block,
Philippe Mathieu-Daudé, Markus Armbruster, Max Reitz,
Philippe Mathieu-Daudé
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v6 8/8] block/file: switch to use qemu_open/qemu_create for improved errors
2020-09-03 15:22 ` [PATCH v6 8/8] block/file: switch to use qemu_open/qemu_create for improved errors Daniel P. Berrangé
@ 2020-09-03 16:56 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2020-09-03 16:56 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Kevin Wolf, qemu-block, Philippe Mathieu-Daudé,
Markus Armbruster, Max Reitz, Philippe Mathieu-Daudé
On 9/3/20 8:22 AM, Daniel P. Berrangé wrote:
> Currently at startup if using cache=none on a filesystem lacking
> O_DIRECT such as tmpfs, at startup QEMU prints
>
> qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: file system may not support O_DIRECT
> qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Could not open '/tmp/foo.img': Invalid argument
>
> 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>
> ---
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v6 0/8] block: improve error reporting for unsupported O_DIRECT
2020-09-03 15:22 [PATCH v6 0/8] block: improve error reporting for unsupported O_DIRECT Daniel P. Berrangé
` (7 preceding siblings ...)
2020-09-03 15:22 ` [PATCH v6 8/8] block/file: switch to use qemu_open/qemu_create for improved errors Daniel P. Berrangé
@ 2020-09-10 13:07 ` Daniel P. Berrangé
8 siblings, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-09-10 13:07 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Max Reitz,
Philippe Mathieu-Daudé
Kevin / Max,
Could you confirm whether you're ok with me sending a pull request for
this, or prefer to funnel it through the block tree for sanity checking.
It changes error messages from the block file driver on non-win32
platforms.
I've tested qemu-iotests with -raw and -qcow2 and didn't see failures
On Thu, Sep 03, 2020 at 04:22:02PM +0100, Daniel P. Berrangé wrote:
> 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
> v5: https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg00947.html
>
> See patch commit messages for rationale
>
> Ideally we would convert other callers of qemu_open_old to use
> qemu_open, and eventually remove qemu_open_old entirely, so every
> caller gets use of Error **errp.
>
> Improved in v6:
>
> - Fix errno regression dup'ing FD
> - Move qapi header to correct patch
> - Fix whitespace and commit messages
> - Converted more use of qemu_open to qemu_open_old after rebase
>
> 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/usb/u2f-passthru.c | 4 +-
> 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 +-
> 27 files changed, 150 insertions(+), 116 deletions(-)
>
> --
> 2.26.2
>
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 :|
^ permalink raw reply [flat|nested] 19+ messages in thread