[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 1/8] monitor: simplify functions for getting a dup'd fdset
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v5 1/8] monitor: simplify functions for getting a dup'd fdset entry |
Date: |
Thu, 03 Sep 2020 10:52:40 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
Daniel P. Berrangé <berrange@redhat.com> writes:
> Currently code has to call monitor_fdset_get_fd, then dup
> the return fd, and then add the duplicate FD back into the
> fdset. This dance is overly verbose for the caller and
> introduces extra failure modes which can be avoided by
> folding all the logic into monitor_fdset_dup_fd_add and
> removing monitor_fdset_get_fd entirely.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> include/monitor/monitor.h | 3 +-
> include/qemu/osdep.h | 1 +
> monitor/misc.c | 58 +++++++++++++++++----------------------
> stubs/fdset.c | 8 ++----
> util/osdep.c | 19 ++-----------
> 5 files changed, 32 insertions(+), 57 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 1018d754a6..c0170773d4 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -43,8 +43,7 @@ int monitor_read_password(MonitorHMP *mon, ReadLineFunc
> *readline_func,
> AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
> bool has_opaque, const char *opaque,
> Error **errp);
> -int monitor_fdset_get_fd(int64_t fdset_id, int flags);
> -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
> +int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags);
> void monitor_fdset_dup_fd_remove(int dup_fd);
> int64_t monitor_fdset_dup_fd_find(int dup_fd);
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 412962d91a..66ee5bc45d 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -501,6 +501,7 @@ int qemu_open(const char *name, int flags, ...);
> int qemu_close(int fd);
> int qemu_unlink(const char *name);
> #ifndef _WIN32
> +int qemu_dup_flags(int fd, int flags);
> int qemu_dup(int fd);
> #endif
> int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> diff --git a/monitor/misc.c b/monitor/misc.c
> index e847b58a8c..98e389e4a8 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -1547,69 +1547,61 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool
> has_fdset_id, int64_t fdset_id,
> return fdinfo;
> }
>
> -int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> +
Extra blank line.
> +int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags)
> {
> #ifdef _WIN32
> return -ENOENT;
> #else
> MonFdset *mon_fdset;
> - MonFdsetFd *mon_fdset_fd;
> - int mon_fd_flags;
> - int ret;
>
> qemu_mutex_lock(&mon_fdsets_lock);
> QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> + MonFdsetFd *mon_fdset_fd;
> + MonFdsetFd *mon_fdset_fd_dup;
> + int fd = -1;
> + int dup_fd;
> + int mon_fd_flags;
> +
> if (mon_fdset->id != fdset_id) {
> continue;
> }
> +
> QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
> mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL);
> if (mon_fd_flags == -1) {
> - ret = -errno;
> - goto out;
> + qemu_mutex_unlock(&mon_fdsets_lock);
> + return -1;
> }
>
> if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
> - ret = mon_fdset_fd->fd;
> - goto out;
> + fd = mon_fdset_fd->fd;
> + break;
> }
> }
> - ret = -EACCES;
> - goto out;
> - }
> - ret = -ENOENT;
>
> -out:
> - qemu_mutex_unlock(&mon_fdsets_lock);
> - return ret;
> -#endif
> -}
> -
> -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> -{
> - MonFdset *mon_fdset;
> - MonFdsetFd *mon_fdset_fd_dup;
> -
> - qemu_mutex_lock(&mon_fdsets_lock);
> - QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> - if (mon_fdset->id != fdset_id) {
> - continue;
> + if (fd == -1) {
> + errno = EINVAL;
> + return -1;
Missing qemu_mutex_unlock().
> }
Old monitor_fdset_get_fd() returns -ENOENT when @fdset_id does not
exist, and -EACCES when it doesn't contain a file descriptor matching
@flags.
The new code seems to use EINVAL for the latter case. Intentional?
> - QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
> - if (mon_fdset_fd_dup->fd == dup_fd) {
> - goto err;
> - }
> +
> + dup_fd = qemu_dup_flags(fd, flags);
> + if (dup_fd == -1) {
> + qemu_mutex_unlock(&mon_fdsets_lock);
> + return -1;
> }
> +
> mon_fdset_fd_dup = g_malloc0(sizeof(*mon_fdset_fd_dup));
> mon_fdset_fd_dup->fd = dup_fd;
> QLIST_INSERT_HEAD(&mon_fdset->dup_fds, mon_fdset_fd_dup, next);
> qemu_mutex_unlock(&mon_fdsets_lock);
> - return 0;
> + return dup_fd;
> }
>
> -err:
> qemu_mutex_unlock(&mon_fdsets_lock);
> + errno = ENOENT;
> return -1;
> +#endif
> }
>
> static int64_t monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
> diff --git a/stubs/fdset.c b/stubs/fdset.c
> index 67dd5e1d34..56b3663d58 100644
> --- a/stubs/fdset.c
> +++ b/stubs/fdset.c
> @@ -1,8 +1,9 @@
> #include "qemu/osdep.h"
> #include "monitor/monitor.h"
>
> -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> +int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags)
> {
> + errno = ENOSYS;
> return -1;
> }
>
> @@ -11,11 +12,6 @@ int64_t monitor_fdset_dup_fd_find(int dup_fd)
> return -1;
> }
>
> -int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> -{
> - return -ENOENT;
> -}
> -
> void monitor_fdset_dup_fd_remove(int dupfd)
> {
> }
> diff --git a/util/osdep.c b/util/osdep.c
> index 4829c07ff6..3d94b4d732 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -122,7 +122,7 @@ static int fcntl_op_getlk = -1;
> /*
> * Dups an fd and sets the flags
> */
> -static int qemu_dup_flags(int fd, int flags)
> +int qemu_dup_flags(int fd, int flags)
> {
> int ret;
> int serrno;
> @@ -293,7 +293,7 @@ int qemu_open(const char *name, int flags, ...)
> /* Attempt dup of fd from fd set */
> if (strstart(name, "/dev/fdset/", &fdset_id_str)) {
> int64_t fdset_id;
> - int fd, dupfd;
> + int dupfd;
>
> fdset_id = qemu_parse_fdset(fdset_id_str);
> if (fdset_id == -1) {
> @@ -301,24 +301,11 @@ int qemu_open(const char *name, int flags, ...)
> return -1;
> }
>
> - fd = monitor_fdset_get_fd(fdset_id, flags);
> - if (fd < 0) {
> - errno = -fd;
> - return -1;
> - }
> -
> - dupfd = qemu_dup_flags(fd, flags);
> + dupfd = monitor_fdset_dup_fd_add(fdset_id, flags);
> if (dupfd == -1) {
> return -1;
> }
>
> - ret = monitor_fdset_dup_fd_add(fdset_id, dupfd);
> - if (ret == -1) {
> - close(dupfd);
> - errno = EINVAL;
> - return -1;
> - }
> -
> return dupfd;
> }
> #endif
Clear improvement, thanks!
- [PATCH v5 0/8] block: improve error reporting for unsupported O_DIRECT, Daniel P . Berrangé, 2020/09/02
- [PATCH v5 1/8] monitor: simplify functions for getting a dup'd fdset entry, Daniel P . Berrangé, 2020/09/02
- Re: [PATCH v5 1/8] monitor: simplify functions for getting a dup'd fdset entry,
Markus Armbruster <=
- [PATCH v5 2/8] util: split off a helper for dealing with O_CLOEXEC flag, Daniel P . Berrangé, 2020/09/02
- [PATCH v5 3/8] util: rename qemu_open() to qemu_open_old(), Daniel P . Berrangé, 2020/09/02
- [PATCH v5 4/8] util: refactor qemu_open_old to split off variadic args handling, Daniel P . Berrangé, 2020/09/02
- [PATCH v5 5/8] util: add Error object for qemu_open_internal error reporting, Daniel P . Berrangé, 2020/09/02