Hi Balbir,
Looking good.
This is a quick scan, so i didnt look at little details.
Some comments embedded.
On Wed, 2006-22-03 at 13:19 +0530, Balbir Singh wrote:
>
> diff -puN /dev/null include/linux/taskstats.h
> + * The struct is versioned. Newer versions should only add fields to
> + * the bottom of the struct to maintain backward compatibility.
> + *
> + * To create the next version, bump up the taskstats_version variable
> + * and delineate the start of newly added fields with a comment indicating
> + * the version number.
> + */
> +
> +enum {
> + TASKSTATS_MSG_UNICAST, /* send data only to requester */
> + TASKSTATS_MSG_MULTICAST, /* send data to a group */
> +};
> +
Above will never be used outside of the kernel.
Should it go under the ifdef kernel below?
> +#ifdef __KERNEL__
> +
Note: some people will argue that you should probably have
two header files. One for all kernel things and includes another
which contains all the stuff above ifdef __KERNEL__
> +
> +#endif /* __KERNEL__ */
> +#endif /* _LINUX_TASKSTATS_H */
> diff -puN kernel/Makefile~delayacct-genetlink kernel/Makefile
> --- linux-2.6.16/kernel/Makefile~delayacct-genetlink 2006-03-22
> 11:56:03.000000000 +0530
> +++ linux-2.6.16-balbir/kernel/Makefile 2006-03-22 11:56:03.000000000
> +0530
> +
> +const int taskstats_version = TASKSTATS_VERSION;
> +static DEFINE_PER_CPU(__u32, taskstats_seqnum) = { 0 };
> +static int family_registered = 0;
> +
> +static struct genl_family family = {
> + .id = GENL_ID_GENERATE,
> + .name = TASKSTATS_GENL_NAME,
> + .version = TASKSTATS_GENL_VERSION,
> + .hdrsize = 0,
Do you need to specify hdrsize of 0?
> +static int prepare_reply(struct genl_info *info, u8 cmd, struct sk_buff
> **skbp,
> + void **replyp)
> +{
> + struct sk_buff *skb;
> + void *reply;
> +
> + skb = nlmsg_new(NLMSG_GOODSIZE);
Ok, getting a size of NLMSG_GOODSIZE is not a good idea.
The max size youll ever get it seems to me is 2*32-bit-data TLVs +
sizeof struct stats. Why dont you allocate that size?
> + if (!skb)
> + return -ENOMEM;
> +
> + if (!info) {
> + int seq = get_cpu_var(taskstats_seqnum)++;
> + put_cpu_var(taskstats_seqnum);
> +
> + reply = genlmsg_put(skb, 0, seq,
> + family.id, 0, NLM_F_REQUEST,
> + cmd, family.version);
Double check if you need NLM_F_REQUEST
> + } else
> + reply = genlmsg_put(skb, info->snd_pid, info->snd_seq,
> + family.id, 0, info->nlhdr->nlmsg_flags,
> + info->genlhdr->cmd, family.version);
A Response to a GET is a NEW. So i dont think info->genlhdr->cmd is the
right thing?
> +static int taskstats_send_stats(struct sk_buff *skb, struct genl_info *info)
> +{
> + int rc;
> + struct sk_buff *rep_skb;
> + struct taskstats stats;
> + void *reply;
> +
> + memset(&stats, 0, sizeof(stats));
> + rc = prepare_reply(info, info->genlhdr->cmd, &rep_skb, &reply);
Same comment as before: a response to a GET is a NEW; so
info->genlhdr->cmd doesnt seem right.
> + if (rc < 0)
> + return rc;
> +
> + if (info->attrs[TASKSTATS_CMD_ATTR_PID]) {
> + u32 pid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_PID]);
> + NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_PID, pid);
> + rc = fill_pid((pid_t)pid, NULL, &stats);
> + if (rc < 0)
> + return rc;
> + }
> +
> + if (info->attrs[TASKSTATS_CMD_ATTR_TGID]) {
> + u32 tgid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_TGID]);
> + NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_TGID, tgid);
> + rc = fill_tgid((pid_t)tgid, NULL, &stats);
> + if (rc < 0)
> + return rc;
> + }
> +
Should there be at least either a pid or tgid? If yes, you need to
validate here...
> + NLA_PUT_TYPE(rep_skb, struct taskstats, TASKSTATS_TYPE_STATS, stats);
> + return send_reply(rep_skb, info->snd_pid, TASKSTATS_MSG_UNICAST);
> +
> +nla_put_failure:
> + return genlmsg_cancel(rep_skb, reply);
> +
As a general comment double check your logic for errors; if you already
have stashed something in the skb, you need to remove it etc.
> +}
> +
> +
> +/* Send pid data out on exit */
> +void taskstats_exit_pid(struct task_struct *tsk)
> +{
> + int rc;
> + struct sk_buff *rep_skb;
> + void *reply;
> + struct taskstats stats;
> +
> + /*
> + * tasks can start to exit very early. Ensure that the family
> + * is registered before notifications are sent out
> + */
> + if (!family_registered)
> + return;
> +
> + memset(&stats, 0, sizeof(stats));
> + rc = prepare_reply(NULL, TASKSTATS_CMD_NEW, &rep_skb, &reply);
> + if (rc < 0)
> + return;
> +
> + NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_PID, (u32)tsk->pid);
> + rc = fill_pid(tsk->pid, tsk, &stats);
> + if (rc < 0)
> + return;
> +
> + NLA_PUT_TYPE(rep_skb, struct taskstats, TASKSTATS_TYPE_STATS, stats);
> + rc = send_reply(rep_skb, 0, TASKSTATS_MSG_MULTICAST);
> +
> + if (rc || thread_group_empty(tsk))
> + return;
> +
> + /* Send tgid data too */
> + rc = prepare_reply(NULL, TASKSTATS_CMD_NEW, &rep_skb, &reply);
> + if (rc < 0)
> + return;
> +
A single message with PID+TGID sounds reasonable. Why two messages with
two stats? all you will need to do is get rid of the prepare_reply()
above and NLA_PUT_U32() below (just like you do in a response to a GET.
> + NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_TGID, (u32)tsk->tgid);
> + rc = fill_tgid(tsk->tgid, tsk, &stats);
> + if (rc < 0)
> + return;
> +
> + NLA_PUT_TYPE(rep_skb, struct taskstats, TASKSTATS_TYPE_STATS, stats);
> + send_reply(rep_skb, 0, TASKSTATS_MSG_MULTICAST);
> +
> +nla_put_failure:
> + genlmsg_cancel(rep_skb, reply);
> +}
Other than the above comments - I believe you have it right.
cheers,
jamal
-
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
