Re: [Qemu-devel] [PATCH v4 05/11] netfilter: hook packets before net queue send

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

On 08/06/2015 03:37 PM, Jason Wang wrote:
>
>
> On 08/06/2015 03:28 PM, Yang Hongyang wrote:
>>
>>
>> On 08/06/2015 03:13 PM, Jason Wang wrote:
>>>
>>>
>>> On 08/04/2015 04:30 PM, Yang Hongyang wrote:
>>>> Capture packets that will be sent.
>>>>
>>>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>>>> ---
>>>>    net/net.c | 66
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 65 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/net.c b/net/net.c
>>>> index 03b2296..efccd56 100644
>>>> --- a/net/net.c
>>>> +++ b/net/net.c
>>>> @@ -569,6 +569,42 @@ int qemu_can_send_packet(NetClientState *sender)
>>>>        return 1;
>>>>    }
>>>>
>>>> +static ssize_t filter_receive_iov(NetClientState *nc, int chain,
>>>> +                                  NetClientState *sender,
>>>> +                                  unsigned flags,
>>>> +                                  const struct iovec *iov,
>>>> +                                  int iovcnt)
>>>> +{
>>>> +    ssize_t ret = 0;
>>>> +    NetFilterState *nf = NULL;
>>>> +    ssize_t size = iov_size(iov, iovcnt);
>>>> +
>>>> +    QTAILQ_FOREACH(nf, &nc->filters, next) {
>>>> +        if (nf->chain == chain || nf->chain == NET_FILTER_ALL) {
>>>> +            ret = nf->info->receive_iov(nf, sender, flags, iov,
>>>> iovcnt);
>>>> +            if (ret == size) {
>>>> +                return ret;
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static ssize_t filter_receive(NetClientState *nc, int chain,
>>>> +                              NetClientState *sender,
>>>> +                              unsigned flags,
>>>> +                              const uint8_t *data,
>>>> +                              size_t size)
>>>> +{
>>>> +    struct iovec iov = {
>>>> +        .iov_base = (void *)data,
>>>> +        .iov_len = size
>>>> +    };
>>>> +
>>>> +    return filter_receive_iov(nc, chain, sender, flags, &iov, 1);
>>>> +}
>>>> +
>>>>    ssize_t qemu_deliver_packet(NetClientState *sender,
>>>>                                unsigned flags,
>>>>                                const uint8_t *data,
>>>> @@ -640,6 +676,7 @@ static ssize_t
>>>> qemu_send_packet_async_with_flags(NetClientState *sender,
>>>>                                                     NetPacketSent
>>>> *sent_cb)
>>>>    {
>>>>        NetQueue *queue;
>>>> +    int ret;
>>>>
>>>>    #ifdef DEBUG_NET
>>>>        printf("qemu_send_packet_async:\n");
>>>> @@ -650,6 +687,18 @@ static ssize_t
>>>> qemu_send_packet_async_with_flags(NetClientState *sender,
>>>>            return size;
>>>>        }
>>>>
>>>> +    /* Let filters handle the packet first */
>>>> +    ret = filter_receive(sender, NET_FILTER_OUT, sender, flags,
>>>> buf, size);
>>>> +    if (ret == size) {
>>>> +        return size;
>>>> +    }
>>>> +
>>>> +    ret = filter_receive(sender->peer, NET_FILTER_IN,
>>>> +                         sender, flags, buf, size);
>>>> +    if (ret == size) {
>>>> +        return size;
>>>> +    }
>>>
>>> I think we can just trust the return value of filter_receive(), no need
>>> to check it against iov_size.
>>
>> You mean:
>>    if (ret)
>>        return ret;
>> ? We need to check ret however.
>
> Yes, but the problem is you check it against size. What if
> filter_receive() returns a value which is greater than 0 but smaller
> than size. If this could not happen, probably no need to check.  Not a
> must but better to change this.

Ok, will fix, thank you.

>
>>
>>>
>>> Also sent_cb was lost track if the packet was held by filter which seems
>>> wrong.
>>
>> So we need to extend the params of filter's receive_iov API, add a
>> sent_cb
>> param.
>
> Yes.
>
>> BTW, it seems that sent_cb will only been called when packet is
>> queueed and then sent, if the packet being sent directly sent_cb won't
>> be called? am I right? or I missed somthing from the code...
>
> Yes, but if for some reason it couldn't. Sent cb is usually used to let
> main loop to re-poll network backend.

Yes, this matches what I saw. It is kind of strange though, a provided
sent_cb won't be called when a packet been sent directly...

>
>>
>>>
>>> [...]
>>> .
>>>
>>
>
> .
>

-- 
Thanks,
Yang.