On Wednesday, July 29, 2015 12:51:46 PM,
"Yang Hongyang" <yan...@cn.fujitsu.com> wrote:
>
> This is mostly the same with init/cleanup of netdev object.
>
> Signed-off-by: Yang Hongyang <yan...@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.
> +{
> + /* 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
