qemu-block
[Top][All Lists]
Advanced

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]