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

[Thomas Huth]

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.

> +{
> +    /* 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.

> +    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?

> +        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.

 Thomas