On Thu, Dec 22, 2011 at 5:01 PM, Jesse Gross <je...@nicira.com> wrote:
> On Dec 21, 2011, at 8:28 PM, Pravin B Shelar wrote:
>> diff --git a/datapath/linux/compat/genetlink.inc
>> b/datapath/linux/compat/genetlink.inc
>> index bf96980..1301006 100644
>> --- a/datapath/linux/compat/genetlink.inc
>> +++ b/datapath/linux/compat/genetlink.inc
>> +static struct sk_buff *genlmsg_skb;
>> +
>> +int genl_exec_init(void)
>> +{
>> + int err;
>> +
>> + err = genl_register_family_with_ops(&genl_exec_family,
>> + genl_exec_ops,
>> + ARRAY_SIZE(genl_exec_ops));
>> +
>> + if (err)
>> + return err;
>> +
>> + genlmsg_skb = genlmsg_new( NLMSG_HDRLEN, GFP_KERNEL);
>
> The argument to genlmsg_new() is actually the size of the payload that you
> plan on inserting, which in this case is nothing so I think we can just pass
> in 0 here. There's also an extra space before NLMSG_HDRLEN.
>
ok.
>> +void genl_exec_exit(void)
>> +{
>> + genl_unregister_family(&genl_exec_family);
>> + mutex_lock(&genl_exec_lock);
>> + kfree_skb(genlmsg_skb);
>> + genlmsg_skb = NULL;
>> + mutex_unlock(&genl_exec_lock);
>
> I think it's safe to assume that nobody is using this anymore. This is the
> last step before unloading the module, so if there's any chance that somebody
> could still be calling into our code, we're in trouble.
>
ok.
>> +/* genl_lock() is not exported from older kernel.
>> + * Following function allows any function to be executed with
>> + * genl_mutex held. */
>> +
>> +int genl_exec(genl_exec_func_t func, void *data)
>> +{
>> + int ret;
>> + struct nlmsghdr *nlh;
>
> Is this ever used?
>
>> + mutex_lock(&genl_exec_lock);
>> + if (!genlmsg_skb) {
>> + ret = -ESHUTDOWN;
>> + goto out;
>> + }
>
> Similarly, I think this condition can never happen.
>
ok.
>> + skb_get(genlmsg_skb);
>> +
>> + nlh = genlmsg_put(genlmsg_skb, 0, 0, &genl_exec_family, NLM_F_REQUEST,
>> GENL_EXEC_RUN);
>
> Is it possible to do this once when the skb is allocated or does Netlink
> actually change the contents of the skb? If not, then I think we could avoid
> all the resetting at the end as well.
>
I wanted to do same. But netlink_rcv_skb() does skb_pull(). So some
sort of resetting is required in any case.
>> + genl_exec_function = func;
>> + genl_exec_data = data;
>> + init_completion(&done);
>> +
>> + ret = genlmsg_unicast(&init_net, genlmsg_skb, 0);
>> + if (!ret) {
>> + wait_for_completion(&done);
>> + ret = genl_exec_function_ret;
>> + } else
>> + printk(KERN_ERR "[%s] Msg send error %d\n",__func__, ret);
>
> This is a somewhat strange form for an error message. Can't we just use the
> normal pr_err("genl_exec send error %d", ret") or even just BUG_ON(ret) since
> we're claiming that this can never happen?
>
ok.
>> + genlmsg_skb->data = genlmsg_skb->head;
>
> Is this actually necessary? genlmsg_put() eventually calls skb_put(), which
> advances the tail pointer but not data (this is back to the question of
> whether anybody else is modifying the skb).
>
netlink_rcv_skb does sbk_pull() which advances data.
>> + skb_reset_tail_pointer(genlmsg_skb);
>
> This doesn't decrement skb->len. I guess nobody checks it but it doesn't
> seem good to let it grow without bound.
same skb_pull() decrements skb->len.
>
>> diff --git a/datapath/linux/compat/include/linux/skbuff.h
>> b/datapath/linux/compat/include/linux/skbuff.h
>> index 96d8012..01e524e 100644
>> --- a/datapath/linux/compat/include/linux/skbuff.h
>> +++ b/datapath/linux/compat/include/linux/skbuff.h
>> @@ -34,6 +34,12 @@ static inline void skb_copy_to_linear_data_offset(struct
>> sk_buff *skb,
>>
>> #endif /* !HAVE_SKB_COPY_FROM_LINEAR_DATA_OFFSET */
>>
>> +#ifndef HAVE_SKB_RESET_TAIL_POINTER
>> +static inline void skb_reset_tail_pointer(struct sk_buff *skb)
>> +{
>> + skb->tail = skb->data;
>> +}
>> +#endif
>
> I'm assuming that this is backported by some kernel?
yes, RHEL 5.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
