Re: [Qemu-devel] [PATCH v1] netfilter: various comment fixes

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

Hi Markus,

   If I missed something, please feel free to point out...

On 09/25/2015 04:08 PM, Yang Hongyang wrote:
> This patchset addressed Markus comment on netfilter patch
> series, most of them are comment fixes.
>
> It is based on jason's net tree:
> https://github.com/jasowang/qemu/tree/net
>
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> ---
>   include/net/filter.h |  2 ++
>   net/filter-buffer.c  | 15 +++++++++------
>   net/filter.c         |  2 +-
>   3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/filter.h b/include/net/filter.h
> index 3fa80c9..d3ff569 100644
> --- a/include/net/filter.h
> +++ b/include/net/filter.h
> @@ -39,8 +39,10 @@ typedef ssize_t (FilterReceiveIOV)(NetFilterState *nc,
>   struct NetFilterClass {
>       ObjectClass parent_class;
>
> +    /* optional */
>       FilterSetup *setup;
>       FilterCleanup *cleanup;
> +    /* mandatory */
>       FilterReceiveIOV *receive_iov;
>   };
>   typedef struct NetFilterClass NetFilterClass;
> diff --git a/net/filter-buffer.c b/net/filter-buffer.c
> index ef94e91..a528737 100644
> --- a/net/filter-buffer.c
> +++ b/net/filter-buffer.c
> @@ -42,8 +42,10 @@ static void filter_buffer_flush(NetFilterState *nf)
>   static void filter_buffer_release_timer(void *opaque)
>   {
>       NetFilterState *nf = opaque;
> +
>       FilterBufferState *s = FILTER_BUFFER(nf);
>       filter_buffer_flush(nf);
> +    /* Timer rearmed to fire again in s->interval microseconds. */
>       timer_mod(&s->release_timer,
>                 qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + s->interval);
>   }
> @@ -59,9 +61,9 @@ static ssize_t filter_buffer_receive_iov(NetFilterState *nf,
>       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
> -     * FIXME: even if guest can't receive packet for some reasons. Filter
> +     * 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.
> +     * FIXME: Even if guest can't receive packet for some reasons. Filter
>        * can still accept packet until its internal queue is full.
>        */
>       qemu_net_queue_append_iov(s->incoming_queue, sender, flags,
> @@ -89,7 +91,7 @@ static void filter_buffer_setup(NetFilterState *nf, Error **errp)
>       FilterBufferState *s = FILTER_BUFFER(nf);
>
>       /*
> -     * this check should be dropped when there're VM FT solutions like MC
> +     * We may want to accept zero interval when VM FT solutions like MC
>        * or COLO use this filter to release packets on demand.
>        */
>       if (!s->interval) {
> @@ -102,6 +104,7 @@ static void filter_buffer_setup(NetFilterState *nf, Error **errp)
>       if (s->interval) {
>           timer_init_us(&s->release_timer, QEMU_CLOCK_VIRTUAL,
>                         filter_buffer_release_timer, nf);
> +        /* Timer armed to fire in s->interval microseconds. */
>           timer_mod(&s->release_timer,
>                     qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + s->interval);
>       }
> @@ -137,8 +140,8 @@ static void filter_buffer_set_interval(Object *obj, Visitor *v, void *opaque,
>           goto out;
>       }
>       if (!value) {
> -        error_setg(&local_err, "Property '%s.%s' doesn't take value '%"
> -                   PRIu32 "'", object_get_typename(obj), name, value);
> +        error_setg(&local_err, "Property '%s.%s' requires a positive value",
> +                   object_get_typename(obj), name);
>           goto out;
>       }
>       s->interval = value;
> diff --git a/net/filter.c b/net/filter.c
> index a8adc89..7b2437a 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -164,7 +164,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>                      "a network backend id");
>           return;
>       } else if (queues > 1) {
> -        error_setg(errp, "Multi queue is not supported");
> +        error_setg(errp, "multiqueue is not supported");
>           return;
>       }
>
>

-- 
Thanks,
Yang.