Messages in this thread | | | Date | Sat, 25 Mar 2006 23:52:12 +0530 | From | "Balbir Singh" <> | Subject | Re: [RFC][UPDATED PATCH 2.6.16] [Patch 9/9] Generic netlink interface for delay accounting |
| |
On 3/25/06, jamal <hadi@cyberus.ca> 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 linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |