| Message ID | 20180416181844.7851-1-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | mux: fix ctrl-a b again | expand |
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
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
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
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
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
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
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
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 >
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
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?
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)
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
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) >
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
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
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
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).
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
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); }
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(+)