[ovs-dev] [PATCH 1/3] datapath: Add genl_exec().

Jesse Gross jesse at nicira.com
Fri Dec 16 02:03:59 UTC 2011


On Wed, Dec 14, 2011 at 6:09 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
> diff --git a/datapath/linux/compat/genetlink.inc b/datapath/linux/compat/genetlink.inc
> index bf96980..43c3227 100644
> --- a/datapath/linux/compat/genetlink.inc
> +++ b/datapath/linux/compat/genetlink.inc
> +static int genl_exec_cmd(struct sk_buff *dummy, struct genl_info *dummy2)
> +{
> +       if (genl_exec_function)
> +               genl_exec_function_ret = genl_exec_function(genl_exec_data);

Can genl_exec_function ever be NULL (and is there a point to NULLing
it out after use)?

> +enum genl_exec_attr {
> +       GENL_EXEC_UNSPEC,
> +       GENL_EXEC_RUN,
> +       GENL_EXEC_MAX,
> +};

This MAX is off by one, it should be the index of the highest element,
not the one after that.

> +static struct genl_family genl_exec_family = {
> +       .id = GENL_ID_GENERATE,
> +       .hdrsize = 0,
> +       .name = "GENL EXEC FAMILY",

The name should be prefixed with ovs like "ovs_genl_exec".

> +       .version = 1,
> +       .maxattr = GENL_EXEC_MAX,

This should be the highest attribute, of which there are none, but
this is the highest command.

> +int genl_exec(genl_exec_func_t func, void *data)
> +{
> +       struct sk_buff *skb;
> +       int ret;
> +
> +       skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +       if (!skb)
> +               return -ENOMEM;

I'm worried about the consequences of this failing when it is used to
cleanup namespaces.

> +       genlmsg_put(skb, 0, 0, &genl_exec_family, NLM_F_REQUEST, GENL_EXEC_RUN);
> +
> +       mutex_lock(&genl_exec_lock);
> +       genl_exec_function = func;
> +       genl_exec_data = data;
> +       init_completion(&done);
> +       genlmsg_unicast(&init_net, skb, 0);

genlmsg_unicast() can return an error if the socket buffer is full on
kernels that actually add it to a queue but this will silently ignore
it.



More information about the dev mailing list