[Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Marc-André Lureau]

Commit fb5e19d2e1472e96d72d5e4d89c20033f8ab345c originally fixed the
regression, but was inadvertently broken again in merge commit
2d6752d38d8acda.

Fixes:
https://bugs.launchpad.net/qemu/+bug/1654137

Cc: qemu-stable@nongnu.org
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-mux.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 1b925c8dec..6055e76293 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -304,6 +304,7 @@ void mux_set_focus(Chardev *chr, int focus)
     }
 
     d->focus = focus;
+    chr->be = d->backends[focus];
     mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
 }
 
-- 
2.17.0.rc1.36.gcedb63ea2f

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Peter Maydell]

On 16 April 2018 at 19:18, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> Commit fb5e19d2e1472e96d72d5e4d89c20033f8ab345c originally fixed the
> regression, but was inadvertently broken again in merge commit
> 2d6752d38d8acda.
>
> Fixes:
> https://bugs.launchpad.net/qemu/+bug/1654137
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  chardev/char-mux.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> index 1b925c8dec..6055e76293 100644
> --- a/chardev/char-mux.c
> +++ b/chardev/char-mux.c
> @@ -304,6 +304,7 @@ void mux_set_focus(Chardev *chr, int focus)
>      }
>
>      d->focus = focus;
> +    chr->be = d->backends[focus];
>      mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
>  }
>
> --
> 2.17.0.rc1.36.gcedb63ea2f

Opinions welcome on whether this is a regression fix worth
putting into rc4.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Daniel P. Berrangé]

On Mon, Apr 16, 2018 at 07:28:28PM +0100, Peter Maydell wrote:
> On 16 April 2018 at 19:18, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> > Commit fb5e19d2e1472e96d72d5e4d89c20033f8ab345c originally fixed the
> > regression, but was inadvertently broken again in merge commit
> > 2d6752d38d8acda.
> >
> > Fixes:
> > https://bugs.launchpad.net/qemu/+bug/1654137
> >
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  chardev/char-mux.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> > index 1b925c8dec..6055e76293 100644
> > --- a/chardev/char-mux.c
> > +++ b/chardev/char-mux.c
> > @@ -304,6 +304,7 @@ void mux_set_focus(Chardev *chr, int focus)
> >      }
> >
> >      d->focus = focus;
> > +    chr->be = d->backends[focus];
> >      mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
> >  }
> >
> > --
> > 2.17.0.rc1.36.gcedb63ea2f
> 
> Opinions welcome on whether this is a regression fix worth
> putting into rc4.

It is a regression, but a long standing one - we've been broken for quite
a while since 2.9.0 or even before.

If we're doing an rc4 anyway I'd suggest including it, but not the end of
the world if it has to go in via -stable given how long we've been broken
for.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Peter Maydell]

On 16 April 2018 at 19:44, Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Mon, Apr 16, 2018 at 07:28:28PM +0100, Peter Maydell wrote:
>> On 16 April 2018 at 19:18, Marc-André Lureau
>> <marcandre.lureau@redhat.com> wrote:
>> > Commit fb5e19d2e1472e96d72d5e4d89c20033f8ab345c originally fixed the
>> > regression, but was inadvertently broken again in merge commit
>> > 2d6752d38d8acda.
>> >
>> > Fixes:
>> > https://bugs.launchpad.net/qemu/+bug/1654137
>> >
>> > Cc: qemu-stable@nongnu.org
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  chardev/char-mux.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/chardev/char-mux.c b/chardev/char-mux.c
>> > index 1b925c8dec..6055e76293 100644
>> > --- a/chardev/char-mux.c
>> > +++ b/chardev/char-mux.c
>> > @@ -304,6 +304,7 @@ void mux_set_focus(Chardev *chr, int focus)
>> >      }
>> >
>> >      d->focus = focus;
>> > +    chr->be = d->backends[focus];
>> >      mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
>> >  }
>> >
>> > --
>> > 2.17.0.rc1.36.gcedb63ea2f
>>
>> Opinions welcome on whether this is a regression fix worth
>> putting into rc4.
>
> It is a regression, but a long standing one - we've been broken for quite
> a while since 2.9.0 or even before.
>
> If we're doing an rc4 anyway I'd suggest including it, but not the end of
> the world if it has to go in via -stable given how long we've been broken
> for.
>

