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

[Marc-André Lureau]

Hi

On Wed, Apr 18, 2018 at 3:47 PM, Paolo Bonzini <address@hidden> 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