Re: [Qemu-devel] [PATCH v3 04/12] net: add/remove filters from network backend

[Yang Hongyang]

On 08/04/2015 01:53 PM, Jason Wang wrote:
> On 08/04/2015 01:39 PM, Yang Hongyang wrote:
> > On 08/04/2015 12:56 PM, Jason Wang wrote:
> > > On 08/03/2015 04:30 PM, Yang Hongyang wrote:
> > > > add/remove filters from network backend
> > > >
> > > > Signed-off-by: Yang Hongyang <address@hidden>
> > > > ---
> > > >    include/net/net.h |  8 ++++++++
> > > >    net/filter.c      |  4 ++--
> > > >    net/net.c         | 33 +++++++++++++++++++++++++++++++++
> > > >    3 files changed, 43 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/net/net.h b/include/net/net.h
> > > > index 6a6cbef..5c5c109 100644
> > > > --- a/include/net/net.h
> > > > +++ b/include/net/net.h
> > > > @@ -40,6 +40,11 @@ typedef struct NICConf {
> > > >
> > > >
> > > >    /* Net clients */
> > > > +typedef struct Filter Filter;
> > > > +struct Filter {
> > > > +    NetFilterState *nf;
> > > > +    QTAILQ_ENTRY(Filter) next;
> > > > +};
> > >
> > > Didn't understand why need another structure here. Could we just use
> > > NetFilterState?
> >
> > There's a QTAILQ_ENTRY next in NetFilterState, but used by filter
> > layer, so
> > that we can manage all filters together.
> >
> > This struct used by netdev. filter belongs to the specific netdev is
> > in this queue.
> >
> > Just use NetFilterState in this case need to introduce another
> > QTAILQ_ENTRY
> > in NetFilterState, maybe will make it confusing?
>
> Probably not.
>
> > and it seems that it's
> > not common to have 2 QTAILQ_ENTRYs in one struct?
>
> I think this is ok. You can use something like global_list.

Sounds good, thank you!

> > > >    typedef void (NetPoll)(NetClientState *, bool enable);
> > > >    typedef int (NetCanReceive)(NetClientState *);
> > > > @@ -92,6 +97,7 @@ struct NetClientState {
> > > >        NetClientDestructor *destructor;
> > > >        unsigned int queue_index;
> > > >        unsigned rxfilter_notify_enabled:1;
> > > > +    QTAILQ_HEAD(, Filter) filters;
> > > >    };
> > > >
> > > >    typedef struct NICState {
> > > > @@ -109,6 +115,8 @@ NetClientState
> > > > *qemu_new_net_client(NetClientInfo *info,
> > > >                                        NetClientState *peer,
> > > >                                        const char *model,
> > > >                                        const char *name);
> > > > +int qemu_netdev_add_filter(NetClientState *nc, NetFilterState *nf);
> > > > +void qemu_netdev_remove_filter(NetClientState *nc, NetFilterState
> > > > *nf);
> > > >    NICState *qemu_new_nic(NetClientInfo *info,
> > > >                           NICConf *conf,
> > > >                           const char *model,
> > > > diff --git a/net/filter.c b/net/filter.c
> > > > index 86eed8a..1ae9344 100644
> > > > --- a/net/filter.c
> > > > +++ b/net/filter.c
> > > > @@ -38,14 +38,14 @@ NetFilterState
> > > > *qemu_new_net_filter(NetFilterInfo *info,
> > > >        nf->netdev = netdev;
> > > >        nf->chain = chain;
> > > >        QTAILQ_INSERT_TAIL(&net_filters, nf, next);
> > > > -    /* TODO: attach netfilter to netdev */
> > > > +    qemu_netdev_add_filter(netdev, nf);
> > > >
> > > >        return nf;
> > > >    }
> > > >
> > > >    static void qemu_cleanup_net_filter(NetFilterState *nf)
> > > >    {
> > > > -    /* TODO: remove netfilter from netdev */
> > > > +    qemu_netdev_remove_filter(nf->netdev, nf);
> > > >
> > > >        QTAILQ_REMOVE(&net_filters, nf, next);
> > > >
> > > > diff --git a/net/net.c b/net/net.c
> > > > index 28a5597..00c5ca3 100644
> > > > --- a/net/net.c
> > > > +++ b/net/net.c
> > > > @@ -287,6 +287,7 @@ static void qemu_net_client_setup(NetClientState
> > > > *nc,
> > > >
> > > >        nc->incoming_queue = qemu_new_net_queue(nc);
> > > >        nc->destructor = destructor;
> > > > +    QTAILQ_INIT(&nc->filters);
> > > >    }
> > > >
> > > >    NetClientState *qemu_new_net_client(NetClientInfo *info,
> > > > @@ -305,6 +306,38 @@ NetClientState
> > > > *qemu_new_net_client(NetClientInfo *info,
> > > >        return nc;
> > > >    }
> > > >
> > > > +int qemu_netdev_add_filter(NetClientState *nc, NetFilterState *nf)
> > > > +{
> > > > +    Filter *filter = g_malloc0(sizeof(*filter));
> > > > +
> > > > +    filter->nf = nf;
> > > > +    QTAILQ_INSERT_TAIL(&nc->filters, filter, next);
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static void remove_filter(NetClientState *nc, Filter *filter)
> > > > +{
> > > > +    if (!filter) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    QTAILQ_REMOVE(&nc->filters, filter, next);
> > > > +    g_free(filter);
> > > > +}
> > > > +
> > > > +void qemu_netdev_remove_filter(NetClientState *nc, NetFilterState *nf)
> > > > +{
> > > > +    Filter *filter = NULL;
> > > > +
> > > > +    QTAILQ_FOREACH(filter, &nc->filters, next) {
> > > > +        if (filter->nf == nf) {
> > > > +            break;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    remove_filter(nc, filter);
> > > > +}
> > > > +
> > > >    NICState *qemu_new_nic(NetClientInfo *info,
> > > >                           NICConf *conf,
> > > >                           const char *model,
> > >
> > > Another thing may need consider is qemu_flush_queued_packets(). Look
> > > like we need also flush packets inside each filter in this case?
> >
> > When a filter is removed, the filter itself will flush the packets.
> > when a filter queued a packet, we assume the filter will take care
> > of it. the filter is not a netdev, so I think we do not need to flush
> > packets in qemu_flush_queued_packets(), otherwise, the buffer filter
> > will be useless, because when qemu_flush_queued_packets() is called,
> > it will flush the packets, it's not what we want.
>
> Right.
> .

--
Thanks,
Yang.