Thanks for the clarification; I've applied this to master.

-- PMM

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Philippe Mathieu-Daudé]

Hi,

>>> On 16 April 2018 at 19:18, Marc-André Lureau
>>> <marcandre.lureau@redhat.com> wrote:
>>>> Commit fb5e19d2e1472e96d72d5e4d89c20033f8ab345c originally fixed the
>>>> regression, but was inadvertently broken again in merge commit
>>>> 2d6752d38d8acda.
>>>>
>>>> Fixes:
>>>> https://bugs.launchpad.net/qemu/+bug/1654137
>>>>
>>>> Cc: qemu-stable@nongnu.org
>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>> ---
>>>>  chardev/char-mux.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/chardev/char-mux.c b/chardev/char-mux.c
>>>> index 1b925c8dec..6055e76293 100644
>>>> --- a/chardev/char-mux.c
>>>> +++ b/chardev/char-mux.c
>>>> @@ -304,6 +304,7 @@ void mux_set_focus(Chardev *chr, int focus)
>>>>      }
>>>>
>>>>      d->focus = focus;
>>>> +    chr->be = d->backends[focus];
>>>>      mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
>>>>  }
>>>>
>>>> --
>>>> 2.17.0.rc1.36.gcedb63ea2f
>>>
>>> Opinions welcome on whether this is a regression fix worth
>>> putting into rc4.
>>
>> It is a regression, but a long standing one - we've been broken for quite
>> a while since 2.9.0 or even before.
>>
>> If we're doing an rc4 anyway I'd suggest including it, but not the end of
>> the world if it has to go in via -stable given how long we've been broken
>> for.
>>
> 
> Thanks for the clarification; I've applied this to master.

Since this commit, the console on the Malta board stay black...

Before:
$ qemu-system-mips -M malta -m 512 \
  -kernel vmlinux-3.2.0-4-4kc-malta -append 'root=/dev/sda1' \
  -nographic
[    0.000000] Initializing cgroup subsys cpuset
[    0.000000] Initializing cgroup subsys cpu
[    0.000000] Linux version 3.2.0-4-4kc-malta
(debian-kernel@lists.debian.org) (gcc version 4.6.3 (Debian 4.6.3-14) )
#1 Debian 3.2.51-1
[    0.000000] Config serial console: console=ttyS0,38400n8r
[    0.000000] bootconsole [early0] enabled
...

After:
$ qemu-system-mips -M malta -m 512 \
  -kernel vmlinux-3.2.0-4-4kc-malta -append 'root=/dev/sda1' \
  -nographic
QEMU 2.11.92 monitor - type 'help' for more information
(qemu) QEMU 2.11.92 monitor - type 'help' for more information
(qemu) q

1b2503fcf7b5932c5a3779ca2ceb92bd403c4ee7 is the first bad commit
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Mon Apr 16 20:18:44 2018 +0200

    mux: fix ctrl-a b again

    Commit fb5e19d2e1472e96d72d5e4d89c20033f8ab345c originally fixed the
    regression, but was inadvertently broken again in merge commit
    2d6752d38d8acda.

    Fixes:
    https://bugs.launchpad.net/qemu/+bug/1654137

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Peter Maydell]

On 17 April 2018 at 19:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi,
>
>>>> Opinions welcome on whether this is a regression fix worth
>>>> putting into rc4.
>>>
>>> It is a regression, but a long standing one - we've been broken for quite
>>> a while since 2.9.0 or even before.
>>>
>>> If we're doing an rc4 anyway I'd suggest including it, but not the end of
>>> the world if it has to go in via -stable given how long we've been broken
>>> for.
>>>
>>
>> Thanks for the clarification; I've applied this to master.
>
> Since this commit, the console on the Malta board stay black...

Thanks for catching that. Since as Dan says the send-break
feature wasn't a regression from 2.11, we're best off reverting
this patch for now, and then we can look at what's happening
for 2.13.

-- PMM

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Peter Maydell]

