Re: [PATCH] monitor: Use LOCK_GUARD macros

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Use the lock guard macros in monitor/misc.c - saves
> > a bunch of goto's, and a temporary variable, but mostly
> > because I prefer not having to release them in error paths.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  monitor/misc.c | 62 ++++++++++++++++----------------------------------
> >  1 file changed, 20 insertions(+), 42 deletions(-)
> >
> > diff --git a/monitor/misc.c b/monitor/misc.c
> > index e847b58a8c..7f0b313d53 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);
> 
> 
> Hardly an improvement here.  I figure you want it for the sake of
> consistency.

Right, I was trying to do all the ones that weren't
worse;  there's one case I left where it explicitly needed to do
something after the unlock.

> > @@ -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);
> >          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);
> >  }
> >  
> 
> This one makes more sense.
> 
> [More of the same...]
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks.

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK