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

[Yang Hongyang]

On 08/10/2015 05:18 PM, Jason Wang wrote:
> On 08/07/2015 10:46 AM, Yang Hongyang wrote:
> > QTAILQ_ENTRY global_list but used by filter layer, so that we can
> > manage all filters together.
> > QTAILQ_ENTRY next used by netdev, filter belongs to the specific netdev is
> > in this queue.
> > This is mostly the same with init/cleanup of netdev object.
> >
> > Signed-off-by: Yang Hongyang <address@hidden>
> > ---
> > v6: add multiqueue support (net_filter_init1)
> > v5: remove model from NetFilterState
> >      add a sent_cb param to receive_iov API
> > ---
> >   include/net/filter.h    |  42 +++++++++++++++
> >   include/net/net.h       |   1 +
> >   include/qemu/typedefs.h |   1 +
> >   net/filter.c            | 141 ++++++++++++++++++++++++++++++++++++++++++++++++
> >   net/net.c               |   1 +
> >   qapi-schema.json        |  37 +++++++++++++
> >   6 files changed, 223 insertions(+)
> >
> > diff --git a/include/net/filter.h b/include/net/filter.h
> > index 4242ded..7a858d8 100644
> > --- a/include/net/filter.h
> [...]
> > +static
> > +NetFilterInit * const net_filter_init_fun[NET_FILTER_OPTIONS_KIND_MAX] = {
> > +};
> > +
> > +static int net_filter_init1(const NetFilter *netfilter, Error **errp)
> > +{
> > +    NetClientState *ncs[MAX_QUEUE_NUM];
> > +    const char *name = netfilter->id;
> > +    const char *netdev_id = netfilter->netdev;
> > +    const char *chain_str = NULL;
> > +    const NetFilterOptions *opts = netfilter->opts;
> > +    int chain, queues, i;
> > +
> > +    if (!net_filter_init_fun[opts->kind]) {
> > +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
> > +                   "a net filter type");
> > +        return -1;
> > +    }
> > +
> > +    if (netfilter->has_chain) {
> > +        chain_str = netfilter->chain;
> > +        if (!strcmp(chain_str, "in")) {
> > +            chain = NET_FILTER_IN;
> > +        } else if (!strcmp(chain_str, "out")) {
> > +            chain = NET_FILTER_OUT;
> > +        } else if (!strcmp(chain_str, "all")) {
> > +            chain = NET_FILTER_ALL;
> > +        } else {
> > +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "chain",
> > +                       "netfilter chain (in/out/all)");
> > +            return -1;
> > +        }
> > +    } else {
> > +        /* default */
> > +        chain = NET_FILTER_ALL;
> > +    }
> > +
> > +    queues = qemu_find_net_clients_except(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 -1;
> > +    }
>
> Let's fail when vhost is used here.

I think you mean vhost-user here?

> > +
> > +    for (i = 0; i < queues; i++) {
> > +        if (net_filter_init_fun[opts->kind](opts, name,
> > +                                            chain, ncs[i], 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;
> > +    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);

should print out the error here instead of propagate it,otherwise the error msg
is lost.

> > +    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;
> >   }
>
> Then errors will be lost here?

Yes, good catch, thank you !

> .

--
Thanks,
Yang.