On 17 April 2018 at 19:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Since this commit, the console on the Malta board stay black...
>
> Before:
> $ qemu-system-mips -M malta -m 512 \
>   -kernel vmlinux-3.2.0-4-4kc-malta -append 'root=/dev/sda1' \
>   -nographic
> [    0.000000] Initializing cgroup subsys cpuset
> [    0.000000] Initializing cgroup subsys cpu
> [    0.000000] Linux version 3.2.0-4-4kc-malta
> (debian-kernel@lists.debian.org) (gcc version 4.6.3 (Debian 4.6.3-14) )
> #1 Debian 3.2.51-1
> [    0.000000] Config serial console: console=ttyS0,38400n8r
> [    0.000000] bootconsole [early0] enabled
> ...
>
> After:
> $ qemu-system-mips -M malta -m 512 \
>   -kernel vmlinux-3.2.0-4-4kc-malta -append 'root=/dev/sda1' \
>   -nographic
> QEMU 2.11.92 monitor - type 'help' for more information
> (qemu) QEMU 2.11.92 monitor - type 'help' for more information
> (qemu) q

I reproed this, and as you can see from the transcripts you
give the problem is that the mux is confused about what the
active console should be. Before the patch, we correctly
start with the serial as the active console and C-a c
switches, but afterwards we start out with the monitor
active and C-a c doesn't switch. Revert pushed to master.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Marc-André Lureau]

Hi

On Tue, Apr 17, 2018 at 11:19 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 17 April 2018 at 19:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Since this commit, the console on the Malta board stay black...
>>
>> Before:
>> $ qemu-system-mips -M malta -m 512 \
>>   -kernel vmlinux-3.2.0-4-4kc-malta -append 'root=/dev/sda1' \
>>   -nographic
>> [    0.000000] Initializing cgroup subsys cpuset
>> [    0.000000] Initializing cgroup subsys cpu
>> [    0.000000] Linux version 3.2.0-4-4kc-malta
>> (debian-kernel@lists.debian.org) (gcc version 4.6.3 (Debian 4.6.3-14) )
>> #1 Debian 3.2.51-1
>> [    0.000000] Config serial console: console=ttyS0,38400n8r
>> [    0.000000] bootconsole [early0] enabled
>> ...
>>
>> After:
>> $ qemu-system-mips -M malta -m 512 \
>>   -kernel vmlinux-3.2.0-4-4kc-malta -append 'root=/dev/sda1' \
>>   -nographic
>> QEMU 2.11.92 monitor - type 'help' for more information
>> (qemu) QEMU 2.11.92 monitor - type 'help' for more information
>> (qemu) q
>
> I reproed this, and as you can see from the transcripts you
> give the problem is that the mux is confused about what the
> active console should be. Before the patch, we correctly
> start with the serial as the active console and C-a c
> switches, but afterwards we start out with the monitor
> active and C-a c doesn't switch. Revert pushed to master.

Thanks Peter for taking care of this "mess" so simply and diligently.

I'll look into it again asap

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Marc-André Lureau]

Hi

