| Message ID | 1453451811-11860-7-git-send-email-zhang.zhanghailiang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/22/2016 04:36 PM, zhanghailiang wrote: > We add each netdev a default buffer filter, which the name is > 'nop', and the default buffer filter is disabled, so it has > no side effect for packets delivering in qemu net layer. > > The default buffer filter can be used by COLO or Micro-checkpoint, > The reason we add the default filter is we hope to support > hot add network during COLO state in future. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > --- > include/net/filter.h | 11 +++++++++++ > net/dump.c | 2 -- > net/filter.c | 15 ++++++++++++++- > net/net.c | 18 ++++++++++++++++++ > 4 files changed, 43 insertions(+), 3 deletions(-) > > diff --git a/include/net/filter.h b/include/net/filter.h > index c7bd8f9..2043609 100644 > --- a/include/net/filter.h > +++ b/include/net/filter.h > @@ -22,6 +22,16 @@ > #define NETFILTER_CLASS(klass) \ > OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER) > > +#define DEFAULT_FILTER_NAME "nop" Maybe DEFAULT_FILTER_TYPE? > + > +#define TYPE_FILTER_BUFFER "filter-buffer" > +#define TYPE_FILTER_DUMP "filter-dump" > + > +#define NETFILTER_ID_BUFFER 1 > +#define NETFILTER_ID_DUMP 2 > + > +extern const char *const netfilter_type_lookup[]; > + > typedef void (FilterSetup) (NetFilterState *nf, Error **errp); > typedef void (FilterCleanup) (NetFilterState *nf); > /* > @@ -55,6 +65,7 @@ struct NetFilterState { > char *netdev_id; > NetClientState *netdev; > NetFilterDirection direction; > + bool is_default; > bool enabled; > QTAILQ_ENTRY(NetFilterState) next; > }; > diff --git a/net/dump.c b/net/dump.c > index 88d9582..82727a6 100644 > --- a/net/dump.c > +++ b/net/dump.c > @@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts, const char *name, > > /* Dumping via filter */ > > -#define TYPE_FILTER_DUMP "filter-dump" > - > #define FILTER_DUMP(obj) \ > OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP) > > diff --git a/net/filter.c b/net/filter.c > index 4d96301..a126a3b 100644 > --- a/net/filter.c > +++ b/net/filter.c > @@ -21,6 +21,11 @@ > #include "qapi/qmp-input-visitor.h" > #include "monitor/monitor.h" > > +const char *const netfilter_type_lookup[] = { > + [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER, > + [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP, > +}; > + > ssize_t qemu_netfilter_receive(NetFilterState *nf, > NetFilterDirection direction, > NetClientState *sender, > @@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp) > NetFilterClass *nfc = NETFILTER_GET_CLASS(uc); > int queues; > Error *local_err = NULL; > - > + char *path = object_get_canonical_path_component(OBJECT(nf)); > > if (!nf->netdev_id) { > error_setg(errp, "Parameter 'netdev' is required"); > @@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable *uc, Error **errp) > } > > nf->netdev = ncs[0]; > + nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME); > + /* > + * For the default buffer filter, it will be disabled by default, > + * So it will not buffer any packets. > + */ > + if (nf->is_default) { > + nf->enabled = false; > + } This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status" into properties. > > if (nfc->setup) { > nfc->setup(nf, &local_err); > diff --git a/net/net.c b/net/net.c > index ec43105..9630234 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -76,6 +76,12 @@ const char *host_net_devices[] = { > > int default_net = 1; > > +/* > + * FIXME: Export this with an option for users to control > + * this with comand line ? This could be done in the future. > + */ > +int default_netfilter = NETFILTER_ID_BUFFER; Why not just use a string here? > + > /***********************************************************/ > /* network device redirectors */ > > @@ -1032,6 +1038,18 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp) > } > return -1; > } > + > + if (is_netdev) { > + const Netdev *netdev = object; > + /* > + * Here we add each netdev a default filter whose name is 'nop', > + * it will disabled by default, Users can enable it when necessary. > + */ If we support default properties, the above comment could be removed. > + netdev_add_filter(netdev->id, > + netfilter_type_lookup[default_netfilter], > + DEFAULT_FILTER_NAME, I believe some logic to generate id automatically is needed here. > + errp); > + } > return 0; > } >
On 2016/1/25 13:18, Jason Wang wrote: > > > On 01/22/2016 04:36 PM, zhanghailiang wrote: >> We add each netdev a default buffer filter, which the name is >> 'nop', and the default buffer filter is disabled, so it has >> no side effect for packets delivering in qemu net layer. >> >> The default buffer filter can be used by COLO or Micro-checkpoint, >> The reason we add the default filter is we hope to support >> hot add network during COLO state in future. >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> --- >> include/net/filter.h | 11 +++++++++++ >> net/dump.c | 2 -- >> net/filter.c | 15 ++++++++++++++- >> net/net.c | 18 ++++++++++++++++++ >> 4 files changed, 43 insertions(+), 3 deletions(-) >> >> diff --git a/include/net/filter.h b/include/net/filter.h >> index c7bd8f9..2043609 100644 >> --- a/include/net/filter.h >> +++ b/include/net/filter.h >> @@ -22,6 +22,16 @@ >> #define NETFILTER_CLASS(klass) \ >> OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER) >> >> +#define DEFAULT_FILTER_NAME "nop" > > Maybe DEFAULT_FILTER_TYPE? > >> + >> +#define TYPE_FILTER_BUFFER "filter-buffer" >> +#define TYPE_FILTER_DUMP "filter-dump" >> + >> +#define NETFILTER_ID_BUFFER 1 >> +#define NETFILTER_ID_DUMP 2 >> + >> +extern const char *const netfilter_type_lookup[]; >> + >> typedef void (FilterSetup) (NetFilterState *nf, Error **errp); >> typedef void (FilterCleanup) (NetFilterState *nf); >> /* >> @@ -55,6 +65,7 @@ struct NetFilterState { >> char *netdev_id; >> NetClientState *netdev; >> NetFilterDirection direction; >> + bool is_default; >> bool enabled; >> QTAILQ_ENTRY(NetFilterState) next; >> }; >> diff --git a/net/dump.c b/net/dump.c >> index 88d9582..82727a6 100644 >> --- a/net/dump.c >> +++ b/net/dump.c >> @@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts, const char *name, >> >> /* Dumping via filter */ >> >> -#define TYPE_FILTER_DUMP "filter-dump" >> - >> #define FILTER_DUMP(obj) \ >> OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP) >> >> diff --git a/net/filter.c b/net/filter.c >> index 4d96301..a126a3b 100644 >> --- a/net/filter.c >> +++ b/net/filter.c >> @@ -21,6 +21,11 @@ >> #include "qapi/qmp-input-visitor.h" >> #include "monitor/monitor.h" >> >> +const char *const netfilter_type_lookup[] = { >> + [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER, >> + [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP, >> +}; >> + >> ssize_t qemu_netfilter_receive(NetFilterState *nf, >> NetFilterDirection direction, >> NetClientState *sender, >> @@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp) >> NetFilterClass *nfc = NETFILTER_GET_CLASS(uc); >> int queues; >> Error *local_err = NULL; >> - >> + char *path = object_get_canonical_path_component(OBJECT(nf)); >> >> if (!nf->netdev_id) { >> error_setg(errp, "Parameter 'netdev' is required"); >> @@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable *uc, Error **errp) >> } >> >> nf->netdev = ncs[0]; >> + nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME); >> + /* >> + * For the default buffer filter, it will be disabled by default, >> + * So it will not buffer any packets. >> + */ >> + if (nf->is_default) { >> + nf->enabled = false; >> + } > > This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may > also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status" > into properties. > A little confused, do you mean add a 'default' property for filter ? Just like the new 'status' property which is exported to users ? Is the type of 'default' property string or bool ? >> >> if (nfc->setup) { >> nfc->setup(nf, &local_err); >> diff --git a/net/net.c b/net/net.c >> index ec43105..9630234 100644 >> --- a/net/net.c >> +++ b/net/net.c >> @@ -76,6 +76,12 @@ const char *host_net_devices[] = { >> >> int default_net = 1; >> >> +/* >> + * FIXME: Export this with an option for users to control >> + * this with comand line ? > > This could be done in the future. > Should i change the tag to 'TODO' ? >> + */ >> +int default_netfilter = NETFILTER_ID_BUFFER; > > Why not just use a string here? > No special, i will convert it to use string here. >> + >> /***********************************************************/ >> /* network device redirectors */ >> >> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp) >> } >> return -1; >> } >> + >> + if (is_netdev) { >> + const Netdev *netdev = object; >> + /* >> + * Here we add each netdev a default filter whose name is 'nop', >> + * it will disabled by default, Users can enable it when necessary. >> + */ > > If we support default properties, the above comment could be removed. > >> + netdev_add_filter(netdev->id, >> + netfilter_type_lookup[default_netfilter], >> + DEFAULT_FILTER_NAME, > > I believe some logic to generate id automatically is needed here. > Yes, you are right, here this patch will break QEMU with multi-NICs configured, the error report is " attempt to add duplicate property 'nop' to object". I will fix it in next version. Thanks, Hailiang >> + errp); >> + } >> return 0; >> } >> > > > . >
On 2016/1/25 15:22, Hailiang Zhang wrote: > On 2016/1/25 13:18, Jason Wang wrote: >> >> >> On 01/22/2016 04:36 PM, zhanghailiang wrote: >>> We add each netdev a default buffer filter, which the name is >>> 'nop', and the default buffer filter is disabled, so it has >>> no side effect for packets delivering in qemu net layer. >>> >>> The default buffer filter can be used by COLO or Micro-checkpoint, >>> The reason we add the default filter is we hope to support >>> hot add network during COLO state in future. >>> >>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>> --- >>> include/net/filter.h | 11 +++++++++++ >>> net/dump.c | 2 -- >>> net/filter.c | 15 ++++++++++++++- >>> net/net.c | 18 ++++++++++++++++++ >>> 4 files changed, 43 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/net/filter.h b/include/net/filter.h >>> index c7bd8f9..2043609 100644 >>> --- a/include/net/filter.h >>> +++ b/include/net/filter.h >>> @@ -22,6 +22,16 @@ >>> #define NETFILTER_CLASS(klass) \ >>> OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER) >>> >>> +#define DEFAULT_FILTER_NAME "nop" >> >> Maybe DEFAULT_FILTER_TYPE? >> > >>> + >>> +#define TYPE_FILTER_BUFFER "filter-buffer" >>> +#define TYPE_FILTER_DUMP "filter-dump" >>> + >>> +#define NETFILTER_ID_BUFFER 1 >>> +#define NETFILTER_ID_DUMP 2 >>> + >>> +extern const char *const netfilter_type_lookup[]; >>> + >>> typedef void (FilterSetup) (NetFilterState *nf, Error **errp); >>> typedef void (FilterCleanup) (NetFilterState *nf); >>> /* >>> @@ -55,6 +65,7 @@ struct NetFilterState { >>> char *netdev_id; >>> NetClientState *netdev; >>> NetFilterDirection direction; >>> + bool is_default; >>> bool enabled; >>> QTAILQ_ENTRY(NetFilterState) next; >>> }; >>> diff --git a/net/dump.c b/net/dump.c >>> index 88d9582..82727a6 100644 >>> --- a/net/dump.c >>> +++ b/net/dump.c >>> @@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts, const char *name, >>> >>> /* Dumping via filter */ >>> >>> -#define TYPE_FILTER_DUMP "filter-dump" >>> - >>> #define FILTER_DUMP(obj) \ >>> OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP) >>> >>> diff --git a/net/filter.c b/net/filter.c >>> index 4d96301..a126a3b 100644 >>> --- a/net/filter.c >>> +++ b/net/filter.c >>> @@ -21,6 +21,11 @@ >>> #include "qapi/qmp-input-visitor.h" >>> #include "monitor/monitor.h" >>> >>> +const char *const netfilter_type_lookup[] = { >>> + [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER, >>> + [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP, >>> +}; >>> + >>> ssize_t qemu_netfilter_receive(NetFilterState *nf, >>> NetFilterDirection direction, >>> NetClientState *sender, >>> @@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp) >>> NetFilterClass *nfc = NETFILTER_GET_CLASS(uc); >>> int queues; >>> Error *local_err = NULL; >>> - >>> + char *path = object_get_canonical_path_component(OBJECT(nf)); >>> >>> if (!nf->netdev_id) { >>> error_setg(errp, "Parameter 'netdev' is required"); >>> @@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable *uc, Error **errp) >>> } >>> >>> nf->netdev = ncs[0]; >>> + nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME); >>> + /* >>> + * For the default buffer filter, it will be disabled by default, >>> + * So it will not buffer any packets. >>> + */ >>> + if (nf->is_default) { >>> + nf->enabled = false; >>> + } >> >> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may >> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status" >> into properties. >> > > A little confused, do you mean add a 'default' property for filter ? > Just like the new 'status' property which is exported to users ? > Is the type of 'default' property string or bool ? > I don't think it is necessary to add a 'default' property, because we don't want such a property to be controlled by user. It is only used internally. We have another choice to deal with this, Add a 'bool is_default' parameter for netdev_add_filter(), and handle the default filter in it, just like: void netdev_add_filter(const char *netdev_id, const char *filter_type, const char *id, bool is_default, Error **errp) { ... ... object_add(filter_type, id, qdict, iv, &err); .... ... if (is_default) { Object *obj, *container; NetFilterState *nf; container = object_get_objects_root(); obj = object_resolve_path_component(container, id); if (!obj) { error_setg(errp, "object id not found"); return; } nf = NETFILTER(obj); nf->is_default = true; nf->enabled = false; } } Is this acceptable ? Thanks, Hailiang >>> >>> if (nfc->setup) { >>> nfc->setup(nf, &local_err); >>> diff --git a/net/net.c b/net/net.c >>> index ec43105..9630234 100644 >>> --- a/net/net.c >>> +++ b/net/net.c >>> @@ -76,6 +76,12 @@ const char *host_net_devices[] = { >>> >>> int default_net = 1; >>> >>> +/* >>> + * FIXME: Export this with an option for users to control >>> + * this with comand line ? >> >> This could be done in the future. >> > > Should i change the tag to 'TODO' ? > >>> + */ >>> +int default_netfilter = NETFILTER_ID_BUFFER; >> >> Why not just use a string here? >> > > No special, i will convert it to use string here. > >>> + >>> /***********************************************************/ >>> /* network device redirectors */ >>> >>> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp) >>> } >>> return -1; >>> } >>> + >>> + if (is_netdev) { >>> + const Netdev *netdev = object; >>> + /* >>> + * Here we add each netdev a default filter whose name is 'nop', >>> + * it will disabled by default, Users can enable it when necessary. >>> + */ >> >> If we support default properties, the above comment could be removed. >> >>> + netdev_add_filter(netdev->id, >>> + netfilter_type_lookup[default_netfilter], >>> + DEFAULT_FILTER_NAME, >> >> I believe some logic to generate id automatically is needed here. >> > > Yes, you are right, here this patch will break QEMU with multi-NICs configured, > the error report is " attempt to add duplicate property 'nop' to object". > I will fix it in next version. > > Thanks, > Hailiang > >>> + errp); >>> + } >>> return 0; >>> } >>> >> >> >> . >> >
On 01/25/2016 03:22 PM, Hailiang Zhang wrote: > On 2016/1/25 13:18, Jason Wang wrote: >> >> >> On 01/22/2016 04:36 PM, zhanghailiang wrote: >>> We add each netdev a default buffer filter, which the name is >>> 'nop', and the default buffer filter is disabled, so it has >>> no side effect for packets delivering in qemu net layer. >>> >>> The default buffer filter can be used by COLO or Micro-checkpoint, >>> The reason we add the default filter is we hope to support >>> hot add network during COLO state in future. >>> >>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>> --- >>> include/net/filter.h | 11 +++++++++++ >>> net/dump.c | 2 -- >>> net/filter.c | 15 ++++++++++++++- >>> net/net.c | 18 ++++++++++++++++++ >>> 4 files changed, 43 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/net/filter.h b/include/net/filter.h >>> index c7bd8f9..2043609 100644 >>> --- a/include/net/filter.h >>> +++ b/include/net/filter.h >>> @@ -22,6 +22,16 @@ >>> #define NETFILTER_CLASS(klass) \ >>> OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER) >>> [...] >>> >>> nf->netdev = ncs[0]; >>> + nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME); >>> + /* >>> + * For the default buffer filter, it will be disabled by default, >>> + * So it will not buffer any packets. >>> + */ >>> + if (nf->is_default) { >>> + nf->enabled = false; >>> + } >> >> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may >> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status" >> into properties. >> > > A little confused, do you mean add a 'default' property for filter ? > Just like the new 'status' property which is exported to users ? > Is the type of 'default' property string or bool ? For example, is it possible to store the default property into a string and just create the filter through qemu_opts_parse_noisily() by just pass a string as its parameter? (Of course, it needs some code to generate ids). With this, there's even no need for you to duplicate codes like what patch 4 does. > >>> >>> if (nfc->setup) { >>> nfc->setup(nf, &local_err); >>> diff --git a/net/net.c b/net/net.c >>> index ec43105..9630234 100644 >>> --- a/net/net.c >>> +++ b/net/net.c >>> @@ -76,6 +76,12 @@ const char *host_net_devices[] = { >>> >>> int default_net = 1; >>> >>> +/* >>> + * FIXME: Export this with an option for users to control >>> + * this with comand line ? >> >> This could be done in the future. >> > > Should i change the tag to 'TODO' ? Ok. > >>> + */ >>> +int default_netfilter = NETFILTER_ID_BUFFER; >> >> Why not just use a string here? >> > > No special, i will convert it to use string here. > >>> + >>> /***********************************************************/ >>> /* network device redirectors */ >>> >>> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void >>> *object, int is_netdev, Error **errp) >>> } >>> return -1; >>> } >>> + >>> + if (is_netdev) { >>> + const Netdev *netdev = object; >>> + /* >>> + * Here we add each netdev a default filter whose name is >>> 'nop', >>> + * it will disabled by default, Users can enable it when >>> necessary. >>> + */ >> >> If we support default properties, the above comment could be removed. >> >>> + netdev_add_filter(netdev->id, >>> + netfilter_type_lookup[default_netfilter], >>> + DEFAULT_FILTER_NAME, >> >> I believe some logic to generate id automatically is needed here. >> > > Yes, you are right, here this patch will break QEMU with multi-NICs > configured, > the error report is " attempt to add duplicate property 'nop' to object". > I will fix it in next version. > > Thanks, > Hailiang > >>> + errp); >>> + } >>> return 0; >>> } >>> >> >> >> . >> > >
On 2016/1/26 11:18, Jason Wang wrote: > > > On 01/25/2016 03:22 PM, Hailiang Zhang wrote: >> On 2016/1/25 13:18, Jason Wang wrote: >>> >>> >>> On 01/22/2016 04:36 PM, zhanghailiang wrote: >>>> We add each netdev a default buffer filter, which the name is >>>> 'nop', and the default buffer filter is disabled, so it has >>>> no side effect for packets delivering in qemu net layer. >>>> >>>> The default buffer filter can be used by COLO or Micro-checkpoint, >>>> The reason we add the default filter is we hope to support >>>> hot add network during COLO state in future. >>>> >>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>> --- >>>> include/net/filter.h | 11 +++++++++++ >>>> net/dump.c | 2 -- >>>> net/filter.c | 15 ++++++++++++++- >>>> net/net.c | 18 ++++++++++++++++++ >>>> 4 files changed, 43 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/include/net/filter.h b/include/net/filter.h >>>> index c7bd8f9..2043609 100644 >>>> --- a/include/net/filter.h >>>> +++ b/include/net/filter.h >>>> @@ -22,6 +22,16 @@ >>>> #define NETFILTER_CLASS(klass) \ >>>> OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER) >>>> > > [...] > >>>> >>>> nf->netdev = ncs[0]; >>>> + nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME); >>>> + /* >>>> + * For the default buffer filter, it will be disabled by default, >>>> + * So it will not buffer any packets. >>>> + */ >>>> + if (nf->is_default) { >>>> + nf->enabled = false; >>>> + } >>> >>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may >>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status" >>> into properties. >>> >> >> A little confused, do you mean add a 'default' property for filter ? >> Just like the new 'status' property which is exported to users ? >> Is the type of 'default' property string or bool ? > > For example, is it possible to store the default property into a string > and just create the filter through qemu_opts_parse_noisily() by just > pass a string as its parameter? (Of course, it needs some code to > generate ids). With this, there's even no need for you to duplicate > codes like what patch 4 does. > Er, got it, that seems to be a cleverer method, i will look at it, thanks very much for your patient explanation. >> >>>> >>>> if (nfc->setup) { >>>> nfc->setup(nf, &local_err); >>>> diff --git a/net/net.c b/net/net.c >>>> index ec43105..9630234 100644 >>>> --- a/net/net.c >>>> +++ b/net/net.c >>>> @@ -76,6 +76,12 @@ const char *host_net_devices[] = { >>>> >>>> int default_net = 1; >>>> >>>> +/* >>>> + * FIXME: Export this with an option for users to control >>>> + * this with comand line ? >>> >>> This could be done in the future. >>> >> >> Should i change the tag to 'TODO' ? > > Ok. > >> >>>> + */ >>>> +int default_netfilter = NETFILTER_ID_BUFFER; >>> >>> Why not just use a string here? >>> >> >> No special, i will convert it to use string here. >> >>>> + >>>> /***********************************************************/ >>>> /* network device redirectors */ >>>> >>>> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void >>>> *object, int is_netdev, Error **errp) >>>> } >>>> return -1; >>>> } >>>> + >>>> + if (is_netdev) { >>>> + const Netdev *netdev = object; >>>> + /* >>>> + * Here we add each netdev a default filter whose name is >>>> 'nop', >>>> + * it will disabled by default, Users can enable it when >>>> necessary. >>>> + */ >>> >>> If we support default properties, the above comment could be removed. >>> >>>> + netdev_add_filter(netdev->id, >>>> + netfilter_type_lookup[default_netfilter], >>>> + DEFAULT_FILTER_NAME, >>> >>> I believe some logic to generate id automatically is needed here. >>> >> >> Yes, you are right, here this patch will break QEMU with multi-NICs >> configured, >> the error report is " attempt to add duplicate property 'nop' to object". >> I will fix it in next version. >> >> Thanks, >> Hailiang >> >>>> + errp); >>>> + } >>>> return 0; >>>> } >>>> >>> >>> >>> . >>> >> >> > > > . >
On 2016/1/26 11:18, Jason Wang wrote: > > > On 01/25/2016 03:22 PM, Hailiang Zhang wrote: >> On 2016/1/25 13:18, Jason Wang wrote: >>> >>> >>> On 01/22/2016 04:36 PM, zhanghailiang wrote: >>>> We add each netdev a default buffer filter, which the name is >>>> 'nop', and the default buffer filter is disabled, so it has >>>> no side effect for packets delivering in qemu net layer. >>>> >>>> The default buffer filter can be used by COLO or Micro-checkpoint, >>>> The reason we add the default filter is we hope to support >>>> hot add network during COLO state in future. >>>> >>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>> --- >>>> include/net/filter.h | 11 +++++++++++ >>>> net/dump.c | 2 -- >>>> net/filter.c | 15 ++++++++++++++- >>>> net/net.c | 18 ++++++++++++++++++ >>>> 4 files changed, 43 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/include/net/filter.h b/include/net/filter.h >>>> index c7bd8f9..2043609 100644 >>>> --- a/include/net/filter.h >>>> +++ b/include/net/filter.h >>>> @@ -22,6 +22,16 @@ >>>> #define NETFILTER_CLASS(klass) \ >>>> OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER) >>>> > > [...] > >>>> >>>> nf->netdev = ncs[0]; >>>> + nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME); >>>> + /* >>>> + * For the default buffer filter, it will be disabled by default, >>>> + * So it will not buffer any packets. >>>> + */ >>>> + if (nf->is_default) { >>>> + nf->enabled = false; >>>> + } >>> >>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may >>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status" >>> into properties. >>> >> >> A little confused, do you mean add a 'default' property for filter ? >> Just like the new 'status' property which is exported to users ? >> Is the type of 'default' property string or bool ? > > For example, is it possible to store the default property into a string > and just create the filter through qemu_opts_parse_noisily() by just We still need to use some *visit* helpers to realize the capability, because the object_add() helper need a 'Visitor *v' parameter, and the codes will be like: QemuOptsList qemu_filter_opts = { .name = "default-filter", .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head), .desc = { { .name = "netdev", .type = QEMU_OPT_STRING, },{ .name = "status", .type = QEMU_OPT_STRING, }, { /* end of list */ } }, }; void netdev_add_filter(const char *netdev_id, const char *filter_type, const char *id, bool is_default, Error **errp) { sprintf(optarg, "netdev=%s,status=%s", netdev_id, is_default ? "disable" : "enable"); opts = qemu_opts_parse_noisily(&qemu_filter_opts, optarg, false); if (!opts) { error_report("Failed to parse param '%s'", optarg); exit(1); } qdict = qemu_opts_to_qdict(opts, NULL); ov = opts_visitor_new(opts); visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err); if (err) { goto out_clean; } object_add(filter_type, id, qdict, opts_get_visitor(ov), &err); if (err) { goto out_clean; } visit_end_struct(opts_get_visitor(ov), &err); if (err) { qmp_object_del(id, NULL); goto out_clean; } } Or, we can simplify patch 4 by using qmp_object_add(), codes will be like: void netdev_add_filter(const char *netdev_id, const char *filter_type, const char *id, bool is_default, Error **errp) { ... ... qov = qmp_output_visitor_new(); ov = qmp_output_get_visitor(qov); visit_start_struct(ov, &dummy, NULL, NULL, 0, &err); if (err) { goto out; } visit_type_str(ov, &nc->name, "netdev", &err); if (err) { goto out; } status = is_default ? g_strdup("disable") : g_strdup("enable"); visit_type_str(ov, &status, "status", &err); g_free(status); if (err) { goto out; } visit_end_struct(ov, &err); if (err) { goto out; } obj = qmp_output_get_qobject(qov); g_assert(obj != NULL); qmp_object_add(filter_type, id, true, obj, &err); qmp_output_visitor_cleanup(qov); qobject_decref(obj); } what's your suggestion ? :) Thanks, Hailiang > pass a string as its parameter? (Of course, it needs some code to > generate ids). With this, there's even no need for you to duplicate > codes like what patch 4 does. > >> >>>> >>>> if (nfc->setup) { >>>> nfc->setup(nf, &local_err); >>>> diff --git a/net/net.c b/net/net.c >>>> index ec43105..9630234 100644 >>>> --- a/net/net.c >>>> +++ b/net/net.c >>>> @@ -76,6 +76,12 @@ const char *host_net_devices[] = { >>>> >>>> int default_net = 1; >>>> >>>> +/* >>>> + * FIXME: Export this with an option for users to control >>>> + * this with comand line ? >>> >>> This could be done in the future. >>> >> >> Should i change the tag to 'TODO' ? > > Ok. > >> >>>> + */ >>>> +int default_netfilter = NETFILTER_ID_BUFFER; >>> >>> Why not just use a string here? >>> >> >> No special, i will convert it to use string here. >> >>>> + >>>> /***********************************************************/ >>>> /* network device redirectors */ >>>> >>>> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void >>>> *object, int is_netdev, Error **errp) >>>> } >>>> return -1; >>>> } >>>> + >>>> + if (is_netdev) { >>>> + const Netdev *netdev = object; >>>> + /* >>>> + * Here we add each netdev a default filter whose name is >>>> 'nop', >>>> + * it will disabled by default, Users can enable it when >>>> necessary. >>>> + */ >>> >>> If we support default properties, the above comment could be removed. >>> >>>> + netdev_add_filter(netdev->id, >>>> + netfilter_type_lookup[default_netfilter], >>>> + DEFAULT_FILTER_NAME, >>> >>> I believe some logic to generate id automatically is needed here. >>> >> >> Yes, you are right, here this patch will break QEMU with multi-NICs >> configured, >> the error report is " attempt to add duplicate property 'nop' to object". >> I will fix it in next version. >> >> Thanks, >> Hailiang >> >>>> + errp); >>>> + } >>>> return 0; >>>> } >>>> >>> >>> >>> . >>> >> >> > > > . >
On 01/25/2016 07:19 PM, Hailiang Zhang wrote: > On 2016/1/25 15:22, Hailiang Zhang wrote: >> On 2016/1/25 13:18, Jason Wang wrote: >>> >>> >>> On 01/22/2016 04:36 PM, zhanghailiang wrote: >>>> We add each netdev a default buffer filter, which the name is >>>> 'nop', and the default buffer filter is disabled, so it has >>>> no side effect for packets delivering in qemu net layer. >>>> >>>> The default buffer filter can be used by COLO or Micro-checkpoint, >>>> The reason we add the default filter is we hope to support >>>> hot add network during COLO state in future. >>>> >>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>> --- >>>> include/net/filter.h | 11 +++++++++++ >>>> net/dump.c | 2 -- >>>> net/filter.c | 15 ++++++++++++++- >>>> net/net.c | 18 ++++++++++++++++++ >>>> 4 files changed, 43 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/include/net/filter.h b/include/net/filter.h >>>> index c7bd8f9..2043609 100644 >>>> --- a/include/net/filter.h >>>> +++ b/include/net/filter.h >>>> @@ -22,6 +22,16 @@ >>>> #define NETFILTER_CLASS(klass) \ >>>> OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER) >>>> >>>> +#define DEFAULT_FILTER_NAME "nop" >>> >>> Maybe DEFAULT_FILTER_TYPE? >>> >> >>>> + >>>> +#define TYPE_FILTER_BUFFER "filter-buffer" >>>> +#define TYPE_FILTER_DUMP "filter-dump" >>>> + >>>> +#define NETFILTER_ID_BUFFER 1 >>>> +#define NETFILTER_ID_DUMP 2 >>>> + >>>> +extern const char *const netfilter_type_lookup[]; >>>> + >>>> typedef void (FilterSetup) (NetFilterState *nf, Error **errp); >>>> typedef void (FilterCleanup) (NetFilterState *nf); >>>> /* >>>> @@ -55,6 +65,7 @@ struct NetFilterState { >>>> char *netdev_id; >>>> NetClientState *netdev; >>>> NetFilterDirection direction; >>>> + bool is_default; >>>> bool enabled; >>>> QTAILQ_ENTRY(NetFilterState) next; >>>> }; >>>> diff --git a/net/dump.c b/net/dump.c >>>> index 88d9582..82727a6 100644 >>>> --- a/net/dump.c >>>> +++ b/net/dump.c >>>> @@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts, >>>> const char *name, >>>> >>>> /* Dumping via filter */ >>>> >>>> -#define TYPE_FILTER_DUMP "filter-dump" >>>> - >>>> #define FILTER_DUMP(obj) \ >>>> OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP) >>>> >>>> diff --git a/net/filter.c b/net/filter.c >>>> index 4d96301..a126a3b 100644 >>>> --- a/net/filter.c >>>> +++ b/net/filter.c >>>> @@ -21,6 +21,11 @@ >>>> #include "qapi/qmp-input-visitor.h" >>>> #include "monitor/monitor.h" >>>> >>>> +const char *const netfilter_type_lookup[] = { >>>> + [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER, >>>> + [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP, >>>> +}; >>>> + >>>> ssize_t qemu_netfilter_receive(NetFilterState *nf, >>>> NetFilterDirection direction, >>>> NetClientState *sender, >>>> @@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable >>>> *uc, Error **errp) >>>> NetFilterClass *nfc = NETFILTER_GET_CLASS(uc); >>>> int queues; >>>> Error *local_err = NULL; >>>> - >>>> + char *path = object_get_canonical_path_component(OBJECT(nf)); >>>> >>>> if (!nf->netdev_id) { >>>> error_setg(errp, "Parameter 'netdev' is required"); >>>> @@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable >>>> *uc, Error **errp) >>>> } >>>> >>>> nf->netdev = ncs[0]; >>>> + nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME); >>>> + /* >>>> + * For the default buffer filter, it will be disabled by default, >>>> + * So it will not buffer any packets. >>>> + */ >>>> + if (nf->is_default) { >>>> + nf->enabled = false; >>>> + } >>> >>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may >>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status" >>> into properties. >>> >> >> A little confused, do you mean add a 'default' property for filter ? >> Just like the new 'status' property which is exported to users ? >> Is the type of 'default' property string or bool ? >> > > I don't think it is necessary to add a 'default' property, because > we don't want such a property to be controlled by user. It is > only used internally. If it was only used internally, then just not export the ability to configure default filter and its properties to user. > We have another choice to deal with this, > Add a 'bool is_default' parameter for netdev_add_filter(), > and handle the default filter in it, just like: This does not scale consider if somebody want to add more common properties to netfilters. > > void netdev_add_filter(const char *netdev_id, > const char *filter_type, > const char *id, > bool is_default, > Error **errp) > { > ... ... > object_add(filter_type, id, qdict, iv, &err); > .... ... > if (is_default) { > Object *obj, *container; > NetFilterState *nf; > > container = object_get_objects_root(); > obj = object_resolve_path_component(container, id); > if (!obj) { > error_setg(errp, "object id not found"); > return; > } > nf = NETFILTER(obj); > nf->is_default = true; > nf->enabled = false; > } > > } > > Is this acceptable ? > > Thanks, > Hailiang > >>>> >>>> if (nfc->setup) { >>>> nfc->setup(nf, &local_err); >>>> diff --git a/net/net.c b/net/net.c >>>> index ec43105..9630234 100644 >>>> --- a/net/net.c >>>> +++ b/net/net.c >>>> @@ -76,6 +76,12 @@ const char *host_net_devices[] = { >>>> >>>> int default_net = 1; >>>> >>>> +/* >>>> + * FIXME: Export this with an option for users to control >>>> + * this with comand line ? >>> >>> This could be done in the future. >>> >> >> Should i change the tag to 'TODO' ? >> >>>> + */ >>>> +int default_netfilter = NETFILTER_ID_BUFFER; >>> >>> Why not just use a string here? >>> >> >> No special, i will convert it to use string here. >> >>>> + >>>> /***********************************************************/ >>>> /* network device redirectors */ >>>> >>>> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void >>>> *object, int is_netdev, Error **errp) >>>> } >>>> return -1; >>>> } >>>> + >>>> + if (is_netdev) { >>>> + const Netdev *netdev = object; >>>> + /* >>>> + * Here we add each netdev a default filter whose name is >>>> 'nop', >>>> + * it will disabled by default, Users can enable it when >>>> necessary. >>>> + */ >>> >>> If we support default properties, the above comment could be removed. >>> >>>> + netdev_add_filter(netdev->id, >>>> + netfilter_type_lookup[default_netfilter], >>>> + DEFAULT_FILTER_NAME, >>> >>> I believe some logic to generate id automatically is needed here. >>> >> >> Yes, you are right, here this patch will break QEMU with multi-NICs >> configured, >> the error report is " attempt to add duplicate property 'nop' to >> object". >> I will fix it in next version. >> >> Thanks, >> Hailiang >> >>>> + errp); >>>> + } >>>> return 0; >>>> } >>>> >>> >>> >>> . >>> >> > > >
On 01/27/2016 08:37 AM, Hailiang Zhang wrote: > On 2016/1/26 11:18, Jason Wang wrote: >> >> >> On 01/25/2016 03:22 PM, Hailiang Zhang wrote: >>> On 2016/1/25 13:18, Jason Wang wrote: >>>> >>>> >>>> On 01/22/2016 04:36 PM, zhanghailiang wrote: >>>>> We add each netdev a default buffer filter, which the name is >>>>> 'nop', and the default buffer filter is disabled, so it has >>>>> no side effect for packets delivering in qemu net layer. >>>>> >>>>> The default buffer filter can be used by COLO or Micro-checkpoint, >>>>> The reason we add the default filter is we hope to support >>>>> hot add network during COLO state in future. >>>>> >>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>>> --- >>>>> include/net/filter.h | 11 +++++++++++ >>>>> net/dump.c | 2 -- >>>>> net/filter.c | 15 ++++++++++++++- >>>>> net/net.c | 18 ++++++++++++++++++ >>>>> 4 files changed, 43 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/include/net/filter.h b/include/net/filter.h >>>>> index c7bd8f9..2043609 100644 >>>>> --- a/include/net/filter.h >>>>> +++ b/include/net/filter.h >>>>> @@ -22,6 +22,16 @@ >>>>> #define NETFILTER_CLASS(klass) \ >>>>> OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER) >>>>> >> >> [...] >> >>>>> >>>>> nf->netdev = ncs[0]; >>>>> + nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME); >>>>> + /* >>>>> + * For the default buffer filter, it will be disabled by default, >>>>> + * So it will not buffer any packets. >>>>> + */ >>>>> + if (nf->is_default) { >>>>> + nf->enabled = false; >>>>> + } >>>> >>>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may >>>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status" >>>> into properties. >>>> >>> >>> A little confused, do you mean add a 'default' property for filter ? >>> Just like the new 'status' property which is exported to users ? >>> Is the type of 'default' property string or bool ? >> >> For example, is it possible to store the default property into a string >> and just create the filter through qemu_opts_parse_noisily() by just > > We still need to use some *visit* helpers to realize the capability, > because the object_add() helper need a 'Visitor *v' parameter, and the > codes > will be like: > QemuOptsList qemu_filter_opts = { > .name = "default-filter", > .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head), > .desc = { > { > .name = "netdev", > .type = QEMU_OPT_STRING, > },{ > .name = "status", > .type = QEMU_OPT_STRING, > }, > { /* end of list */ } > }, > }; > void netdev_add_filter(const char *netdev_id, > const char *filter_type, > const char *id, > bool is_default, > Error **errp) > { > sprintf(optarg, "netdev=%s,status=%s", netdev_id, > is_default ? "disable" : "enable"); > opts = qemu_opts_parse_noisily(&qemu_filter_opts, > optarg, false); > if (!opts) { > error_report("Failed to parse param '%s'", optarg); > exit(1); > } > > qdict = qemu_opts_to_qdict(opts, NULL); > ov = opts_visitor_new(opts); > visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, > &err); > if (err) { > goto out_clean; > } > object_add(filter_type, id, qdict, opts_get_visitor(ov), &err); > if (err) { > goto out_clean; > } > > visit_end_struct(opts_get_visitor(ov), &err); > if (err) { > qmp_object_del(id, NULL); > goto out_clean; > } > > } > > Or, we can simplify patch 4 by using qmp_object_add(), codes will be > like: > > void netdev_add_filter(const char *netdev_id, > const char *filter_type, > const char *id, > bool is_default, > Error **errp) > { > ... ... > > qov = qmp_output_visitor_new(); > ov = qmp_output_get_visitor(qov); > visit_start_struct(ov, &dummy, NULL, NULL, 0, &err); > if (err) { > goto out; > } > visit_type_str(ov, &nc->name, "netdev", &err); > if (err) { > goto out; > } > status = is_default ? g_strdup("disable") : g_strdup("enable"); > visit_type_str(ov, &status, "status", &err); > g_free(status); > if (err) { > goto out; > } > visit_end_struct(ov, &err); > if (err) { > goto out; > } > obj = qmp_output_get_qobject(qov); > g_assert(obj != NULL); > qmp_object_add(filter_type, id, true, obj, &err); > qmp_output_visitor_cleanup(qov); > qobject_decref(obj); > > } > > what's your suggestion ? :) > Can we just reuse object_create()? here > Thanks, > Hailiang
On 2016/1/27 13:59, Jason Wang wrote: > > > On 01/27/2016 08:37 AM, Hailiang Zhang wrote: >> On 2016/1/26 11:18, Jason Wang wrote: >>> >>> >>> On 01/25/2016 03:22 PM, Hailiang Zhang wrote: >>>> On 2016/1/25 13:18, Jason Wang wrote: >>>>> >>>>> >>>>> On 01/22/2016 04:36 PM, zhanghailiang wrote: >>>>>> We add each netdev a default buffer filter, which the name is >>>>>> 'nop', and the default buffer filter is disabled, so it has >>>>>> no side effect for packets delivering in qemu net layer. >>>>>> >>>>>> The default buffer filter can be used by COLO or Micro-checkpoint, >>>>>> The reason we add the default filter is we hope to support >>>>>> hot add network during COLO state in future. >>>>>> >>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>>>> --- >>>>>> include/net/filter.h | 11 +++++++++++ >>>>>> net/dump.c | 2 -- >>>>>> net/filter.c | 15 ++++++++++++++- >>>>>> net/net.c | 18 ++++++++++++++++++ >>>>>> 4 files changed, 43 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/include/net/filter.h b/include/net/filter.h >>>>>> index c7bd8f9..2043609 100644 >>>>>> --- a/include/net/filter.h >>>>>> +++ b/include/net/filter.h >>>>>> @@ -22,6 +22,16 @@ >>>>>> #define NETFILTER_CLASS(klass) \ >>>>>> OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER) >>>>>> >>> >>> [...] >>> >>>>>> >>>>>> nf->netdev = ncs[0]; >>>>>> + nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME); >>>>>> + /* >>>>>> + * For the default buffer filter, it will be disabled by default, >>>>>> + * So it will not buffer any packets. >>>>>> + */ >>>>>> + if (nf->is_default) { >>>>>> + nf->enabled = false; >>>>>> + } >>>>> >>>>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may >>>>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status" >>>>> into properties. >>>>> >>>> >>>> A little confused, do you mean add a 'default' property for filter ? >>>> Just like the new 'status' property which is exported to users ? >>>> Is the type of 'default' property string or bool ? >>> >>> For example, is it possible to store the default property into a string >>> and just create the filter through qemu_opts_parse_noisily() by just >> >> We still need to use some *visit* helpers to realize the capability, >> because the object_add() helper need a 'Visitor *v' parameter, and the >> codes >> will be like: >> QemuOptsList qemu_filter_opts = { >> .name = "default-filter", >> .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head), >> .desc = { >> { >> .name = "netdev", >> .type = QEMU_OPT_STRING, >> },{ >> .name = "status", >> .type = QEMU_OPT_STRING, >> }, >> { /* end of list */ } >> }, >> }; >> void netdev_add_filter(const char *netdev_id, >> const char *filter_type, >> const char *id, >> bool is_default, >> Error **errp) >> { >> sprintf(optarg, "netdev=%s,status=%s", netdev_id, >> is_default ? "disable" : "enable"); >> opts = qemu_opts_parse_noisily(&qemu_filter_opts, >> optarg, false); >> if (!opts) { >> error_report("Failed to parse param '%s'", optarg); >> exit(1); >> } >> >> qdict = qemu_opts_to_qdict(opts, NULL); >> ov = opts_visitor_new(opts); >> visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, >> &err); >> if (err) { >> goto out_clean; >> } >> object_add(filter_type, id, qdict, opts_get_visitor(ov), &err); >> if (err) { >> goto out_clean; >> } >> >> visit_end_struct(opts_get_visitor(ov), &err); >> if (err) { >> qmp_object_del(id, NULL); >> goto out_clean; >> } >> >> } >> >> Or, we can simplify patch 4 by using qmp_object_add(), codes will be >> like: >> >> void netdev_add_filter(const char *netdev_id, >> const char *filter_type, >> const char *id, >> bool is_default, >> Error **errp) >> { >> ... ... >> >> qov = qmp_output_visitor_new(); >> ov = qmp_output_get_visitor(qov); >> visit_start_struct(ov, &dummy, NULL, NULL, 0, &err); >> if (err) { >> goto out; >> } >> visit_type_str(ov, &nc->name, "netdev", &err); >> if (err) { >> goto out; >> } >> status = is_default ? g_strdup("disable") : g_strdup("enable"); >> visit_type_str(ov, &status, "status", &err); >> g_free(status); >> if (err) { >> goto out; >> } >> visit_end_struct(ov, &err); >> if (err) { >> goto out; >> } >> obj = qmp_output_get_qobject(qov); >> g_assert(obj != NULL); >> qmp_object_add(filter_type, id, true, obj, &err); >> qmp_output_visitor_cleanup(qov); >> qobject_decref(obj); >> >> } >> >> what's your suggestion ? :) >> > > Can we just reuse object_create()? here > Yes, the codes is more clean if we reuse it. I will fix it like that in v2, thanks. >> Thanks, >> Hailiang > > > . >
diff --git a/include/net/filter.h b/include/net/filter.h index c7bd8f9..2043609 100644 --- a/include/net/filter.h +++ b/include/net/filter.h @@ -22,6 +22,16 @@ #define NETFILTER_CLASS(klass) \ OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER) +#define DEFAULT_FILTER_NAME "nop" + +#define TYPE_FILTER_BUFFER "filter-buffer" +#define TYPE_FILTER_DUMP "filter-dump" + +#define NETFILTER_ID_BUFFER 1 +#define NETFILTER_ID_DUMP 2 + +extern const char *const netfilter_type_lookup[]; + typedef void (FilterSetup) (NetFilterState *nf, Error **errp); typedef void (FilterCleanup) (NetFilterState *nf); /* @@ -55,6 +65,7 @@ struct NetFilterState { char *netdev_id; NetClientState *netdev; NetFilterDirection direction; + bool is_default; bool enabled; QTAILQ_ENTRY(NetFilterState) next; }; diff --git a/net/dump.c b/net/dump.c index 88d9582..82727a6 100644 --- a/net/dump.c +++ b/net/dump.c @@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts, const char *name, /* Dumping via filter */ -#define TYPE_FILTER_DUMP "filter-dump" - #define FILTER_DUMP(obj) \ OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP) diff --git a/net/filter.c b/net/filter.c index 4d96301..a126a3b 100644 --- a/net/filter.c +++ b/net/filter.c @@ -21,6 +21,11 @@ #include "qapi/qmp-input-visitor.h" #include "monitor/monitor.h" +const char *const netfilter_type_lookup[] = { + [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER, + [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP, +}; + ssize_t qemu_netfilter_receive(NetFilterState *nf, NetFilterDirection direction, NetClientState *sender, @@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp) NetFilterClass *nfc = NETFILTER_GET_CLASS(uc); int queues; Error *local_err = NULL; - + char *path = object_get_canonical_path_component(OBJECT(nf)); if (!nf->netdev_id) { error_setg(errp, "Parameter 'netdev' is required"); @@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable *uc, Error **errp) } nf->netdev = ncs[0]; + nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME); + /* + * For the default buffer filter, it will be disabled by default, + * So it will not buffer any packets. + */ + if (nf->is_default) { + nf->enabled = false; + } if (nfc->setup) { nfc->setup(nf, &local_err); diff --git a/net/net.c b/net/net.c index ec43105..9630234 100644 --- a/net/net.c +++ b/net/net.c @@ -76,6 +76,12 @@ const char *host_net_devices[] = { int default_net = 1; +/* + * FIXME: Export this with an option for users to control + * this with comand line ? + */ +int default_netfilter = NETFILTER_ID_BUFFER; + /***********************************************************/ /* network device redirectors */ @@ -1032,6 +1038,18 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp) } return -1; } + + if (is_netdev) { + const Netdev *netdev = object; + /* + * Here we add each netdev a default filter whose name is 'nop', + * it will disabled by default, Users can enable it when necessary. + */ + netdev_add_filter(netdev->id, + netfilter_type_lookup[default_netfilter], + DEFAULT_FILTER_NAME, + errp); + } return 0; }
We add each netdev a default buffer filter, which the name is 'nop', and the default buffer filter is disabled, so it has no side effect for packets delivering in qemu net layer. The default buffer filter can be used by COLO or Micro-checkpoint, The reason we add the default filter is we hope to support hot add network during COLO state in future. Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> --- include/net/filter.h | 11 +++++++++++ net/dump.c | 2 -- net/filter.c | 15 ++++++++++++++- net/net.c | 18 ++++++++++++++++++ 4 files changed, 43 insertions(+), 3 deletions(-)