On Wed, Dec 14, 2011 at 6:09 PM, Pravin B Shelar <pshe...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
