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

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

[Luiz Capitulino]

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;
     }
-- 
1.6.3.GIT

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1439 bytes --]

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.

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?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Luiz Capitulino]

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.

 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.

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

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.

> 
>  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!

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Luiz Capitulino]

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.