On 3/25/06, jamal <[EMAIL PROTECTED]> wrote:
> On Sat, 2006-25-03 at 21:06 +0530, Balbir Singh wrote:
> > On Sat, Mar 25, 2006 at 07:52:13AM -0500, jamal wrote:
>
>
> I didnt pay attention to failure paths etc; i suppose your testing
> should catch those. Getting there, a couple more comments:
>
Yes, I have tried several negative test cases.
>
> > +enum {
> > + TASKSTATS_CMD_UNSPEC = 0, /* Reserved */
> > + TASKSTATS_CMD_GET, /* user->kernel request */
> > + TASKSTATS_CMD_NEW, /* kernel->user event */
>
> Should the comment read "kernel->user event/get-response"
>
Yes, good catch. I will update the comment.
>
> > +
> > +static int taskstats_send_stats(struct sk_buff *skb, struct genl_info
> > *info)
> > +{
>
>
> > +
> > + if (info->attrs[TASKSTATS_CMD_ATTR_PID]) {
> > + u32 pid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_PID]);
> > + rc = fill_pid((pid_t)pid, NULL, &stats);
> > + if (rc < 0)
> > + goto err;
> > +
> > + na = nla_nest_start(rep_skb, TASKSTATS_TYPE_AGGR_PID);
> > + NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_PID, pid);
> > + } else if (info->attrs[TASKSTATS_CMD_ATTR_TGID]) {
>
> in regards to the elseif above:
> Could you not have both PID and TGID passed? From my earlier
> understanding it seemed legit, no? if answer is yes, then you will have
> to do your sizes + reply TLVs at the end.
No, we cannot have both passed. If we pass both a PID and a TGID and
then the code returns just the stats for the PID.
>
> Also in regards to the nesting, isnt there a need for nla_nest_cancel in
> case of failures to add TLVs?
>
I thought about it, but when I looked at the code of genlmsg_cancel()
and nla_nest_cancel(). It seemed that genlmsg_cancel() should
suffice.
<snippet>
static inline int genlmsg_cancel(struct sk_buff *skb, void *hdr)
{
return nlmsg_cancel(skb, hdr - GENL_HDRLEN - NLMSG_HDRLEN);
}
static inline int nlmsg_cancel(struct sk_buff *skb, struct nlmsghdr *nlh)
{
skb_trim(skb, (unsigned char *) nlh - skb->data);
return -1;
}
static inline int nla_nest_cancel(struct sk_buff *skb, struct nlattr *start)
{
if (start)
skb_trim(skb, (unsigned char *) start - skb->data);
return -1;
}
</snippet>
genlmsg_cancel() seemed more generic, since it handles skb_trim from
the nlmsghdr down to skb->data, where as nla_test_cancel() does it
only from the start of the nested attributes to skb->data.
Is my understanding correct?
> cheers,
> jamal
>
Thanks,
Balbir
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
