Re: [Qemu-devel] [PATCH 08/54] char: allocate CharDriverState as a single object

[Eric Blake]

On 01/04/2017 03:09 PM, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jan 4, 2017 at 9:26 PM Eric Blake <address@hidden> wrote:
> 
>> On 12/12/2016 04:42 PM, Marc-André Lureau wrote:
>>> Use a single allocation for CharDriverState, this avoids extra
>>> allocations & pointers, and is a step towards more object-oriented
>>> CharDriver.
>>>
>>> Gtk console is a bit peculiar, gd_vc_chr_set_echo
>>
>> Truncated paragraph?
>>
> 
> yes, fixed
> 

>>> @@ -270,7 +270,7 @@ static int baum_deferred_init(BaumDriverState *baum)
>>>  /* The serial port can receive more of our data */
>>>  static void baum_accept_input(struct CharDriverState *chr)
>>>  {
>>> -    BaumDriverState *baum = chr->opaque;
>>> +    BaumDriverState *baum = (BaumDriverState *)chr;
>>
>> It might be a little nicer to use:
>>
>> BaumDriverState *baum = container_of(chr, BaumDriverState, parent);
>>
>>
> The follow-up of the series converts it to the more appropriate
>  BaumChardev *baum = BAUM_CHARDEV(obj);
> 
> So considering that this is temporary commit, do I have to change that?

Ah, nice. So BAUM_CHARDEV(obj) will be a nice wrapper around
container_of(). I can live with it being temporary (although a note in
the commit message can't hurt).

> 
> to avoid a cast and work even if the members are rearranged within the
>> structure. You can even write a one-line helper function to hide the
>> cast behind a more legible semantic (for example, see to_qov() in
>> qobject-output-visitor.c).

My example of to_qov() is matched by your BAUM_CHARDEV() macro.

>> My biggest gripe is the number of casts that could be container_of()
>> instead; but otherwise it looks like a sane conversion.
>>
>>
> thanks, the goal is to get rid of them, but not in this commit :) See
> "chardev: qom-ify".

That's what I get for only reviewing the series one patch at a time. I'm
fine with temporary and partial hacks, but calling them out as such
makes review easier if we know it's going to improve later.

Since I think you've answered my questions, then with an improved commit
message, v2 can add:

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature