Re: [Qemu-devel] Re: monitor: check for readline in monitor_event()

[Luiz Capitulino <lcapitulino@redhat.com>]

On Tue, 16 Jun 2009 17:46:05 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> Luiz Capitulino wrote:
> > On Tue, 16 Jun 2009 00:42:26 +0200
> > Jan Kiszka <jan.kiszka@web.de> wrote:
> > 
> >> Luiz Capitulino wrote:
> >>> The call of readline_show_prompt() in CHR_EVENT_RESET's body will
> >>> trig a segfault if readline is not being used, because 'mon->rs'
> >>> will be NULL.
> >>>
> >>> This fixes the problem by adding the proper check.
> >>>
> >>> I've trigged this while playing with an off-tree code that
> >>> disables readline support, I'm not sure whether in-tree code can
> >>> trig this.
> >>>
> >>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >>> ---
> >>>  monitor.c |    2 +-
> >>>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/monitor.c b/monitor.c
> >>> index 6b45f6c..787101d 100644
> >>> --- a/monitor.c
> >>> +++ b/monitor.c
> >>> @@ -3002,7 +3002,7 @@ static void monitor_event(void *opaque, int
> >>> event) case CHR_EVENT_RESET:
> >>>          monitor_printf(mon, "QEMU %s monitor - type 'help' for
> >>> more " "information\n", QEMU_VERSION);
> >>> -        if (mon->chr->focus == 0)
> >>> +        if (mon->rs && mon->chr->focus == 0)
> >>>              readline_show_prompt(mon->rs);
> >>>          break;
> >>>      }
> >> In-tree code is not affected as no monitor user will call
> >> qemu_chr_reset for the associated char device if there is no
> >> readline active as well. Yeah, secret de-facto rule.
> > 
> >  Ok.
> > 
> >> The patch is not incorrect, but I would like to understand the
> >> out-of-tree use case behind it. So you do want the info line
> >> printed, but provide your own readline processor? What kind of
> >> terminal is this?
> > 
> >  I'm working on a machine-friendly monitor for QEMU, which disables
> > readline.
> 
> Again, I bet that your monitor will not be interested in the prompt
> header. So I think we should rather exclude the whole CHR_EVENT_RESET
> path for the !mon->rs case.

 It has a greeting message, although my real question is whether it's
going to support the mux part or not. If it's not, then I can have
my own reset handler.

 Ouch, I said I didn't want to get into details. :)

> >  I didn't want to get into the details now, it will be better to
> > discuss the prototype I'm writing, should be finished soon.
> 
> Looking forward!

 Great, will appreciate your feedback.