Message ID | 1438590616-21142-5-git-send-email-yanghy@cn.fujitsu.com |
---|---|
State | New |
Headers | show |
On 08/03/2015 04:30 PM, Yang Hongyang wrote: > add/remove filters from network backend > > Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com> > --- > 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? > > 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?
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 <yanghy@cn.fujitsu.com> >> --- >> 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? and it seems that it's not common to have 2 QTAILQ_ENTRYs in one struct? > >> >> 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. > . >
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 <yanghy@cn.fujitsu.com> >>> --- >>> 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. > >> >>> >>> 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.
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 <yanghy@cn.fujitsu.com> >>>> --- >>>> 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. > . >
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; +}; 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,
add/remove filters from network backend Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com> --- include/net/net.h | 8 ++++++++ net/filter.c | 4 ++-- net/net.c | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 2 deletions(-)