Re: [Qemu-devel] [PATCH 1/1] char: Add a QemuChrHandlers struct to initialise chardev handlers

[Anthony Liguori]

On 02/10/2012 12:08 PM, Kevin Wolf wrote:
> Am 10.02.2012 18:22, schrieb Anthony Liguori:
> > On 02/10/2012 11:09 AM, Amit Shah wrote:
> > > On (Fri) 10 Feb 2012 [07:28:19], Anthony Liguori wrote:
> > > > On 02/10/2012 07:19 AM, Amit Shah wrote:
> > > > > Instead of passing each handler in the qemu_add_handlers() function,
> > > > > create a struct of handlers that can be passed to the function instead.
> > > > >
> > > > > Signed-off-by: Amit Shah<address@hidden>
> > > >
> > > > Why?
> > >
> > > It's a good cleanup.
> > >
> > > > It's not a win in terms of code size.  If you plan on introducing
> > > > additional handlers, perhaps you should include this in that series
> > > > where it's more appropriately justified.
> > > >
> > > > As a change on it's own, it doesn't seem to make a lot of sense.
> > >
> > > Makes the code much easier to look at.  Can't really compare on code
> > > size, since there's zero change in the resulting binary, but the code
> > > just becomes more readable and manageable.
> >
> > It's not more readable IMHO.  You've taken function call arguments from the
> > place they naturally belong (in the function call) and placed them somewhere 
> > else.
> >
> > More importantly, this isn't a pattern we use in QEMU anywhere.
>
> The obvious next step is to replace
> CharDriverState.chr_can_read/read/event with a CharDriverState.ops that
> refers to a CharDriverHandler struct, and suddenly you have a pattern
> that is used a lot in QEMU (and that indeed increases readability in my
> opinion).

I actually think that you need to change chr_can_read/read/event to take 
CharDriverHandler as the first argument instead of CharDriverState, then drop 
the handle_opaque and refactor callers appropriately.

But then at this stage, we should drop NULL handlers as a disconnect mechanism 
and switch to a qemu_chr_fe_connect(), qemu_chr_fe_disconnect().  And that 
indicates that we should call it CharFrontEnd instead of CharDriverHandler.

So in such a series, I don't think this patch would even survive.

Regards,

Anthony Liguori

> Kevin