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é --- util/osdep.c | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) (limited to 'util') 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(-) (limited to 'util') 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é --- util/osdep.c | 2 +- util/oslib-posix.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'util') 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(-) (limited to 'util') 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(-) (limited to 'util') 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é --- util/osdep.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) (limited to 'util') 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(-) (limited to 'util') 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