On Tue, Apr 17, 2018 at 8:36 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi,
>
>>>> On 16 April 2018 at 19:18, Marc-André Lureau
>>>> <marcandre.lureau@redhat.com> wrote:
>>>>> Commit fb5e19d2e1472e96d72d5e4d89c20033f8ab345c originally fixed the
>>>>> regression, but was inadvertently broken again in merge commit
>>>>> 2d6752d38d8acda.
>>>>>
>>>>> Fixes:
>>>>> https://bugs.launchpad.net/qemu/+bug/1654137
>>>>>
>>>>> Cc: qemu-stable@nongnu.org
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>> ---
>>>>>  chardev/char-mux.c | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/chardev/char-mux.c b/chardev/char-mux.c
>>>>> index 1b925c8dec..6055e76293 100644
>>>>> --- a/chardev/char-mux.c
>>>>> +++ b/chardev/char-mux.c
>>>>> @@ -304,6 +304,7 @@ void mux_set_focus(Chardev *chr, int focus)
>>>>>      }
>>>>>
>>>>>      d->focus = focus;
>>>>> +    chr->be = d->backends[focus];
>>>>>      mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
>>>>>  }
>>>>>
>>>>> --
>>>>> 2.17.0.rc1.36.gcedb63ea2f
>>>>
>>>> Opinions welcome on whether this is a regression fix worth
>>>> putting into rc4.
>>>
>>> It is a regression, but a long standing one - we've been broken for quite
>>> a while since 2.9.0 or even before.
>>>
>>> If we're doing an rc4 anyway I'd suggest including it, but not the end of
>>> the world if it has to go in via -stable given how long we've been broken
>>> for.
>>>
>>
>> Thanks for the clarification; I've applied this to master.
>
> Since this commit, the console on the Malta board stay black...
>
> Before:
> $ qemu-system-mips -M malta -m 512 \
>   -kernel vmlinux-3.2.0-4-4kc-malta -append 'root=/dev/sda1' \
>   -nographic
> [    0.000000] Initializing cgroup subsys cpuset
> [    0.000000] Initializing cgroup subsys cpu
> [    0.000000] Linux version 3.2.0-4-4kc-malta
> (debian-kernel@lists.debian.org) (gcc version 4.6.3 (Debian 4.6.3-14) )
> #1 Debian 3.2.51-1
> [    0.000000] Config serial console: console=ttyS0,38400n8r
> [    0.000000] bootconsole [early0] enabled
> ...
>
> After:
> $ qemu-system-mips -M malta -m 512 \
>   -kernel vmlinux-3.2.0-4-4kc-malta -append 'root=/dev/sda1' \
>   -nographic
> QEMU 2.11.92 monitor - type 'help' for more information
> (qemu) QEMU 2.11.92 monitor - type 'help' for more information
> (qemu) q
>

In  commit cd9526ab7c04f2c32c63340b04401f6ed25682b9
Author: Philippe Mathieu-Daudé <f4bug@amsat.org>
Date:   Thu Mar 8 23:39:32 2018 +0100

    hw/isa/superio: Factor out the serial code from pc87312.c

You changed from:

-            if (chr == NULL) {
-                snprintf(name, sizeof(name), "ser%d", i);
-                chr = qemu_chr_new(name, "null");
-            }

to:

+            if (chr == NULL || chr->be) {
+                name = g_strdup_printf("discarding-serial%d", i);
+                chr = qemu_chr_new(name, "null");
+            } else {
+                name = g_strdup_printf("serial%d", i);
+            }

Why do you check if chr->be ? In case of a mux chardev, it may already
have an active frontend (yeah be is CharBackend which is the frontend,
I still can't grasp that either, please Paolo change your mind! ;).
And this is the job for a mux chardev, handling several frontends.

Furthermore, there is a better API for checking the limit of a
chardev: qemu_chr_is_busy(). Accessing char/fe internal structure
should warn you something could be done better.

> 1b2503fcf7b5932c5a3779ca2ceb92bd403c4ee7 is the first bad commit
> Author: Marc-André Lureau <marcandre.lureau@redhat.com>
> Date:   Mon Apr 16 20:18:44 2018 +0200
>
>     mux: fix ctrl-a b again
>
>     Commit fb5e19d2e1472e96d72d5e4d89c20033f8ab345c originally fixed the
>     regression, but was inadvertently broken again in merge commit
>     2d6752d38d8acda.
>
>     Fixes:
>     https://bugs.launchpad.net/qemu/+bug/1654137
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Paolo Bonzini]

On 18/04/2018 12:36, Marc-André Lureau wrote:
> 
> +            if (chr == NULL || chr->be) {
> +                name = g_strdup_printf("discarding-serial%d", i);
> +                chr = qemu_chr_new(name, "null");
> +            } else {
> +                name = g_strdup_printf("serial%d", i);
> +            }
> 
> Why do you check if chr->be ? In case of a mux chardev, it may already
> have an active frontend (yeah be is CharBackend which is the frontend,
> I still can't grasp that either, please Paolo change your mind! ;).

CharBackend is not the frontend, it is *used* by the front-end.  It is
the qemu_chr_* functions that are named wrong (they're named according
to the user rather than the recipient).

Paolo

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Marc-André Lureau]

