Message ID | 1373512429-17865-6-git-send-email-xiawenc@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On Thu, 11 Jul 2013 11:13:41 +0800 Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > Now all completion functions do not use *cur_mon any more, instead > they use rs->mon. In short, structure ReadLineState decide where > the complete action would be taken now. > > Tested with the case that qemu have two telnet monitors, auto > completion function works normal. > > Take a better look at monitor_init(), it calls readline_init() which > initialize mon->rs, result is mon->rs->mon == mon. Then it calls > qemu_chr_add_handlers(), 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 actuall monitor > instance, so it is safe to replace *cur_mon in those functions. I find this paragraph a bit confusing and not exactly related to this patch. I know I asked you to explain why dropping cur_mon usage shouldn't brake things, but looks like you don't have a good place to have that info. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > --- > readline.c | 3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/readline.c b/readline.c > index c91b324..abf27dd 100644 > --- a/readline.c > +++ b/readline.c > @@ -276,7 +276,6 @@ void readline_set_completion_index(ReadLineState *rs, int index) > > static void readline_completion(ReadLineState *rs) > { > - Monitor *mon = cur_mon; > int len, i, j, max_width, nb_cols, max_prefix; > char *cmdline; > > @@ -300,7 +299,7 @@ static void readline_completion(ReadLineState *rs) > if (len > 0 && rs->completions[0][len - 1] != '/') > readline_insert_char(rs, ' '); > } else { > - monitor_printf(mon, "\n"); > + monitor_printf(rs->mon, "\n"); > max_width = 0; > max_prefix = 0; > for(i = 0; i < rs->nb_completions; i++) {
于 2013-7-18 3:34, Luiz Capitulino 写道: > On Thu, 11 Jul 2013 11:13:41 +0800 > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > >> Now all completion functions do not use *cur_mon any more, instead >> they use rs->mon. In short, structure ReadLineState decide where >> the complete action would be taken now. >> >> Tested with the case that qemu have two telnet monitors, auto >> completion function works normal. >> >> Take a better look at monitor_init(), it calls readline_init() which >> initialize mon->rs, result is mon->rs->mon == mon. Then it calls >> qemu_chr_add_handlers(), 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 actuall monitor >> instance, so it is safe to replace *cur_mon in those functions. > > I find this paragraph a bit confusing and not exactly related to > this patch. I know I asked you to explain why dropping cur_mon usage > shouldn't brake things, but looks like you don't have a good place > to have that info. > >> How about refine the message as: "Now usage of cur_mon is dropped in readline_completion(), it is safe because: ..... " ?
On Thu, 18 Jul 2013 09:55:51 +0800 Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > 于 2013-7-18 3:34, Luiz Capitulino 写道: > > On Thu, 11 Jul 2013 11:13:41 +0800 > > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > > > >> Now all completion functions do not use *cur_mon any more, instead > >> they use rs->mon. In short, structure ReadLineState decide where > >> the complete action would be taken now. > >> > >> Tested with the case that qemu have two telnet monitors, auto > >> completion function works normal. > >> > >> Take a better look at monitor_init(), it calls readline_init() which > >> initialize mon->rs, result is mon->rs->mon == mon. Then it calls > >> qemu_chr_add_handlers(), 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 actuall monitor > >> instance, so it is safe to replace *cur_mon in those functions. > > > > I find this paragraph a bit confusing and not exactly related to > > this patch. I know I asked you to explain why dropping cur_mon usage > > shouldn't brake things, but looks like you don't have a good place > > to have that info. > > > >> > How about refine the message as: > "Now usage of cur_mon is dropped in readline_completion(), it is safe > because: > ..... > " > ? I think you have to just describe what this commit does.
diff --git a/readline.c b/readline.c index c91b324..abf27dd 100644 --- a/readline.c +++ b/readline.c @@ -276,7 +276,6 @@ void readline_set_completion_index(ReadLineState *rs, int index) static void readline_completion(ReadLineState *rs) { - Monitor *mon = cur_mon; int len, i, j, max_width, nb_cols, max_prefix; char *cmdline; @@ -300,7 +299,7 @@ static void readline_completion(ReadLineState *rs) if (len > 0 && rs->completions[0][len - 1] != '/') readline_insert_char(rs, ' '); } else { - monitor_printf(mon, "\n"); + monitor_printf(rs->mon, "\n"); max_width = 0; max_prefix = 0; for(i = 0; i < rs->nb_completions; i++) {
Now all completion functions do not use *cur_mon any more, instead they use rs->mon. In short, structure ReadLineState decide where the complete action would be taken now. Tested with the case that qemu have two telnet monitors, auto completion function works normal. Take a better look at monitor_init(), it calls readline_init() which initialize mon->rs, result is mon->rs->mon == mon. Then it calls qemu_chr_add_handlers(), 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 actuall monitor instance, so it is safe to replace *cur_mon in those functions. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- readline.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)