Re: [Qemu-devel] [PATCH v2 2/3] net/filter.c: Add Options to insert filters anywhere in the filter list

[Lukas Straub]

On Mon, 2 Sep 2019 11:43:57 +0000
"Zhang, Chen" <address@hidden> wrote:

> > -----Original Message-----
> > From: Lukas Straub <address@hidden>
> > Sent: Friday, August 23, 2019 2:21 PM
> > To: Zhang, Chen <address@hidden>
> > Cc: qemu-devel <address@hidden>; Jason Wang
> > <address@hidden>; Wen Congyang <address@hidden>;
> > Xie Changlong <address@hidden>
> > Subject: Re: [PATCH v2 2/3] net/filter.c: Add Options to insert filters
> > anywhere in the filter list
> >
> > On Fri, 23 Aug 2019 03:24:02 +0000
> > "Zhang, Chen" <address@hidden> wrote:
> >
> > > > -----Original Message-----
> > > > From: Lukas Straub [mailto:address@hidden]
> > > > Sent: Friday, August 16, 2019 2:49 AM
> > > > To: qemu-devel <address@hidden>
> > > > Cc: Zhang, Chen <address@hidden>; Jason Wang
> > > > <address@hidden>; Wen Congyang
> > <address@hidden>; Xie
> > > > Changlong <address@hidden>
> > > > Subject: [PATCH v2 2/3] net/filter.c: Add Options to insert filters
> > > > anywhere in the filter list
> > > >
> > > > To switch the Secondary to Primary, we need to insert new filters
> > > > before the filter-rewriter.
> > > >
> > > > Add the options insert= and position= to be able to insert filters
> > > > anywhere in the filter list.
> > > >
> > > > position should be either "head", "tail" or the id of another filter.
> > > > insert should be either "before" or "after" to specify where to
> > > > insert the new filter relative to the one specified with position.
> > > >
> > >
> > > Hi Lukas,
> > >
> > > It looks no need to add the "insert = xxx" for this operation.
> > > For example:
> > >
> > > We have 3 net-filters, the running order like that:
> > >
> > > Fiter1   ---------->   Filter2 ------------> Filter3
> > >
> > > If we want to add another filter between filter1 and filter2.
> > > The "Position = head, insert = after" always seam with "position = 
> > > filter2 id,
> > insert = before".
> >
> > Hi Zhang,
> > The insert= parameter is ignored if position=head or tail. It always 
> > Inserts at
> > the head (before Filter1) or the tail (after Filter3) of the List in these 
> > cases.
> >
> > > It seems the "insert" is a redundant args.
> > > So I think it is enough with the "position", we can make the "insert" 
> > > always
> > equal "after" except the "head".
> >
> > Yes, we still could do it without it, but its more convenient with the 
> > insert=
> > parameter. For example our Case with inserting before the rewriter:
> >
> > 'filter-mirror', 'id': 'm0', 'props': { 'insert': 'before', 'position': 
> > 'rew0', 'netdev':
> > 'hn0', 'queue': 'tx', 'outdev': 'mirror0' } 'filter-redirector', 'id': 
> > 'redire0', 'props':
> > { 'insert': 'before', 'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 
> > 'indev':
> > 'compare_out' } 'filter-redirector', 'id': 'redire1', 'props': { 'insert': 
> > 'before',
> > 'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'outdev': 'compare0' }
> >
> > You see directly that here 3 Filters are inserted before the rewriter.
> >
> > would have to become:
> >
> > 'filter-mirror', 'id': 'm0', 'props': { 'position': 'head', 'netdev': 
> > 'hn0', 'queue': 'tx',
> > 'outdev': 'mirror0' } 'filter-redirector', 'id': 'redire0', 'props': { 
> > 'position': 'm0',
> > 'netdev': 'hn0', 'queue': 'rx', 'indev': 'compare_out' } 
> > 'filter-redirector', 'id':
> > 'redire1', 'props': { 'position': 'redire0', 'netdev': 'hn0', 'queue': 
> > 'rx', 'outdev':
> > 'compare0' }
> >
> > Which is less obvious.
>
> OK, It is fine for me.
> But in the code have some other issues like that:
>
> +
> +static void netfilter_set_insert(Object *obj, const char *str, Error **errp)
> +{
> +    NetFilterState *nf = NETFILTER(obj);
> +
> +    if (strcmp(str, "before") && strcmp(str, "after")) {
> +        error_setg(errp, "Invalid value for netfilter insert, "
> +                         "should be 'head' or 'tail'");                      
>             -------------------------------->>> I think you should change the 
> "head/tail"  to "before/after".

Oops, that was a typo.

> +        return;
> +    }
> +
> +    nf->insert_before = !strcmp(str, "before");
> +}
>
>
> And I think the "front/behind" is better than "before/after" in this status.
> At the same time the name of the "insert_before" change to "front_flag" is 
> better.

What I like about the "before" is that it sounds more like an English sentence.
insert: before, position: rew0
vs.
insert: front, position: rew0

But I agree, "behind" is more clear than "after".

What do you think about "first/last" instead of "head/tail"?

Regards,
Lukas Straub

>
> Thanks
> Zhang Chen
>
> >
> > Regards,
> > Lukas Straub
> >
> > >
> > > Thanks
> > > Zhang Chen
> > >
> > > > Signed-off-by: Lukas Straub <address@hidden>
> > > > ---
> > > >  include/net/filter.h |  2 ++
> > > >  net/filter.c         | 71
> > +++++++++++++++++++++++++++++++++++++++++++-
> > > >  qemu-options.hx      | 10 +++----
> > > >  3 files changed, 77 insertions(+), 6 deletions(-)
>