Hi

On Wed, Apr 18, 2018 at 12:55 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 18/04/2018 12:36, Marc-André Lureau wrote:
>>
>> +            if (chr == NULL || chr->be) {
>> +                name = g_strdup_printf("discarding-serial%d", i);
>> +                chr = qemu_chr_new(name, "null");
>> +            } else {
>> +                name = g_strdup_printf("serial%d", i);
>> +            }
>>
>> Why do you check if chr->be ? In case of a mux chardev, it may already
>> have an active frontend (yeah be is CharBackend which is the frontend,
>> I still can't grasp that either, please Paolo change your mind! ;).
>
> CharBackend is not the frontend, it is *used* by the front-end.  It is
> the qemu_chr_* functions that are named wrong (they're named according
> to the user rather than the recipient).

If I follow you and the naming, you have this in mind:

- Chardev: stdio, mux, ringbuf, pty, file, null etc..
- CharBackend: the "user" end
- frontend: the "user"

It is quite confusing to me that CharBackend is for the "user"
frontend, and the backend of Chardev. You have to switch your mind
depending on the context or the point of view.

 I'd rather use that terminology:

- ChardevBackend: stdio, mux, ringbuf, pty, file, null etc..
- CharFrontend: the "user" end
- frontend the "user"

This way, there is only one direction from backend to frontend
(regardless of the point of view from chardev to frontend)

And rename struct and functions accordingly.

Do you have another proposal to simplify the current situation?

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Paolo Bonzini]

On 18/04/2018 13:35, Marc-André Lureau wrote:
>> CharBackend is not the frontend, it is *used* by the front-end.  It is
>> the qemu_chr_* functions that are named wrong (they're named according
>> to the user rather than the recipient).
> If I follow you and the naming, you have this in mind:
> 
> - Chardev: stdio, mux, ringbuf, pty, file, null etc..
> - CharBackend: the "user" end
> - frontend: the "user"

The frontend is the device, the monitor, etc.  The backend is how the
frontend sees a Chardev, it never talks to it directly.

Perhaps the confusing part is that the backend is also how the Chardev
talks to the frontend?

Paolo

> It is quite confusing to me that CharBackend is for the "user"
> frontend, and the backend of Chardev.
>
> You have to switch your mind
> depending on the context or the point of view.
> 
>  I'd rather use that terminology:
> 
> - ChardevBackend: stdio, mux, ringbuf, pty, file, null etc..
> - CharFrontend: the "user" end
> - frontend the "user"
> 
> This way, there is only one direction from backend to frontend
> (regardless of the point of view from chardev to frontend)

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Marc-André Lureau]

Hi

On Wed, Apr 18, 2018 at 1:55 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 18/04/2018 13:35, Marc-André Lureau wrote:
>>> CharBackend is not the frontend, it is *used* by the front-end.  It is
>>> the qemu_chr_* functions that are named wrong (they're named according
>>> to the user rather than the recipient).
>> If I follow you and the naming, you have this in mind:
>>
>> - Chardev: stdio, mux, ringbuf, pty, file, null etc..
>> - CharBackend: the "user" end
>> - frontend: the "user"
>
> The frontend is the device, the monitor, etc.

yes, I should have listed it for clarity

> The backend is how the
> frontend sees a Chardev, it never talks to it directly.


Ok, but why not call CharFrontend, since it's the frontend interface
to the chardev? (which we call the backend, by extension, and because
many functions are named *_be_* )

> Perhaps the confusing part is that the backend is also how the Chardev
> talks to the frontend?

It's less confusing, because that part is mostly internal to char.c
implementation.

But rather than:

static void chr_be_event(Chardev *s, int event)
{
    CharBackend *be = s->be;
...
    be->chr_event(be->opaque, event);
}

I would rather have:

static void chr_be_event(Chardev *s, int event)
{
    CharFrontend *fe = s->fe;
...
    fe->chr_event(be->opaque, event);
}

This would not only mean that we clearly associate CharBackend with
the frontend, but also the chardev code becomes clearer, sending an
event on the chardev (be), forwards it to the frontend side... (that
is so much clearer to me)

