Re: [Qemu-devel] [PATCH v4] hmp: add info iothreads command

[Luiz Capitulino]

On Tue, 23 Jun 2015 13:57:08 +0200
Markus Armbruster <address@hidden> wrote:

> Ting Wang <address@hidden> writes:
> 
> > Hi Luiz and Markus,
> >
> > Would you like to pick up this patch, which has
> > been reviewed by Stefan and Fam?
> 
> Looks like this fell through the cracks back in March.  You should've
> asked for merge much earlier.  Pinging the maintainer after two weeks is
> fair.
> 
> I just did a monitor pull, and I can't yet say whether I'll do another
> for 2.4.

It would be great if you could pick this one, because I'll be out for
some days. I wouldn't mind this one going through the block tree either,
as the QMP command did go through it. Worst case I can merge this patch
and post a pull request right before hard freeze deadline.

> 
> Quick review inline.
> 
> >
> > Thanks.
> >
> > Ting
> >
> > On 2015-3-13 16:58, Ting Wang wrote:
> >> Make "info iothreads" available on the HMP monitor.
> >> 
> >> The results are as follows:
> >> id1: thread_id=thread_id1
> >> id2: thread_id=thread_id2
> 
> Actually, they are like
> 
> id1: thread_id=123
> id2: thread_id=456
> 
> Recommend to paste actual output from your testing.
> 
> >> 
> >> Signed-off-by: Ting Wang <address@hidden>
> >> ---
> >> v4: use the PRId64 format specifier macro for the int64_t thread_id
> >> v3: fix comment and the trailing whitespace
> >> v2: add braces for if
> >> ---
> >>  hmp-commands.hx |  2 ++
> >>  hmp.c           | 22 ++++++++++++++++++++++
> >>  hmp.h           |  1 +
> >>  monitor.c       |  7 +++++++
> >>  4 files changed, 32 insertions(+)
> >> 
> >> diff --git a/hmp-commands.hx b/hmp-commands.hx
> >> index d5022d8..67d76ed 100644
> >> --- a/hmp-commands.hx
> >> +++ b/hmp-commands.hx
> >> @@ -1746,6 +1746,8 @@ show roms
> >>  show the TPM device
> >>  @item info memory-devices
> >>  show the memory devices
> >> address@hidden info iothreads
> >> +show iothreads
> >>  @end table
> >>  ETEXI
> >>  
> >> diff --git a/hmp.c b/hmp.c
> >> index 71c28bc..445a8ad 100644
> >> --- a/hmp.c
> >> +++ b/hmp.c
> >> @@ -821,6 +821,28 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
> >>      qapi_free_TPMInfoList(info_list);
> >>  }
> >>  
> >> +void hmp_info_iothreads(Monitor *mon, const QDict *qdict)
> >> +{
> >> +    IOThreadInfoList *head = NULL, *elem = NULL;
> >> +
> >> +    head = qmp_query_iothreads(NULL);
> >> +    if (!head) {
> >> +        monitor_printf(mon, "No iothread has been added\n");
> >> +        return;
> >> +    }
> 
> Printing something instead of nothing when the list is empty is matter
> of taste.  Consistency would be nice, though.  I know several commands
> that print nothing.  Can you quote commands printing something?
> 
> >> +
> >> +    elem = head;
> >> +    while (elem) {
> >> +        if (elem->value) {
> >> +            monitor_printf(mon, "%s: thread_id=%" PRId64 "\n", 
> >> elem->value->id,
> >> +                    elem->value->thread_id);
> >> +        }
> >> +        elem = elem->next;
> >> +    }
> 
>     for (info = info_list; info; info = info->next) {
>         monitor_printf(mon, "%s: thread_id=%" PRId64 "\n",
>                        elem->value->id, elem->value->thread_id);
>     }
> 
> 1. for is more readable than while, because it has the full loop control
> in one place.
> 
> 2. List elements cannot be null, so don't bother checking for it.
> 
> >> +
> >> +    qapi_free_IOThreadInfoList(head);
> >> +}
> >> +
> >>  void hmp_quit(Monitor *mon, const QDict *qdict)
> >>  {
> >>      monitor_suspend(mon);
> >> diff --git a/hmp.h b/hmp.h
> >> index 81177b2..d99090e 100644
> >> --- a/hmp.h
> >> +++ b/hmp.h
> >> @@ -38,6 +38,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
> >>  void hmp_info_pci(Monitor *mon, const QDict *qdict);
> >>  void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
> >>  void hmp_info_tpm(Monitor *mon, const QDict *qdict);
> >> +void hmp_info_iothreads(Monitor *mon, const QDict *qdict);
> >>  void hmp_quit(Monitor *mon, const QDict *qdict);
> >>  void hmp_stop(Monitor *mon, const QDict *qdict);
> >>  void hmp_system_reset(Monitor *mon, const QDict *qdict);
> >> diff --git a/monitor.c b/monitor.c
> >> index c86a89e..076a306 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -2924,6 +2924,13 @@ static mon_cmd_t info_cmds[] = {
> >>          .mhandler.cmd = hmp_info_memory_devices,
> >>      },
> >>      {
> >> +        .name       = "iothreads",
> >> +        .args_type  = "",
> >> +        .params     = "",
> >> +        .help       = "show iothreads",
> >> +        .mhandler.cmd = hmp_info_iothreads,
> >> +    },
> >> +    {
> >>          .name       = NULL,
> >>      },
> >>  };
> >> 
>