| Message ID | 1456373149-26256-1-git-send-email-zhang.zhanghailiang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/25/2016 12:05 PM, zhanghailiang wrote: > With this property, users can control if this filter is 'enable' > or 'disable'. The default behavior for filter is enabled. > > For buffer filter, we need to release all the buffered packets > while disabled it. Here we use the 'disable' callback of NetFilterClass > to realize this capability. We register it with filter_buffer_flush(). > The other types of filters can realize their own 'disable' callback. > > We will skip the disabled filter when delivering packets in net layer. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Cc: Jason Wang <jasowang@redhat.com> > Cc: Yang Hongyang <hongyang.yang@easystack.cn> > --- > This is picked from COLO series, which is to realize the new 'status' > property for filter. Looks good overall, just few nits. And let's split this into two patches. > --- > include/net/filter.h | 4 ++++ > net/filter-buffer.c | 1 + > net/filter.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > qemu-options.hx | 4 +++- > 4 files changed, 52 insertions(+), 1 deletion(-) > > diff --git a/include/net/filter.h b/include/net/filter.h > index 5639976..bd27074 100644 > --- a/include/net/filter.h > +++ b/include/net/filter.h > @@ -36,12 +36,15 @@ typedef ssize_t (FilterReceiveIOV)(NetFilterState *nc, > int iovcnt, > NetPacketSent *sent_cb); > > +typedef void (FilterDisable) (NetFilterState *nf); Let's rename this to 'FilterStatusChanged' to be aligned with link status. > + > typedef struct NetFilterClass { > ObjectClass parent_class; > > /* optional */ > FilterSetup *setup; > FilterCleanup *cleanup; > + FilterDisable *disable; So we can use 'status_changed' instead of 'disable' here. > /* mandatory */ > FilterReceiveIOV *receive_iov; > } NetFilterClass; > @@ -55,6 +58,7 @@ struct NetFilterState { > char *netdev_id; > NetClientState *netdev; > NetFilterDirection direction; > + bool enabled; 'status' is much better. > QTAILQ_ENTRY(NetFilterState) next; > }; > > diff --git a/net/filter-buffer.c b/net/filter-buffer.c > index 12ad2e3..b1464c9 100644 > --- a/net/filter-buffer.c > +++ b/net/filter-buffer.c > @@ -131,6 +131,7 @@ static void filter_buffer_class_init(ObjectClass *oc, void *data) > nfc->setup = filter_buffer_setup; > nfc->cleanup = filter_buffer_cleanup; > nfc->receive_iov = filter_buffer_receive_iov; > + nfc->disable = filter_buffer_flush; I believe we should start and stop the timer during status changed. > } > > static void filter_buffer_get_interval(Object *obj, Visitor *v, > diff --git a/net/filter.c b/net/filter.c > index d2a514e..edd18c4 100644 > --- a/net/filter.c > +++ b/net/filter.c > @@ -17,6 +17,11 @@ > #include "qom/object_interfaces.h" > #include "qemu/iov.h" > > +static inline bool qemu_can_skip_netfilter(NetFilterState *nf) > +{ > + return nf->enabled ? false : true; > +} > + > ssize_t qemu_netfilter_receive(NetFilterState *nf, > NetFilterDirection direction, > NetClientState *sender, > @@ -25,6 +30,9 @@ ssize_t qemu_netfilter_receive(NetFilterState *nf, > int iovcnt, > NetPacketSent *sent_cb) > { > + if (qemu_can_skip_netfilter(nf)) { > + return 0; > + } > if (nf->direction == direction || > nf->direction == NET_FILTER_DIRECTION_ALL) { > return NETFILTER_GET_CLASS(OBJECT(nf))->receive_iov( > @@ -134,8 +142,41 @@ static void netfilter_set_direction(Object *obj, int direction, Error **errp) > nf->direction = direction; > } > > +static char *netfilter_get_status(Object *obj, Error **errp) > +{ > + NetFilterState *nf = NETFILTER(obj); > + > + if (nf->enabled) { > + return g_strdup("enable"); Let's use "on" and "off" here. > + } else { > + return g_strdup("disable"); > + } > +} > + > +static void netfilter_set_status(Object *obj, const char *str, Error **errp) > +{ > + NetFilterState *nf = NETFILTER(obj); > + NetFilterClass *nfc = NETFILTER_GET_CLASS(obj); > + > + if (!strcmp(str, "enable")) { > + nf->enabled = true; We'd better also call status changed here, in case the filter (e.g future implementation) need some setup. > + } else if (!strcmp(str, "disable")) { > + nf->enabled = false; > + if (nfc->disable) { > + nfc->disable(nf); > + } > + } else { > + error_setg(errp, "Invalid value for netfilter status, " > + "should be 'enable' or 'disable'"); > + } > +} > + > static void netfilter_init(Object *obj) > { > + NetFilterState *nf = NETFILTER(obj); > + > + nf->enabled = true; > + > object_property_add_str(obj, "netdev", > netfilter_get_netdev_id, netfilter_set_netdev_id, > NULL); > @@ -143,6 +184,9 @@ static void netfilter_init(Object *obj) > NetFilterDirection_lookup, > netfilter_get_direction, netfilter_set_direction, > NULL); > + object_property_add_str(obj, "status", > + netfilter_get_status, netfilter_set_status, > + NULL); > } > > static void netfilter_complete(UserCreatable *uc, Error **errp) > diff --git a/qemu-options.hx b/qemu-options.hx > index f528405..15b8e48 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3746,11 +3746,13 @@ version by providing the @var{passwordid} parameter. This provides > the ID of a previously created @code{secret} object containing the > password for decryption. > > -@item -object filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}] > +@item -object filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}][,status=@var{enable|disable}] > > Interval @var{t} can't be 0, this filter batches the packet delivery: all > packets arriving in a given interval on netdev @var{netdevid} are delayed > until the end of the interval. Interval is in microseconds. > +@option{status} is optional that indicate whether the netfilter is enabled > +or disabled, the default status for netfilter will be enabled. > > queue @var{all|rx|tx} is an option that can be applied to any netfilter. >
On 2016/2/26 14:49, Jason Wang wrote: > > > On 02/25/2016 12:05 PM, zhanghailiang wrote: >> With this property, users can control if this filter is 'enable' >> or 'disable'. The default behavior for filter is enabled. >> >> For buffer filter, we need to release all the buffered packets >> while disabled it. Here we use the 'disable' callback of NetFilterClass >> to realize this capability. We register it with filter_buffer_flush(). >> The other types of filters can realize their own 'disable' callback. >> >> We will skip the disabled filter when delivering packets in net layer. >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> Cc: Jason Wang <jasowang@redhat.com> >> Cc: Yang Hongyang <hongyang.yang@easystack.cn> >> --- >> This is picked from COLO series, which is to realize the new 'status' >> property for filter. > > Looks good overall, just few nits. And let's split this into two patches. > OK, and how to do the split ? One patch to add the 'status' property and notifier for filter, and another to realize the status changing callback for buffer filter ? >> --- >> include/net/filter.h | 4 ++++ >> net/filter-buffer.c | 1 + >> net/filter.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> qemu-options.hx | 4 +++- >> 4 files changed, 52 insertions(+), 1 deletion(-) >> >> diff --git a/include/net/filter.h b/include/net/filter.h >> index 5639976..bd27074 100644 >> --- a/include/net/filter.h >> +++ b/include/net/filter.h >> @@ -36,12 +36,15 @@ typedef ssize_t (FilterReceiveIOV)(NetFilterState *nc, >> int iovcnt, >> NetPacketSent *sent_cb); >> >> +typedef void (FilterDisable) (NetFilterState *nf); > > Let's rename this to 'FilterStatusChanged' to be aligned with link status. > OK. >> + >> typedef struct NetFilterClass { >> ObjectClass parent_class; >> >> /* optional */ >> FilterSetup *setup; >> FilterCleanup *cleanup; >> + FilterDisable *disable; > > So we can use 'status_changed' instead of 'disable' here. > >> /* mandatory */ >> FilterReceiveIOV *receive_iov; >> } NetFilterClass; >> @@ -55,6 +58,7 @@ struct NetFilterState { >> char *netdev_id; >> NetClientState *netdev; >> NetFilterDirection direction; >> + bool enabled; > > 'status' is much better. > What's the type of 'status' ? 'char *' ? >> QTAILQ_ENTRY(NetFilterState) next; >> }; >> >> diff --git a/net/filter-buffer.c b/net/filter-buffer.c >> index 12ad2e3..b1464c9 100644 >> --- a/net/filter-buffer.c >> +++ b/net/filter-buffer.c >> @@ -131,6 +131,7 @@ static void filter_buffer_class_init(ObjectClass *oc, void *data) >> nfc->setup = filter_buffer_setup; >> nfc->cleanup = filter_buffer_cleanup; >> nfc->receive_iov = filter_buffer_receive_iov; >> + nfc->disable = filter_buffer_flush; > > I believe we should start and stop the timer during status changed. > Yes, you are right, it is needed. >> } >> >> static void filter_buffer_get_interval(Object *obj, Visitor *v, >> diff --git a/net/filter.c b/net/filter.c >> index d2a514e..edd18c4 100644 >> --- a/net/filter.c >> +++ b/net/filter.c >> @@ -17,6 +17,11 @@ >> #include "qom/object_interfaces.h" >> #include "qemu/iov.h" >> >> +static inline bool qemu_can_skip_netfilter(NetFilterState *nf) >> +{ >> + return nf->enabled ? false : true; >> +} >> + >> ssize_t qemu_netfilter_receive(NetFilterState *nf, >> NetFilterDirection direction, >> NetClientState *sender, >> @@ -25,6 +30,9 @@ ssize_t qemu_netfilter_receive(NetFilterState *nf, >> int iovcnt, >> NetPacketSent *sent_cb) >> { >> + if (qemu_can_skip_netfilter(nf)) { >> + return 0; >> + } >> if (nf->direction == direction || >> nf->direction == NET_FILTER_DIRECTION_ALL) { >> return NETFILTER_GET_CLASS(OBJECT(nf))->receive_iov( >> @@ -134,8 +142,41 @@ static void netfilter_set_direction(Object *obj, int direction, Error **errp) >> nf->direction = direction; >> } >> >> +static char *netfilter_get_status(Object *obj, Error **errp) >> +{ >> + NetFilterState *nf = NETFILTER(obj); >> + >> + if (nf->enabled) { >> + return g_strdup("enable"); > > Let's use "on" and "off" here. > OK. >> + } else { >> + return g_strdup("disable"); >> + } >> +} >> + >> +static void netfilter_set_status(Object *obj, const char *str, Error **errp) >> +{ >> + NetFilterState *nf = NETFILTER(obj); >> + NetFilterClass *nfc = NETFILTER_GET_CLASS(obj); >> + >> + if (!strcmp(str, "enable")) { >> + nf->enabled = true; > > We'd better also call status changed here, in case the filter (e.g > future implementation) need some setup. > I will fix it. Thanks. >> + } else if (!strcmp(str, "disable")) { >> + nf->enabled = false; >> + if (nfc->disable) { >> + nfc->disable(nf); >> + } >> + } else { >> + error_setg(errp, "Invalid value for netfilter status, " >> + "should be 'enable' or 'disable'"); >> + } >> +} >> + >> static void netfilter_init(Object *obj) >> { >> + NetFilterState *nf = NETFILTER(obj); >> + >> + nf->enabled = true; >> + >> object_property_add_str(obj, "netdev", >> netfilter_get_netdev_id, netfilter_set_netdev_id, >> NULL); >> @@ -143,6 +184,9 @@ static void netfilter_init(Object *obj) >> NetFilterDirection_lookup, >> netfilter_get_direction, netfilter_set_direction, >> NULL); >> + object_property_add_str(obj, "status", >> + netfilter_get_status, netfilter_set_status, >> + NULL); >> } >> >> static void netfilter_complete(UserCreatable *uc, Error **errp) >> diff --git a/qemu-options.hx b/qemu-options.hx >> index f528405..15b8e48 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -3746,11 +3746,13 @@ version by providing the @var{passwordid} parameter. This provides >> the ID of a previously created @code{secret} object containing the >> password for decryption. >> >> -@item -object filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}] >> +@item -object filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}][,status=@var{enable|disable}] >> >> Interval @var{t} can't be 0, this filter batches the packet delivery: all >> packets arriving in a given interval on netdev @var{netdevid} are delayed >> until the end of the interval. Interval is in microseconds. >> +@option{status} is optional that indicate whether the netfilter is enabled >> +or disabled, the default status for netfilter will be enabled. >> >> queue @var{all|rx|tx} is an option that can be applied to any netfilter. >> > > > . >
diff --git a/include/net/filter.h b/include/net/filter.h index 5639976..bd27074 100644 --- a/include/net/filter.h +++ b/include/net/filter.h @@ -36,12 +36,15 @@ typedef ssize_t (FilterReceiveIOV)(NetFilterState *nc, int iovcnt, NetPacketSent *sent_cb); +typedef void (FilterDisable) (NetFilterState *nf); + typedef struct NetFilterClass { ObjectClass parent_class; /* optional */ FilterSetup *setup; FilterCleanup *cleanup; + FilterDisable *disable; /* mandatory */ FilterReceiveIOV *receive_iov; } NetFilterClass; @@ -55,6 +58,7 @@ struct NetFilterState { char *netdev_id; NetClientState *netdev; NetFilterDirection direction; + bool enabled; QTAILQ_ENTRY(NetFilterState) next; }; diff --git a/net/filter-buffer.c b/net/filter-buffer.c index 12ad2e3..b1464c9 100644 --- a/net/filter-buffer.c +++ b/net/filter-buffer.c @@ -131,6 +131,7 @@ static void filter_buffer_class_init(ObjectClass *oc, void *data) nfc->setup = filter_buffer_setup; nfc->cleanup = filter_buffer_cleanup; nfc->receive_iov = filter_buffer_receive_iov; + nfc->disable = filter_buffer_flush; } static void filter_buffer_get_interval(Object *obj, Visitor *v, diff --git a/net/filter.c b/net/filter.c index d2a514e..edd18c4 100644 --- a/net/filter.c +++ b/net/filter.c @@ -17,6 +17,11 @@ #include "qom/object_interfaces.h" #include "qemu/iov.h" +static inline bool qemu_can_skip_netfilter(NetFilterState *nf) +{ + return nf->enabled ? false : true; +} + ssize_t qemu_netfilter_receive(NetFilterState *nf, NetFilterDirection direction, NetClientState *sender, @@ -25,6 +30,9 @@ ssize_t qemu_netfilter_receive(NetFilterState *nf, int iovcnt, NetPacketSent *sent_cb) { + if (qemu_can_skip_netfilter(nf)) { + return 0; + } if (nf->direction == direction || nf->direction == NET_FILTER_DIRECTION_ALL) { return NETFILTER_GET_CLASS(OBJECT(nf))->receive_iov( @@ -134,8 +142,41 @@ static void netfilter_set_direction(Object *obj, int direction, Error **errp) nf->direction = direction; } +static char *netfilter_get_status(Object *obj, Error **errp) +{ + NetFilterState *nf = NETFILTER(obj); + + if (nf->enabled) { + return g_strdup("enable"); + } else { + return g_strdup("disable"); + } +} + +static void netfilter_set_status(Object *obj, const char *str, Error **errp) +{ + NetFilterState *nf = NETFILTER(obj); + NetFilterClass *nfc = NETFILTER_GET_CLASS(obj); + + if (!strcmp(str, "enable")) { + nf->enabled = true; + } else if (!strcmp(str, "disable")) { + nf->enabled = false; + if (nfc->disable) { + nfc->disable(nf); + } + } else { + error_setg(errp, "Invalid value for netfilter status, " + "should be 'enable' or 'disable'"); + } +} + static void netfilter_init(Object *obj) { + NetFilterState *nf = NETFILTER(obj); + + nf->enabled = true; + object_property_add_str(obj, "netdev", netfilter_get_netdev_id, netfilter_set_netdev_id, NULL); @@ -143,6 +184,9 @@ static void netfilter_init(Object *obj) NetFilterDirection_lookup, netfilter_get_direction, netfilter_set_direction, NULL); + object_property_add_str(obj, "status", + netfilter_get_status, netfilter_set_status, + NULL); } static void netfilter_complete(UserCreatable *uc, Error **errp) diff --git a/qemu-options.hx b/qemu-options.hx index f528405..15b8e48 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3746,11 +3746,13 @@ version by providing the @var{passwordid} parameter. This provides the ID of a previously created @code{secret} object containing the password for decryption. -@item -object filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}] +@item -object filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}][,status=@var{enable|disable}] Interval @var{t} can't be 0, this filter batches the packet delivery: all packets arriving in a given interval on netdev @var{netdevid} are delayed until the end of the interval. Interval is in microseconds. +@option{status} is optional that indicate whether the netfilter is enabled +or disabled, the default status for netfilter will be enabled. queue @var{all|rx|tx} is an option that can be applied to any netfilter.
With this property, users can control if this filter is 'enable' or 'disable'. The default behavior for filter is enabled. For buffer filter, we need to release all the buffered packets while disabled it. Here we use the 'disable' callback of NetFilterClass to realize this capability. We register it with filter_buffer_flush(). The other types of filters can realize their own 'disable' callback. We will skip the disabled filter when delivering packets in net layer. Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> Cc: Jason Wang <jasowang@redhat.com> Cc: Yang Hongyang <hongyang.yang@easystack.cn> --- This is picked from COLO series, which is to realize the new 'status' property for filter. --- include/net/filter.h | 4 ++++ net/filter-buffer.c | 1 + net/filter.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ qemu-options.hx | 4 +++- 4 files changed, 52 insertions(+), 1 deletion(-)