Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Jason Wang]

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 <address@hidden>
>>>> ---
>>>>   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;
>>>>   }
>>>>
>>>
>>>
>>> .
>>>
>>
>
>
>