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

[Yang Hongyang]

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" <address@hidden> wrote:
> > This is mostly the same with init/cleanup of netdev object.
> >
> > Signed-off-by: Yang Hongyang <address@hidden>
> > ---
> >   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.