Re: [Qemu-devel] [PATCH v12 02/10] init/cleanup of netfilter object

[Yang Hongyang <yanghy@cn.fujitsu.com>]

On 10/01/2015 12:59 AM, Markus Armbruster wrote:
> Yang Hongyang <yanghy@cn.fujitsu.com> writes:
>
>> Add a netfilter object based on QOM.
>>
>> A netfilter is attached to a netdev, captures all network packets
>> that pass through the netdev. When we delete the netdev, we also
>> delete the netfilter object attached to it, because if the netdev is
>> removed, the filter which attached to it is useless.
>>
>> QTAILQ_ENTRY next used by netdev, filter belongs to the specific netdev is
>> in this queue.
>
> I'm afraid this paragraph is incomprehensible.  What are you trying to
> say?
>
>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>> ---
>>   include/net/filter.h    |  62 ++++++++++++++++++++++
>>   include/net/net.h       |   1 +
>>   include/qemu/typedefs.h |   1 +
>>   net/Makefile.objs       |   1 +
>>   net/filter.c            | 138 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   net/net.c               |   7 +++
>>   qapi-schema.json        |  19 +++++++
>>   7 files changed, 229 insertions(+)
>>   create mode 100644 include/net/filter.h
>>   create mode 100644 net/filter.c
>>
>> diff --git a/include/net/filter.h b/include/net/filter.h
>> new file mode 100644
>> index 0000000..e3e14ea
>> --- /dev/null
>> +++ b/include/net/filter.h
>> @@ -0,0 +1,62 @@
>> +/*
>> + * Copyright (c) 2015 FUJITSU LIMITED
>> + * Author: Yang Hongyang <yanghy@cn.fujitsu.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later.  See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef QEMU_NET_FILTER_H
>> +#define QEMU_NET_FILTER_H
>> +
>> +#include "qom/object.h"
>> +#include "qemu-common.h"
>> +#include "qemu/typedefs.h"
>> +#include "net/queue.h"
>> +
>> +#define TYPE_NETFILTER "netfilter"
>> +#define NETFILTER(obj) \
>> +    OBJECT_CHECK(NetFilterState, (obj), TYPE_NETFILTER)
>> +#define NETFILTER_GET_CLASS(obj) \
>> +    OBJECT_GET_CLASS(NetFilterClass, (obj), TYPE_NETFILTER)
>> +#define NETFILTER_CLASS(klass) \
>> +    OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>> +
>> +typedef void (FilterSetup) (NetFilterState *nf, Error **errp);
>> +typedef void (FilterCleanup) (NetFilterState *nf);
>> +/*
>> + * Return:
>> + *   0: finished handling the packet, we should continue
>> + *   size: filter stolen this packet, we stop pass this packet further
>> + */
>> +typedef ssize_t (FilterReceiveIOV)(NetFilterState *nc,
>> +                                   NetClientState *sender,
>> +                                   unsigned flags,
>> +                                   const struct iovec *iov,
>> +                                   int iovcnt,
>> +                                   NetPacketSent *sent_cb);
>> +
>> +struct NetFilterClass {
>> +    ObjectClass parent_class;
>> +
>> +    /* optional */
>> +    FilterSetup *setup;
>> +    FilterCleanup *cleanup;
>> +    /* mandatory */
>> +    FilterReceiveIOV *receive_iov;
>> +};
>> +typedef struct NetFilterClass NetFilterClass;
>
> No separate typedef, please.

Seems this check in checkpatch.pl still present, it will report error
if combine the typedef, but I will do as you said anyway.

