Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define

[Yang Hongyang <yanghy@cn.fujitsu.com>]

On 09/02/2015 09:02 PM, Stefan Hajnoczi wrote:
> On Wed, Sep 02, 2015 at 09:49:18AM +0800, Yang Hongyang wrote:
>> On 09/01/2015 10:43 PM, Stefan Hajnoczi wrote:
>>> On Tue, Sep 01, 2015 at 05:06:18PM +0800, Yang Hongyang wrote:
>>>> This will be used by the next patch in this series.
>>>>
>>>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>   include/net/queue.h | 19 +++++++++++++++++++
>>>>   net/queue.c         | 19 -------------------
>>>>   2 files changed, 19 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/include/net/queue.h b/include/net/queue.h
>>>> index fc02b33..1d65e47 100644
>>>> --- a/include/net/queue.h
>>>> +++ b/include/net/queue.h
>>>> @@ -31,6 +31,25 @@ typedef struct NetQueue NetQueue;
>>>>
>>>>   typedef void (NetPacketSent) (NetClientState *sender, ssize_t ret);
>>>>
>>>> +struct NetPacket {
>>>> +    QTAILQ_ENTRY(NetPacket) entry;
>>>> +    NetClientState *sender;
>>>> +    unsigned flags;
>>>> +    int size;
>>>> +    NetPacketSent *sent_cb;
>>>> +    uint8_t data[0];
>>>> +};
>>>> +
>>>> +struct NetQueue {
>>>> +    void *opaque;
>>>> +    uint32_t nq_maxlen;
>>>> +    uint32_t nq_count;
>>>> +
>>>> +    QTAILQ_HEAD(packets, NetPacket) packets;
>>>> +
>>>> +    unsigned delivering:1;
>>>> +};
>>>> +
>>>
>>> Why is it necessary to expose both structs?
>>
>> In next patch, we add a API to pass the packet to next filter:
>> ssize_t qemu_netfilter_pass_to_next(NetFilterState *nf, NetPacket *packet);
>> which will access the internals of NetPacket.
>>
>> and in buffer filter, we need the NetQueue to Queue packets, and also to
>> access queue->packets(pass this packets to next filter).
>
> Can you do these things by introducing net queue APIs instead of
> exposing the struct?
>
> It's a C anti-pattern to expose structs.  It makes code complex because
> now the net queue code no longer contains all the assumptions about
> NetQueue/NetPacket fields.  People modifying the code now also need to
> go into the net filter code to figure out how this struct works.
>
> Exposing the fields also leads to code duplication since every user will
> reimplement similar stuff if they access the fields directly instead of
> using a function interface.

I can't think of a interface that no need to access the NetPacket internals
to archive the needs. Except I do not reuse the NetPacket and NetQueue,
implement a Queue myself. But that will be more complex.
So in order to not expose those structs, I shall add lots of public get
functions to use these internals, like:
Qemu_NetQueue_get_xxx
Qemu_NetPacket_get_xxx

I'd be very happy if you could give me a better suggestion on this.

Thanks!

>
> Stefan
> .
>

-- 
Thanks,
Yang.