Re: [Qemu-devel] [PATCH 09/12] netfilter: add a netbuffer filter

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

On 07/30/2015 01:13 PM, Jason Wang wrote:
[...]
>> +
>> +#include "net/filter.h"
>> +#include "net/queue.h"
>> +#include "filters.h"
>> +#include "qemu-common.h"
>> +#include "qemu/error-report.h"
>> +
>> +typedef struct FILTERBUFFERState {
>> +    NetFilterState nf;
>> +    NetClientState dummy; /* used to send buffered packets */
>
> Why need this? Couldn't we just infer this from NetFilterState?

Because we use existing API qemu_send_packet_async/raw to send
packet, it takes an NetClientState as the first argument sender,
and use sender->peer->incoming_queue as the dest queue, so in order to
make this API work, we need to use this dummy NC and init it's
peer to our dest(which is the network backend)
Another way is to call qemu_net_queue_send(netdev->incoming_queue,...)
directly, we still need a NetClientState *sender param, can not
use NetFilterState.
This dummy NC also been checked in filter_buffer_receive to avoid buffering
packet been sent by ourself.

>
>> +    NetQueue *incoming_queue;
>> +    NetQueue *inflight_queue;
>> +} FILTERBUFFERState;
>> +
>> +static void packet_send_completed(NetClientState *nc, ssize_t len)
>> +{
>> +    return;
>> +}
>> +
>> +static void filter_buffer_flush(NetFilterState *nf)
>> +{
>> +    FILTERBUFFERState *s = DO_UPCAST(FILTERBUFFERState, nf, nf);
>> +    NetQueue *queue = s->inflight_queue;
>> +    NetPacket *packet;
>> +
>> +    while (queue && !QTAILQ_EMPTY(&queue->packets)) {
>> +        packet = QTAILQ_FIRST(&queue->packets);
>> +        QTAILQ_REMOVE(&queue->packets, packet, entry);
>> +        queue->nq_count--;
>> +
>> +        if (packet->flags & QEMU_NET_PACKET_FLAG_RAW) {
>> +            qemu_send_packet_raw(&s->dummy, packet->data, packet->size);
>> +        } else {
>> +            qemu_send_packet_async(&s->dummy, packet->data, packet->size,
>> +                                   packet->sent_cb);
>> +        }
>> +
>> +        /*
>> +         * now that we pass the packet to sender->peer->incoming_queue, we
>> +         * don't care the reture value here, because the peer's queue will
>> +         * take care of this packet
>> +         */
>> +        g_free(packet);
>> +    }
>> +
>> +    if (queue && QTAILQ_EMPTY(&queue->packets)) {
>
> Under what condition queue->packet could be not empty here?

You're right, this check is nonsense. can be dropped.

> And I don't
> get the point that why a inflight_queue is needed.

maybe squash the interval patch here will make it clear?

>
>> +        g_free(queue);
>> +        s->inflight_queue = NULL;
>> +    }
>> +}
>> +
>> +/* filter APIs */
>> +static ssize_t filter_buffer_receive(NetFilterState *nf,
>> +                                     NetClientState *sender,
>> +                                     unsigned flags,
>> +                                     const uint8_t *data,
>> +                                     size_t size)
>> +{
>> +    FILTERBUFFERState *s = DO_UPCAST(FILTERBUFFERState, nf, nf);
>> +    NetQueue *queue = s->incoming_queue;
>> +
>> +    if (!sender->info) {
>> +        /* This must be a dummy NetClientState, do nothing */
>> +        return 0;
>> +    }
>> +
>> +    if (sender->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
>> +        /* we only buffer guest output packets */
>> +        qemu_net_queue_append(queue, sender, flags, data, size,
>> +                              packet_send_completed);
>
> This may brings some confusion for user. Since the name is 'netbuffer'.
> Maybe we can change the filter to be ingress or out or both? Like:
>
> -device virtio-net-pci,id=virtio0
> -netfilter buffer,id=filter0,dev=virtio0,interval=1000,type=out
>
> Then we can just try to enqueue the packet when virtio-net-pci is sender?
>
>> +        /* Now that we have buffered the packet, return sucess */
>> +        return size;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void filter_buffer_cleanup(NetFilterState *nf)
>> +{
>> +    FILTERBUFFERState *s = DO_UPCAST(FILTERBUFFERState, nf, nf);
>> +
>> +    /* flush inflight packets */
>> +    filter_buffer_flush(nf);
>> +    /* flush incoming packets */
>> +    s->inflight_queue = s->incoming_queue;
>> +    s->incoming_queue = NULL;
>> +    filter_buffer_flush(nf);
>> +
>> +    return;
>> +}
>> +
>> +
>> +static NetFilterInfo net_filter_buffer_info = {
>> +    .type = NET_FILTER_OPTIONS_KIND_BUFFER,
>> +    .size = sizeof(FILTERBUFFERState),
>> +    .receive = filter_buffer_receive,
>> +    .cleanup = filter_buffer_cleanup,
>> +};
>> +
>> +int net_init_filter_buffer(const NetFilterOptions *opts, const char *name,
>> +                           NetClientState *netdev, Error **errp)
>> +{
>> +    NetFilterState *nf;
>> +    FILTERBUFFERState *s;
>> +
>> +    assert(opts->kind == NET_FILTER_OPTIONS_KIND_BUFFER);
>> +
>> +    nf = qemu_new_net_filter(&net_filter_buffer_info, netdev, "buffer", name);
>> +    s = DO_UPCAST(FILTERBUFFERState, nf, nf);
>> +    /*
>> +     * we need the dummy NetClientState to send packets in order to avoid
>> +     * receive packets again.
>> +     * we are buffering guest output packets, our buffered packets should be
>> +     * sent to real network backend, so dummy's peer should be that backend.
>> +     */
>> +    s->dummy.peer = netdev;
>> +    s->incoming_queue = qemu_new_net_queue(nf);
>> +
>> +    return 0;
>> +}
>> diff --git a/net/filter.c b/net/filter.c
>> index 50fb837..e741e2a 100644
>> --- a/net/filter.c
>> +++ b/net/filter.c
>> @@ -18,6 +18,7 @@
>>
>>   #include "net/filter.h"
>>   #include "net/net.h"
>> +#include "filters.h"
>>
>>   static QTAILQ_HEAD(, NetFilterState) net_filters;
>>
>> @@ -152,6 +153,7 @@ typedef int (NetFilterInit)(const NetFilterOptions *opts,
>>
>>   static
>>   NetFilterInit * const net_filter_init_fun[NET_FILTER_OPTIONS_KIND_MAX] = {
>> +    [NET_FILTER_OPTIONS_KIND_BUFFER] = net_init_filter_buffer,
>>   };
>>
>>   static int net_filter_init1(const NetFilter *netfilter, Error **errp)
>> diff --git a/net/filters.h b/net/filters.h
>> new file mode 100644
>> index 0000000..6c249b8
>> --- /dev/null
>> +++ b/net/filters.h
>> @@ -0,0 +1,17 @@
>> +/*
>> + * Copyright (c) 2015 FUJITSU LIMITED
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later.  See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef QEMU_NET_FILTERS_H
>> +#define QEMU_NET_FILTERS_H
>> +
>> +#include "net/net.h"
>> +#include "net/filter.h"
>> +
>> +int net_init_filter_buffer(const NetFilterOptions *opts, const char *name,
>> +                           NetClientState *netdev, Error **errp);
>> +
>> +#endif /* QEMU_NET_FILTERS_H */
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 1fc6390..67e00a0 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -2577,6 +2577,16 @@
>>   { 'command': 'netfilter_del', 'data': {'id': 'str'} }
>>
>>   ##
>> +# @NetFilterBufferOptions
>> +#
>> +# a netbuffer filter for network backend.
>> +#
>> +# Since 2.5
>> +##
>> +{ 'struct': 'NetFilterBufferOptions',
>> +  'data': { } }
>> +
>> +##
>>   # @NetFilterOptions
>>   #
>>   # A discriminated record of network filters.
>> @@ -2585,7 +2595,8 @@
>>   #
>>   ##
>>   { 'union': 'NetFilterOptions',
>> -  'data': { } }
>> +  'data': {
>> +    'buffer':     'NetFilterBufferOptions'} }
>>
>>   ##
>>   # @NetFilter
>
> .
>

-- 
Thanks,
Yang.