>
>> +
>> +
>> +struct NetFilterState {
>> +    /* private */
>> +    Object parent;
>> +
>> +    /* protected */
>> +    char *netdev_id;
>> +    NetClientState *netdev;
>> +    NetFilterDirection direction;
>> +    QTAILQ_ENTRY(NetFilterState) next;
>> +};
>> +
>> +#endif /* QEMU_NET_FILTER_H */
>> diff --git a/include/net/net.h b/include/net/net.h
>> index 6a6cbef..36e5fab 100644
>> --- a/include/net/net.h
>> +++ b/include/net/net.h
>> @@ -92,6 +92,7 @@ struct NetClientState {
>>       NetClientDestructor *destructor;
>>       unsigned int queue_index;
>>       unsigned rxfilter_notify_enabled:1;
>> +    QTAILQ_HEAD(, NetFilterState) filters;
>>   };
>>
>>   typedef struct NICState {
>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
>> index 3a835ff..ee1ce1d 100644
>> --- a/include/qemu/typedefs.h
>> +++ b/include/qemu/typedefs.h
>> @@ -45,6 +45,7 @@ typedef struct Monitor Monitor;
>>   typedef struct MouseTransformInfo MouseTransformInfo;
>>   typedef struct MSIMessage MSIMessage;
>>   typedef struct NetClientState NetClientState;
>> +typedef struct NetFilterState NetFilterState;
>>   typedef struct NICInfo NICInfo;
>>   typedef struct PcGuestInfo PcGuestInfo;
>>   typedef struct PCIBridge PCIBridge;
>> diff --git a/net/Makefile.objs b/net/Makefile.objs
>> index ec19cb3..914aec0 100644
>> --- a/net/Makefile.objs
>> +++ b/net/Makefile.objs
>> @@ -13,3 +13,4 @@ common-obj-$(CONFIG_HAIKU) += tap-haiku.o
>>   common-obj-$(CONFIG_SLIRP) += slirp.o
>>   common-obj-$(CONFIG_VDE) += vde.o
>>   common-obj-$(CONFIG_NETMAP) += netmap.o
>> +common-obj-y += filter.o
>> diff --git a/net/filter.c b/net/filter.c
>> new file mode 100644
>> index 0000000..34e32cd
>> --- /dev/null
>> +++ b/net/filter.c
>> @@ -0,0 +1,138 @@
>> +/*
>> + * Copyright (c) 2015 FUJITSU LIMITED
>> + * Author: Yang Hongyang <yanghy@cn.fujitsu.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later.  See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu-common.h"
>> +#include "qapi/qmp/qerror.h"
>> +#include "qemu/error-report.h"
>> +
>> +#include "net/filter.h"
>> +#include "net/net.h"
>> +#include "net/vhost_net.h"
>> +#include "qom/object_interfaces.h"
>> +
>> +static char *netfilter_get_netdev_id(Object *obj, Error **errp)
>> +{
>> +    NetFilterState *nf = NETFILTER(obj);
>> +
>> +    return g_strdup(nf->netdev_id);
>> +}
>> +
>> +static void netfilter_set_netdev_id(Object *obj, const char *str, Error **errp)
>> +{
>> +    NetFilterState *nf = NETFILTER(obj);
>> +
>> +    nf->netdev_id = g_strdup(str);
>> +}
>> +
>> +static int netfilter_get_direction(Object *obj, Error **errp G_GNUC_UNUSED)
>> +{
>> +    NetFilterState *nf = NETFILTER(obj);
>> +    return nf->direction;
>> +}
>> +
>> +static void netfilter_set_direction(Object *obj, int direction, Error **errp)
>> +{
>> +    NetFilterState *nf = NETFILTER(obj);
>> +    nf->direction = direction;
>> +}
>> +
>> +static void netfilter_init(Object *obj)
>> +{
>> +    object_property_add_str(obj, "netdev",
>> +                            netfilter_get_netdev_id, netfilter_set_netdev_id,
>> +                            NULL);
>> +    object_property_add_enum(obj, "queue", "NetFilterDirection",
>> +                             NetFilterDirection_lookup,
>> +                             netfilter_get_direction, netfilter_set_direction,
>> +                             NULL);
>> +}
>> +
>> +static void netfilter_finalize(Object *obj)
>> +{
>> +    NetFilterState *nf = NETFILTER(obj);
>> +    NetFilterClass *nfc = NETFILTER_GET_CLASS(obj);
>> +
>> +    if (nfc->cleanup) {
>> +        nfc->cleanup(nf);
>> +    }
>> +
>> +    if (nf->netdev && !QTAILQ_EMPTY(&nf->netdev->filters)) {
>> +        QTAILQ_REMOVE(&nf->netdev->filters, nf, next);
>> +    }
>> +}
>
> I still recommend to put netfilter_finalize() after
> netfilter_complete(), because that's how we order creation and
> destruction elsewhere.
>
>> +
>> +static void netfilter_complete(UserCreatable *uc, Error **errp)
>> +{
>> +    NetFilterState *nf = NETFILTER(uc);
>> +    NetClientState *ncs[MAX_QUEUE_NUM];
>> +    NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
>> +    int queues;
>> +    Error *local_err = NULL;
>> +
>> +    if (!nf->netdev_id) {
>> +        error_setg(errp, "Parameter 'netdev' is required");
>> +        return;
>> +    }
>> +
>> +    queues = qemu_find_net_clients_except(nf->netdev_id, ncs,
>> +                                          NET_CLIENT_OPTIONS_KIND_NIC,
>> +                                          MAX_QUEUE_NUM);
>> +    if (queues < 1) {
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "netdev",
>> +                   "a network backend id");
>> +        return;
>> +    } else if (queues > 1) {
>> +        error_setg(errp, "multiqueue is not supported");
>> +        return;
>> +    }
>> +
>> +    if (get_vhost_net(ncs[0])) {
>> +        error_setg(errp, "Vhost is not supported");
>> +        return;
>> +    }
>> +
>> +    nf->netdev = ncs[0];
>> +
>> +    if (nfc->setup) {
>> +        nfc->setup(nf, &local_err);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>> +    }
>> +    QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>> +}
>> +
>> +static void netfilter_class_init(ObjectClass *oc, void *data)
>> +{
>> +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
>> +
>> +    ucc->complete = netfilter_complete;
>> +}
>> +
>> +static const TypeInfo netfilter_info = {
>> +    .name = TYPE_NETFILTER,
>> +    .parent = TYPE_OBJECT,
>> +    .abstract = true,
>> +    .class_size = sizeof(NetFilterClass),
>> +    .class_init = netfilter_class_init,
>> +    .instance_size = sizeof(NetFilterState),
>> +    .instance_init = netfilter_init,
>> +    .instance_finalize = netfilter_finalize,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_USER_CREATABLE },
>> +        { }
>> +    }
>> +};
>> +
>> +static void register_types(void)
>> +{
>> +    type_register_static(&netfilter_info);
>> +}
>> +
>> +type_init(register_types);
>> diff --git a/net/net.c b/net/net.c
>> index 28a5597..033f4f3 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -44,6 +44,7 @@
>>   #include "qapi/opts-visitor.h"
>>   #include "qapi/dealloc-visitor.h"
>>   #include "sysemu/sysemu.h"
>> +#include "net/filter.h"
>>
>>   /* Net bridge is currently not supported for W32. */
>>   #if !defined(_WIN32)
>> @@ -287,6 +288,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,
>> @@ -384,6 +386,7 @@ void qemu_del_net_client(NetClientState *nc)
>>   {
>>       NetClientState *ncs[MAX_QUEUE_NUM];
>>       int queues, i;
>> +    NetFilterState *nf, *next;
>>
>>       assert(nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC);
>>
>> @@ -395,6 +398,10 @@ void qemu_del_net_client(NetClientState *nc)
>>                                             MAX_QUEUE_NUM);
>>       assert(queues != 0);
>>
>> +    QTAILQ_FOREACH_SAFE(nf, &nc->filters, next, next) {
>> +        object_unparent(OBJECT(nf));
>> +    }
>> +
>>       /* If there is a peer NIC, delete and cleanup client, but do not free. */
>>       if (nc->peer && nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
>>           NICState *nic = qemu_get_nic(nc->peer);
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 582a817..a58e8f4 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -2556,6 +2556,25 @@
>>       'opts': 'NetClientOptions' } }
>>
>>   ##
>> +# @NetFilterDirection
>> +#
>> +# Indicates whether a netfilter is attached to a netdev's transmit queue or
>> +# receive queue or both.
>> +#
>> +# @all: the filter will receive packets both sent to/by the netdev (default).
>
> Make this
>
>      @all: the filter is attached both to the receive and the transmit
>      queue of the netdev.
>
>> +#
>> +# @rx: the filter is attached to the receive queue of the netdev,
>> +#      will receive packets sent to the netdev.
>> +#
>> +# @tx: the filter is attached to the transmit queue of the netdev,
>> +#      will receive packets sent by the netdev.
>> +#
>> +# Since 2.5
>> +##
>> +{ 'enum': 'NetFilterDirection',
>> +  'data': [ 'all', 'rx', 'tx' ] }
>> +
>> +##
>>   # @InetSocketAddress
>>   #
>>   # Captures a socket address or address range in the Internet namespace.
> .
>

-- 
Thanks,
Yang.