On 3/25/06, jamal [off-list ref] wrote:

On Sat, 2006-25-03 at 21:06 +0530, Balbir Singh wrote:

quoted

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.

quoted

+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.

quoted

+
+static int taskstats_send_stats(struct sk_buff *skb, struct genl_info *info)
+{

quoted

+
+     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