qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V8 00/13] monitor: support sub command group in


From: Wenchao Xia
Subject: Re: [Qemu-devel] [PATCH V8 00/13] monitor: support sub command group in auto completion and help
Date: Wed, 21 Aug 2013 17:17:04 +0800
User-agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20130801 Thunderbird/17.0.8

于 2013-8-20 22:04, Luiz Capitulino 写道:
On Tue, 30 Jul 2013 12:03:11 -0400
Luiz Capitulino <address@hidden> wrote:

On Fri, 26 Jul 2013 11:20:29 +0800
Wenchao Xia <address@hidden> wrote:

This series make auto completion and help functions works normal for sub
command, by using reentrant functions. In order to do that, global variables
are not directly used in those functions any more. With this series, cmd_table
is a member of structure Monitor so it is possible to create a monitor with
different command table now, auto completion will work in that monitor. In
short, "info" is not treated as a special case now, this series ensure help
and auto complete function works normal for any sub command added in the future.

Patch 5 replaced cur_mon with rs->mon, it is safe because:
monitor_init() calls readline_init() which initialize mon->rs, result is
mon->rs->mon == mon. Then qemu_chr_add_handlers() is called, which make
monitor_read() function take *mon as its opaque. Later, when user input,
monitor_read() is called, where cur_mon is set to *mon by "cur_mon = opaque".
If qemu's monitors run in one thread, then later in readline_handle_byte()
and readline_comletion(), cur_mon is actually equal to rs->mon, in another
word, it points to the monitor instance, so it is safe to replace *cur_mon
in those functions.

I've applied this to qmp-next with the change I suggested for
patch 09/13.

Unfortunately this series brakes make check:

GTESTER check-qtest-x86_64
Broken pipe
GTester: last random seed: R02S3492bd34f44dd17460851643383be44d
main-loop: WARNING: I/O thread spun for 1000 iterations
make: *** [check-qtest-x86_64] Error 1

I debugged it (with some help from Laszlo) and the problem is that it
broke the human-monitor-command command. Any usage of this command
triggers the bug like:

{ "execute": "human-monitor-command",
              "arguments": { "command-line": "info registers" } }

It seems simple to fix, I think you just have to initialize
mon->cmd_table in qmp_human_monitor_command(), but I'd recommend two
things:

  1. It's better to split off some/all QMP initialization from
     monitor_init() and call it from qmp_human_monitor_command()

  2. Can you please take the opportunity and test all commands using
     cur_mon? Just grep for it

Sorry for noticing this only now, but I only run make check before
sending a pull request (although this very likely shows you didn't
run it either).

  My bad that not ran make check before, will fix and retry, sorry for
the trouble.

--
Best Regards

Wenchao Xia




reply via email to

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