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

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

On 09/14/2015 05:04 PM, Daniel P. Berrange wrote:
> On Wed, Sep 09, 2015 at 03:24:40PM +0800, Yang Hongyang wrote:
>> This filter is to buffer/release packets, this feature can be used
>> when using MicroCheckpointing, or other Remus like VM FT solutions, you
>> can also use it to simulate the network delay.
>> It has an interval option, if supplied, this filter will release
>> packets by interval.
>>
>> Usage:
>>   -netdev tap,id=bn0
>>   -object filter-buffer,id=f0,netdev=bn0,chain=in,interval=1000
>>
>> NOTE:
>>   the scale of interval is microsecond.
>>
>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>
>> diff --git a/net/filter-buffer.c b/net/filter-buffer.c
>> new file mode 100644
>> index 0000000..26698d9
>> --- /dev/null
>> +++ b/net/filter-buffer.c
>> @@ -0,0 +1,169 @@
>> +/*
>> + * Copyright (c) 2015 FUJITSU LIMITED
>> + * Author: Yang Hongyang <yanghy@cn.fujitsu.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later.  See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "net/filter.h"
>> +#include "net/queue.h"
>> +#include "qemu-common.h"
>> +#include "qemu/timer.h"
>> +#include "qemu/iov.h"
>> +#include "qapi/qmp/qerror.h"
>> +#include "qapi-visit.h"
>> +#include "qom/object.h"
>> +
>> +#define TYPE_FILTER_BUFFER "filter-buffer"
>> +
>> +#define FILTER_BUFFER(obj) \
>> +    OBJECT_CHECK(FilterBufferState, (obj), TYPE_FILTER_BUFFER)
>> +
>> +typedef struct FilterBufferState {
>> +    NetFilterState parent_obj;
>> +
>> +    NetQueue *incoming_queue;
>> +    uint32_t interval;
>> +    QEMUTimer release_timer;
>> +} FilterBufferState;
>> +
>> +static void filter_buffer_flush(NetFilterState *nf)
>> +{
>> +    FilterBufferState *s = FILTER_BUFFER(nf);
>> +
>> +    if (!qemu_net_queue_flush(s->incoming_queue)) {
>> +        /* Unable to empty the queue, purge remaining packets */
>> +        qemu_net_queue_purge(s->incoming_queue, nf->netdev);
>> +    }
>> +}
>> +
>> +static void filter_buffer_release_timer(void *opaque)
>> +{
>> +    NetFilterState *nf = opaque;
>> +    FilterBufferState *s = FILTER_BUFFER(nf);
>> +    filter_buffer_flush(nf);
>> +    timer_mod(&s->release_timer,
>> +              qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + s->interval);
>> +}
>> +
>> +/* filter APIs */
>> +static ssize_t filter_buffer_receive_iov(NetFilterState *nf,
>> +                                         NetClientState *sender,
>> +                                         unsigned flags,
>> +                                         const struct iovec *iov,
>> +                                         int iovcnt,
>> +                                         NetPacketSent *sent_cb)
>> +{
>> +    FilterBufferState *s = FILTER_BUFFER(nf);
>> +
>> +    /*
>> +     * we return size when buffer a packet, the sender will take it as
>> +     * a already sent packet, so sent_cb should not be called later
>> +     */
>> +    qemu_net_queue_append_iov(s->incoming_queue, sender, flags,
>> +                              iov, iovcnt, NULL);
>> +    return iov_size(iov, iovcnt);
>> +}
>> +
>> +static void filter_buffer_cleanup(NetFilterState *nf)
>
> Same comment about s/cleanup/finalize/

This is not the .instance_finalize func, although it is called
in netfilter_finalize, it is a cleanup virtual method in the
filter class:
include/net/filter.h
  39 typedef struct NetFilterClass {
  40     ObjectClass parent_class;
  41
  42     FilterSetup *setup;
  43     FilterCleanup *cleanup;
  44     FilterReceiveIOV *receive_iov;
  45 } NetFilterClass;

>
>> +{
>> +    FilterBufferState *s = FILTER_BUFFER(nf);
>> +
>> +    if (s->interval) {
>> +        timer_del(&s->release_timer);
>> +    }
>> +
>> +    /* flush packets */
>> +    if (s->incoming_queue) {
>> +        filter_buffer_flush(nf);
>> +        g_free(s->incoming_queue);
>> +    }
>> +}
>
>> diff --git a/vl.c b/vl.c
>> index 672f8b2..30196e4 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2783,7 +2783,12 @@ static bool object_create_initial(const char *type)
>>       if (g_str_equal(type, "rng-egd")) {
>>           return false;
>>       }
>> -    /* TODO: reture false for concrete netfilters */
>
> Oh, I missed the typo in your earlier patch - s/reture/return/
>
>> +
>> +    /* reture false for concrete netfilters */
>
> And again in the line you changed here.

All fixed as well as your comments in patch 2, thank you.

>
>> +    if (g_str_equal(type, "filter-buffer")) {
>> +        return false;
>> +    }
>> +
>
>
> Regards,
> Daniel
>

-- 
Thanks,
Yang.