Re: [PULL 23/26] monitor: Use LOCK_GUARD macros - Marc-André Lureau

From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
To: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: peterx@redhat.com, qemu-devel@nongnu.org, zhengchuan@huawei.com,
	 dovmurik@linux.vnet.ibm.com, hgcoin@gmail.com,
	 zhangjiachen.jaycee@bytedance.com, lvivier@redhat.com,
	stefanha@redhat.com,  vgoyal@redhat.com, jinyan12@huawei.com,
	ann.zhuangyanying@huawei.com,  quintela@redhat.com
Subject: Re: [PULL 23/26] monitor: Use LOCK_GUARD macros
Date: Tue, 7 Feb 2023 18:06:39 +0400	[thread overview]
Message-ID: <CAJ+F1CKLV_XG3pe_4YJD++-R71F5M4yepGOoR7nYocqs65y0fw@mail.gmail.com> (raw)
In-Reply-To: <Y+JVHUrYY2rFbBes@work-vm>

Hi

On Tue, Feb 7, 2023 at 5:41 PM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> > Hi David
> >
> > On Fri, Sep 25, 2020 at 4:27 PM Dr. David Alan Gilbert (git)
> > <dgilbert@redhat.com> wrote:
> > >
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > >
> > > Use the lock guard macros in monitor/misc.c - saves
> > > a lot of unlocks in error paths, and the occasional goto.
> > >
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > Message-Id: <20200922095741.101911-1-dgilbert@redhat.com>
> > > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > >  monitor/misc.c | 44 ++++++++++++++------------------------------
> > >  1 file changed, 14 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/monitor/misc.c b/monitor/misc.c
> > > index 262f2bd951..6e0da0cb96 100644
> > > --- a/monitor/misc.c
> > > +++ b/monitor/misc.c
> > > @@ -141,13 +141,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
> > >      handle_hmp_command(&hmp, command_line);
> > >      cur_mon = old_mon;
> > >
> > > -    qemu_mutex_lock(&hmp.common.mon_lock);
> > > -    if (qstring_get_length(hmp.common.outbuf) > 0) {
> > > -        output = g_strdup(qstring_get_str(hmp.common.outbuf));
> > > -    } else {
> > > -        output = g_strdup("");
> > > +    WITH_QEMU_LOCK_GUARD(&hmp.common.mon_lock) {
> > > +        if (qstring_get_length(hmp.common.outbuf) > 0) {
> > > +            output = g_strdup(qstring_get_str(hmp.common.outbuf));
> > > +        } else {
> > > +            output = g_strdup("");
> > > +        }
> > >      }
> > > -    qemu_mutex_unlock(&hmp.common.mon_lock);
> > >
> > >  out:
> > >      monitor_data_destroy(&hmp.common);
> > > @@ -1248,7 +1248,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> > >          return;
> > >      }
> > >
> > > -    qemu_mutex_lock(&cur_mon->mon_lock);
> > > +    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
> > >      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> > >          if (strcmp(monfd->name, fdname) != 0) {
> > >              continue;
> > > @@ -1256,7 +1256,6 @@ void qmp_getfd(const char *fdname, Error **errp)
> > >
> > >          tmp_fd = monfd->fd;
> > >          monfd->fd = fd;
> > > -        qemu_mutex_unlock(&cur_mon->mon_lock);
> > >          /* Make sure close() is outside critical section */
> > >          close(tmp_fd);
> >
> > The comment doesn't look true after this patch, since the lock is
> > released before returning. commit 9409fc05fe2 ("monitor: protect
> > mon->fds with mon_lock") introduced the lock & comment. Apparently,
> > you didn't convert qmp_closefd() to LOCK_GUARD, presumably because of
> > the comment. But you did it in qmp_getfd. What's the reason for that
> > though, avoiding syscalls while holding the lock or am I missing
> > something?
>
> No, I think you're right!
> Yeh I think the reason was to avoid the (potentially blocking) syscall
> with the lock held.
>

Ok, I'll send a patch, thanks

> Dave
>
> > thanks
> >
> > >          return;
> > > @@ -1267,7 +1266,6 @@ void qmp_getfd(const char *fdname, Error **errp)
> > >      monfd->fd = fd;
> > >
> > >      QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> > > -    qemu_mutex_unlock(&cur_mon->mon_lock);
> > >  }
> > >
> > >  void qmp_closefd(const char *fdname, Error **errp)
> > > @@ -1299,7 +1297,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
> > >  {
> > >      mon_fd_t *monfd;
> > >
> > > -    qemu_mutex_lock(&mon->mon_lock);
> > > +    QEMU_LOCK_GUARD(&mon->mon_lock);
> > >      QLIST_FOREACH(monfd, &mon->fds, next) {
> > >          int fd;
> > >
> > > @@ -1313,12 +1311,10 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
> > >          QLIST_REMOVE(monfd, next);
> > >          g_free(monfd->name);
> > >          g_free(monfd);
> > > -        qemu_mutex_unlock(&mon->mon_lock);
> > >
> > >          return fd;
> > >      }
> > >
> > > -    qemu_mutex_unlock(&mon->mon_lock);
> > >      error_setg(errp, "File descriptor named '%s' has not been found", fdname);
> > >      return -1;
> > >  }
> > > @@ -1350,11 +1346,10 @@ void monitor_fdsets_cleanup(void)
> > >      MonFdset *mon_fdset;
> > >      MonFdset *mon_fdset_next;
> > >
> > > -    qemu_mutex_lock(&mon_fdsets_lock);
> > > +    QEMU_LOCK_GUARD(&mon_fdsets_lock);
> > >      QLIST_FOREACH_SAFE(mon_fdset, &mon_fdsets, next, mon_fdset_next) {
> > >          monitor_fdset_cleanup(mon_fdset);
> > >      }
> > > -    qemu_mutex_unlock(&mon_fdsets_lock);
> > >  }
> > >
> > >  AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
> > > @@ -1389,7 +1384,7 @@ void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
> > >      MonFdsetFd *mon_fdset_fd;
> > >      char fd_str[60];
> > >
> > > -    qemu_mutex_lock(&mon_fdsets_lock);
> > > +    QEMU_LOCK_GUARD(&mon_fdsets_lock);
> > >      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> > >          if (mon_fdset->id != fdset_id) {
> > >              continue;
> > > @@ -1409,12 +1404,10 @@ void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
> > >              goto error;
> > >          }
> > >          monitor_fdset_cleanup(mon_fdset);
> > > -        qemu_mutex_unlock(&mon_fdsets_lock);
> > >          return;
> > >      }
> > >
> > >  error:
> > > -    qemu_mutex_unlock(&mon_fdsets_lock);
> > >      if (has_fd) {
> > >          snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId64 ", fd:%" PRId64,
> > >                   fdset_id, fd);
> > > @@ -1430,7 +1423,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
> > >      MonFdsetFd *mon_fdset_fd;
> > >      FdsetInfoList *fdset_list = NULL;
> > >
> > > -    qemu_mutex_lock(&mon_fdsets_lock);
> > > +    QEMU_LOCK_GUARD(&mon_fdsets_lock);
> > >      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> > >          FdsetInfoList *fdset_info = g_malloc0(sizeof(*fdset_info));
> > >          FdsetFdInfoList *fdsetfd_list = NULL;
> > > @@ -1460,7 +1453,6 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
> > >          fdset_info->next = fdset_list;
> > >          fdset_list = fdset_info;
> > >      }
> > > -    qemu_mutex_unlock(&mon_fdsets_lock);
> > >
> > >      return fdset_list;
> > >  }
> > > @@ -1554,7 +1546,7 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags)
> > >  #else
> > >      MonFdset *mon_fdset;
> > >
> > > -    qemu_mutex_lock(&mon_fdsets_lock);
> > > +    QEMU_LOCK_GUARD(&mon_fdsets_lock);
> > >      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> > >          MonFdsetFd *mon_fdset_fd;
> > >          MonFdsetFd *mon_fdset_fd_dup;
> > > @@ -1569,7 +1561,6 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags)
> > >          QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
> > >              mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL);
> > >              if (mon_fd_flags == -1) {
> > > -                qemu_mutex_unlock(&mon_fdsets_lock);
> > >                  return -1;
> > >              }
> > >
> > > @@ -1580,25 +1571,21 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags)
> > >          }
> > >
> > >          if (fd == -1) {
> > > -            qemu_mutex_unlock(&mon_fdsets_lock);
> > >              errno = EACCES;
> > >              return -1;
> > >          }
> > >
> > >          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 dup_fd;
> > >      }
> > >
> > > -    qemu_mutex_unlock(&mon_fdsets_lock);
> > >      errno = ENOENT;
> > >      return -1;
> > >  #endif
> > > @@ -1609,7 +1596,7 @@ static int64_t monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
> > >      MonFdset *mon_fdset;
> > >      MonFdsetFd *mon_fdset_fd_dup;
> > >
> > > -    qemu_mutex_lock(&mon_fdsets_lock);
> > > +    QEMU_LOCK_GUARD(&mon_fdsets_lock);
> > >      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> > >          QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
> > >              if (mon_fdset_fd_dup->fd == dup_fd) {
> > > @@ -1619,17 +1606,14 @@ static int64_t monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
> > >                      if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
> > >                          monitor_fdset_cleanup(mon_fdset);
> > >                      }
> > > -                    goto err;
> > > +                    return -1;
> > >                  } else {
> > > -                    qemu_mutex_unlock(&mon_fdsets_lock);
> > >                      return mon_fdset->id;
> > >                  }
> > >              }
> > >          }
> > >      }
> > >
> > > -err:
> > > -    qemu_mutex_unlock(&mon_fdsets_lock);
> > >      return -1;
> > >  }
> > >
> > > --
> > > 2.26.2
> > >
> > >
> >
> >
> > --
> > Marc-André Lureau
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>


-- 
Marc-André Lureau