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

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

[Yang Hongyang]

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;
     }
 
-- 
1.9.1

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

[Yang Hongyang]

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.

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 929 bytes --]

On 09/25/2015 02:08 AM, 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

If that tree is not yet merged mainline, wouldn't it be better to squash
these fixes directly into the offending patches, instead of using it as
a followup patch?


>      /*
> -     * 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.

s/a already/an already/

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

Yang Hongyang <yanghy@cn.fujitsu.com> writes:

> 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>

I think we have enough issues to warrant a full respin.  Jason's call,
of course.

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

[Jason Wang]

On 09/25/2015 10:30 PM, Eric Blake wrote:
> On 09/25/2015 02:08 AM, 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
> If that tree is not yet merged mainline, wouldn't it be better to squash
> these fixes directly into the offending patches, instead of using it as
> a followup patch?

Yes, and maybe a new version.

>
>>      /*
>> -     * 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.
> s/a already/an already/
>

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

[Jason Wang]

On 09/25/2015 11:33 PM, Markus Armbruster wrote:
> Yang Hongyang <yanghy@cn.fujitsu.com> writes:
>
>> 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>
> I think we have enough issues to warrant a full respin.  Jason's call,
> of course.
>

I thought most of them were comments fixes but look not.

Hongyang:

I will drop the v11 from the tree, sorry. Please address the comments
and send V12.

Thanks

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

[Yang Hongyang]

On 09/28/2015 02:15 PM, Jason Wang wrote:
>
>
> On 09/25/2015 11:33 PM, Markus Armbruster wrote:
>> Yang Hongyang <yanghy@cn.fujitsu.com> writes:
>>
>>> 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>
>> I think we have enough issues to warrant a full respin.  Jason's call,
>> of course.
>>
>
> I thought most of them were comments fixes but look not.
>
> Hongyang:
>
> I will drop the v11 from the tree, sorry. Please address the comments
> and send V12.

Ok, will do, thanks.

>
> Thanks
>

-- 
Thanks,
Yang.