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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On 01/12/2011 12:07 AM, Amit Shah wrote:
> On (Tue) Jan 11 2011 [17:13:15], Blue Swirl wrote:
>>> +static QemuChrHandlers gdb_handlers = {
>>> +    .fd_can_read = gdb_chr_can_receive,
>>> +    .fd_read = gdb_chr_receive,
>>> +    .fd_event = gdb_chr_event,
>>> +};
>>
>> These structures should be const.
>
> Hm, I had that but looks like it got lost in some rebase.  Added again.
>
>>> @@ -190,15 +190,19 @@ void qemu_chr_send_event(CharDriverState *s, int event)
>>>          s->chr_send_event(s, event);
>>>   }
>>>
>>> +static QemuChrHandlers null_handlers = {
>>> +    /* All handlers are initialised to NULL */
>>> +};
>>> +
>>>   void qemu_chr_add_handlers(CharDriverState *s,
>>> -                           IOCanReadHandler *fd_can_read,
>>> -                           IOReadHandler *fd_read,
>>> -                           IOEventHandler *fd_event,
>>> -                           void *opaque)
>>> -{
>>> -    s->chr_can_read = fd_can_read;
>>> -    s->chr_read = fd_read;
>>> -    s->chr_event = fd_event;
>>> +                           QemuChrHandlers *handlers, void *opaque)
>>> +{
>>
>> Here we could also check if (!s) and return if so. This would simplify
>> the callers a bit.
>
> Simplified in what way?

I assume for reducing the need to have to check s->chr != NULL everytime 
beforehand. It's safer and would save a lot on repetitive code as well.

>
> 		Amit
>