>
> Paolo
>
>> It is quite confusing to me that CharBackend is for the "user"
>> frontend, and the backend of Chardev.
>>
>> You have to switch your mind
>> depending on the context or the point of view.
>>
>>  I'd rather use that terminology:
>>
>> - ChardevBackend: stdio, mux, ringbuf, pty, file, null etc..
>> - CharFrontend: the "user" end
>> - frontend the "user"
>>
>> This way, there is only one direction from backend to frontend
>> (regardless of the point of view from chardev to frontend)
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Paolo Bonzini]

On 18/04/2018 14:22, Marc-André Lureau wrote:
>>> - Chardev: stdio, mux, ringbuf, pty, file, null etc..
>>> - CharBackend: the "user" end
>>> - frontend: the "user"
>> The frontend is the device, the monitor, etc.
> yes, I should have listed it for clarity
> 
>> The backend is how the
>> frontend sees a Chardev, it never talks to it directly.
> 
> Ok, but why not call CharFrontend, since it's the frontend interface
> to the chardev? (which we call the backend, by extension, and because
> many functions are named *_be_* )

The frontend interface is not the same thing as the frontend, though.
The frontend interface is the backend.

> It's less confusing, because that part is mostly internal to char.c
> implementation.
> 
> But rather than:
> 
> static void chr_be_event(Chardev *s, int event)
> {
>     CharBackend *be = s->be;
> ...
>     be->chr_event(be->opaque, event);
> }
> 
> I would rather have:
> 
> static void chr_be_event(Chardev *s, int event)
> {
>     CharFrontend *fe = s->fe;
> ...
>     fe->chr_event(fe->opaque, event);
> }

