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

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

Hi Thomas,
   Thanks for the review.

On 07/29/2015 09:33 PM, Thomas Huth wrote:
> On Wednesday, July 29, 2015 12:51:46 PM,
> "Yang Hongyang" <yanghy@cn.fujitsu.com> wrote:
>>
>> This is mostly the same with init/cleanup of netdev object.
>>
>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>> ---
>>   include/net/filter.h    |  21 ++++++++
>>   include/qemu/typedefs.h |   1 +
>>   net/filter.c            | 131
>>   ++++++++++++++++++++++++++++++++++++++++++++++++
>>   qapi-schema.json        |  30 +++++++++++
>>   4 files changed, 183 insertions(+)
>>
> [...]
>> diff --git a/net/filter.c b/net/filter.c
>> index 4e40f08..e6fdc26 100644
>> --- a/net/filter.c
>> +++ b/net/filter.c
>> @@ -6,10 +6,141 @@
>>    */
>>
>>   #include "qemu-common.h"
>> +#include "qapi-visit.h"
>> +#include "qapi/qmp/qerror.h"
>> +#include "qemu/error-report.h"
>> +#include "qapi-visit.h"
>> +#include "qapi/opts-visitor.h"
>> +#include "qapi/dealloc-visitor.h"
>> +#include "qemu/config-file.h"
>> +
>>   #include "net/filter.h"
>> +#include "net/net.h"
>> +
>> +static QTAILQ_HEAD(, NetFilterState) net_filters;
>> +
>> +NetFilterState *qemu_new_net_filter(NetFilterInfo *info,
>> +                                    NetClientState *netdev,
>> +                                    const char *model,
>> +                                    const char *name)
>> +{
>> +    NetFilterState *nf;
>> +
>> +    assert(info->size >= sizeof(NetFilterState));
>> +
>> +    nf = g_malloc0(info->size);
>> +    nf->info = info;
>> +    nf->model = g_strdup(model);
>> +    nf->name = g_strdup(name);
>> +    nf->netdev = netdev;
>> +    QTAILQ_INSERT_TAIL(&net_filters, nf, next);
>> +    /* TODO: attach netfilter to netdev */
>> +
>> +    return nf;
>> +}
>> +
>> +static __attribute__((unused)) void qemu_cleanup_net_filter(NetFilterState
>> *nf)
>
> Maybe rather add this function in the patch you really need it? Then you could
> avoid the ugly attribute-unused here.

It will be removed in the next patch. I put cleanup here in order to pair with
the new function, and could be easy to review...

>
>> +{
>> +    /* TODO: remove netfilter from netdev */
>> +
>> +    QTAILQ_REMOVE(&net_filters, nf, next);
>> +
>> +    if (nf->info->cleanup) {
>> +        nf->info->cleanup(nf);
>> +    }
>> +
>> +    g_free(nf->name);
>> +    g_free(nf->model);
>> +    g_free(nf);
>> +}
>> +
>> +typedef int (NetFilterInit)(const NetFilterOptions *opts,
>> +                            const char *name,
>> +                            NetClientState *netdev, Error **errp);
>> +
>> +static
>> +NetFilterInit * const net_filter_init_fun[NET_FILTER_OPTIONS_KIND_MAX] = {
>> +};
>> +
>> +static int net_filter_init1(const NetFilter *netfilter, Error **errp)
>> +{
>> +    NetClientState *netdev = NULL;
>> +    NetClientState *ncs[MAX_QUEUE_NUM];
>> +    const char *name = netfilter->id;
>> +    const char *netdev_id = netfilter->netdev;
>> +    const NetFilterOptions *opts = netfilter->opts;
>> +    int queues;
>> +
>> +    if (!net_filter_init_fun[opts->kind]) {
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
>> +                   "a net filter type");
>> +        return -1;
>> +    }
>> +
>> +    queues = qemu_find_net_clients_except(netdev_id, ncs,
>> +                                          NET_CLIENT_OPTIONS_KIND_NIC,
>> +                                          MAX_QUEUE_NUM);
>> +    if (queues > 1) {
>> +        error_setg(errp, "multiqueues is not supported by now");
>> +        return -1;
>> +    } else if (queues < 1) {
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "netdev",
>> +                   "a network backend id");
>> +        return -1;
>> +    }
>> +
>> +    netdev = ncs[0];
>> +
>> +
>
> One empty line should be enough.

Got, thanks!

>
>> +    if (net_filter_init_fun[opts->kind](opts, name, netdev, errp) < 0) {
>> +        if (errp && !*errp) {
>> +            error_setg(errp, QERR_DEVICE_INIT_FAILED,
>> +                       NetFilterOptionsKind_lookup[opts->kind]);
>> +        }
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int net_init_filter(void *dummy, QemuOpts *opts, Error **errp)
>> +{
>> +    NetFilter *object = NULL;
>> +    Error *err = NULL;
>> +    int ret = -1;
>> +
>> +    {
>
> Why the extra curly brackets here? Looks somewhat strange, can you also
> do it without?

actually it is almost the same with net.c, but sure, can remove it in
the next version.

>
>> +        OptsVisitor *ov = opts_visitor_new(opts);
>> +
>> +        visit_type_NetFilter(opts_get_visitor(ov), &object, NULL, &err);
>> +        opts_visitor_cleanup(ov);
>> +    }
>> +
>> +    if (!err) {
>> +        ret = net_filter_init1(object, &err);
>> +    }
>> +
>> +
>> +    if (object) {
>> +        QapiDeallocVisitor *dv = qapi_dealloc_visitor_new();
>> +
>> +        visit_type_NetFilter(qapi_dealloc_get_visitor(dv), &object, NULL,
>> NULL);
>> +        qapi_dealloc_visitor_cleanup(dv);
>> +    }
>> +
>> +    error_propagate(errp, err);
>> +    return ret;
>> +}
>>
>>   int net_init_filters(void)
>>   {
>> +    QTAILQ_INIT(&net_filters);
>> +
>> +    if (qemu_opts_foreach(qemu_find_opts("netfilter"),
>> +                          net_init_filter, NULL, NULL)) {
>> +        return -1;
>> +    }
>> +
>>       return 0;
>>   }
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index a0a45f7..9a7c107 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -2537,6 +2537,36 @@
>>       'opts': 'NetClientOptions' } }
>>
>>   ##
>> +# @NetFilterOptions
>> +#
>> +# A discriminated record of network filters.
>> +#
>> +# Since 2.5
>> +#
>> +##
>> +{ 'union': 'NetFilterOptions',
>> +  'data': { } }
>> +
>> +##
>> +# @NetFilter
>> +#
>> +# Captures the packets of a network backend.
>> +#
>> +# @id: identifier for monitor commands.
>> +#
>> +# @netdev: the network backend it attached to.
>> +#
>> +# @opts: filter type specific properties
>> +#
>> +# Since 2.5
>> +##
>> +{ 'struct': 'NetFilter',
>> +  'data': {
>> +    'id':   'str',
>> +    'netdev': 'str',
>> +    'opts': 'NetFilterOptions' } }
>> +
>> +##
>>   # @InetSocketAddress
>>   #
>>   # Captures a socket address or address range in the Internet namespace.
>
> Apart from the nits, the patch looks ok to me.

Thank you.

>
>   Thomas
> .
>

-- 
Thanks,
Yang.