What I would rather have instead is this:

   static void qemu_chr_be_send_event(CharBackend *be, int event)
   {
       if (be->frontend_ops && be->frontend_ops->chr_event) {
           /* Or: be->frontend->chr_event(be->frontend, event);
           be->frontend_ops->chr_event(be->frontend_ops->opaque, event);
       }
   }

   static void chr_send_event(Chardev *s, int event)
   {
       qemu_chr_be_send_event(s->be, event);
   }

(and likewise qemu_chr_be_can_write asks be->frontend_ops->chr_can_read,
qemu_chr_be_write_impl invokes be->frontend_ops->chr_read).  Then the
tx<->rx switch is clear from accessing frontend_ops.

> This would not only mean that we clearly associate CharBackend with
> the frontend, but also the chardev code becomes clearer, sending an
> event on the chardev (be), forwards it to the frontend side... (that
> is so much clearer to me)

The problem I have with calling it CharFrontend, is that for example
"int tag" has nothing to do with the front end.

Paolo

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Marc-André Lureau]

Hi

On Wed, Apr 18, 2018 at 3:47 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
> On 18/04/2018 14:22, Marc-André Lureau wrote:
>>>> - Chardev: stdio, mux, ringbuf, pty, file, null etc..
>>>> - CharBackend: the "user" end
>>>> - frontend: the "user"
>>> The frontend is the device, the monitor, etc.
>> yes, I should have listed it for clarity
>>
>>> The backend is how the
>>> frontend sees a Chardev, it never talks to it directly.
>>
>> Ok, but why not call CharFrontend, since it's the frontend interface
>> to the chardev? (which we call the backend, by extension, and because
>> many functions are named *_be_* )
>
> The frontend interface is not the same thing as the frontend, though.
> The frontend interface is the backend.
>
>> It's less confusing, because that part is mostly internal to char.c
>> implementation.
>>
>> But rather than:
>>
>> static void chr_be_event(Chardev *s, int event)
>> {
>>     CharBackend *be = s->be;
>> ...
>>     be->chr_event(be->opaque, event);
>> }
>>
>> I would rather have:
>>
>> static void chr_be_event(Chardev *s, int event)
>> {
>>     CharFrontend *fe = s->fe;
>> ...
>>     fe->chr_event(fe->opaque, event);
>> }
>
> What I would rather have instead is this:
>
>    static void qemu_chr_be_send_event(CharBackend *be, int event)
>    {
>        if (be->frontend_ops && be->frontend_ops->chr_event) {
>            /* Or: be->frontend->chr_event(be->frontend, event);
>            be->frontend_ops->chr_event(be->frontend_ops->opaque, event);
>        }
>    }
>
>    static void chr_send_event(Chardev *s, int event)
>    {
>        qemu_chr_be_send_event(s->be, event);
>    }

Yeah I think I got your point. That looks more complicated than my
proposal though. This chardev->be->frontend dance is unnecessarily
complex for me. I would go from be->fe directly.
>
> (and likewise qemu_chr_be_can_write asks be->frontend_ops->chr_can_read,
> qemu_chr_be_write_impl invokes be->frontend_ops->chr_read).  Then the
> tx<->rx switch is clear from accessing frontend_ops.
>
>> This would not only mean that we clearly associate CharBackend with
>> the frontend, but also the chardev code becomes clearer, sending an
>> event on the chardev (be), forwards it to the frontend side... (that
>> is so much clearer to me)
>
> The problem I have with calling it CharFrontend, is that for example
> "int tag" has nothing to do with the front end.
>

well, if it's just that, we may find a way to move it somewhere else,
or simply add some comment that those fields are not to be manipulated
by the frontend directly (well, all fields I suppose, in theory).

> Paolo



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Peter Maydell]

On 18 April 2018 at 11:36, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> In  commit cd9526ab7c04f2c32c63340b04401f6ed25682b9
> Author: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Date:   Thu Mar 8 23:39:32 2018 +0100
>
>     hw/isa/superio: Factor out the serial code from pc87312.c
>
> You changed from:
>
> -            if (chr == NULL) {
> -                snprintf(name, sizeof(name), "ser%d", i);
> -                chr = qemu_chr_new(name, "null");
> -            }
>
> to:
>
> +            if (chr == NULL || chr->be) {
> +                name = g_strdup_printf("discarding-serial%d", i);
> +                chr = qemu_chr_new(name, "null");
> +            } else {
> +                name = g_strdup_printf("serial%d", i);
> +            }
>
> Why do you check if chr->be ?

After commit 12051d82f004024 none of this should be necessary --
the code should handle chr being NULL without needing to
create a fake null chardev to throw away output.

> In case of a mux chardev, it may already
> have an active frontend (yeah be is CharBackend which is the frontend,
> I still can't grasp that either, please Paolo change your mind! ;).

I agree with Marc-André that the terminology for our chardev
code is hopelessly confusing. The things that are logically
backends (file, stdio, etc) are called "chardevs", and the
thing that is called the CharBackend is, well, I don't know
what it is. include/chardev/char.h has a helpful comment:
     /* character device */
     typedef struct CharBackend CharBackend;

The doc comments for functions like qemu_chr_new() that return
a Chardev* say they "create a new character backend".

And then there's ChardevBackend, which is something else again.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Paolo Bonzini]

On 18/04/2018 14:06, Peter Maydell wrote:
> 
>> In case of a mux chardev, it may already
>> have an active frontend (yeah be is CharBackend which is the frontend,
>> I still can't grasp that either, please Paolo change your mind! ;).
> I agree with Marc-André that the terminology for our chardev
> code is hopelessly confusing. The things that are logically
> backends (file, stdio, etc) are called "chardevs", and the
> thing that is called the CharBackend is, well, I don't know
> what it is. include/chardev/char.h has a helpful comment:
>      /* character device */
>      typedef struct CharBackend CharBackend;
> 
> The doc comments for functions like qemu_chr_new() that return
> a Chardev* say they "create a new character backend".
> 
> And then there's ChardevBackend, which is something else again.

Since CharBackend was named like that for consistency with BlockBackend,
ChardevBackend could be changed to ChardevOptions and
CHARDEV_BACKEND_KIND_* to CHARDEV_DRIVER_*.  That would be a good idea
indeed.

Paolo

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Philippe Mathieu-Daudé]

Hi Marc-André,

On 04/18/2018 07:36 AM, Marc-André Lureau wrote:
> In  commit cd9526ab7c04f2c32c63340b04401f6ed25682b9
> Author: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Date:   Thu Mar 8 23:39:32 2018 +0100
> 
>     hw/isa/superio: Factor out the serial code from pc87312.c
> 
> You changed from:
> 
> -            if (chr == NULL) {
> -                snprintf(name, sizeof(name), "ser%d", i);
> -                chr = qemu_chr_new(name, "null");
> -            }
> 
> to:
> 
> +            if (chr == NULL || chr->be) {
> +                name = g_strdup_printf("discarding-serial%d", i);
> +                chr = qemu_chr_new(name, "null");
> +            } else {
> +                name = g_strdup_printf("serial%d", i);
> +            }
> 
> Why do you check if chr->be ? In case of a mux chardev, it may already
> have an active frontend (yeah be is CharBackend which is the frontend,
> I still can't grasp that either, please Paolo change your mind! ;).
> And this is the job for a mux chardev, handling several frontends.

I was afraid this refactor could be related.

IIRC I had troubles running "qemu-system-alpha -append console=srm" but
I tried again with/without the chr->be check and it works fine...

Anyway the new code is buggy, this is wrong (simplified):

 if (chr->be) chr = qemu_chr_new(name, "null");

> 
> Furthermore, there is a better API for checking the limit of a
> chardev: qemu_chr_is_busy(). Accessing char/fe internal structure
> should warn you something could be done better.

This method is static (unexposed).

Re: [Qemu-devel] [PATCH] mux: fix ctrl-a b again

[Philippe Mathieu-Daudé]

On 04/18/2018 11:32 AM, Philippe Mathieu-Daudé wrote:
> Hi Marc-André,
> 
> On 04/18/2018 07:36 AM, Marc-André Lureau wrote:
>> In  commit cd9526ab7c04f2c32c63340b04401f6ed25682b9
>> Author: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Date:   Thu Mar 8 23:39:32 2018 +0100
>>
>>     hw/isa/superio: Factor out the serial code from pc87312.c
>>
>> You changed from:
>>
>> -            if (chr == NULL) {
>> -                snprintf(name, sizeof(name), "ser%d", i);
>> -                chr = qemu_chr_new(name, "null");
>> -            }
>>
>> to:
>>
>> +            if (chr == NULL || chr->be) {
>> +                name = g_strdup_printf("discarding-serial%d", i);
>> +                chr = qemu_chr_new(name, "null");
>> +            } else {
>> +                name = g_strdup_printf("serial%d", i);
>> +            }
>>
>> Why do you check if chr->be ? In case of a mux chardev, it may already
>> have an active frontend (yeah be is CharBackend which is the frontend,
>> I still can't grasp that either, please Paolo change your mind! ;).
>> And this is the job for a mux chardev, handling several frontends.
> 
> I was afraid this refactor could be related.
> 
> IIRC I had troubles running "qemu-system-alpha -append console=srm" but
> I tried again with/without the chr->be check and it works fine...
> 
> Anyway the new code is buggy, this is wrong (simplified):
> 
>  if (chr->be) chr = qemu_chr_new(name, "null");

Indeed with 1b2503fcf7b59 applied and removing the 'chr->be' check I can
successfully boot my mips/alpha images.

-- >8 --
diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
@@ -43,7 +43,7 @@ static void isa_superio_realize(DeviceState *dev,
Error **errp)
         if (!k->parallel.is_enabled || k->parallel.is_enabled(sio, i)) {
             /* FIXME use a qdev chardev prop instead of parallel_hds[] */
             chr = parallel_hds[i];
-            if (chr == NULL || chr->be) {
+            if (chr == NULL) {
                 name = g_strdup_printf("discarding-parallel%d", i);
                 chr = qemu_chr_new(name, "null");
             } else {
@@ -83,7 +83,7 @@ static void isa_superio_realize(DeviceState *dev,
Error **errp)
         if (!k->serial.is_enabled || k->serial.is_enabled(sio, i)) {
             /* FIXME use a qdev chardev prop instead of serial_hds[] */
             chr = serial_hds[i];
-            if (chr == NULL || chr->be) {
+            if (chr == NULL) {
                 name = g_strdup_printf("discarding-serial%d", i);
                 chr = qemu_chr_new(name, "null");
             } else {
--
2.17.0