On Wed, 2008-11-26 at 15:07 -0800, Inaky Perez-Gonzalez wrote:
quoted text
> When user space wants to open a handle to a WiMAX device, it needs
> information that is provided as a response to an "open" generic
> netlink message.

Why is this called "open" when it's not actually any opening that would
be bracketed with "close"? It seems more like "get info" to me?

quoted text
> +	result =3D -ENOMEM;
> +	reply_skb =3D nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (reply_skb =3D=3D NULL)
> +		goto error_new;
> +	data =3D genlmsg_put_reply(reply_skb, genl_info,
> +				 &wimax_dev->gnl_family,
> +				 0, WIMAX_GNL_RP_IFINFO);
> +	if (data =3D=3D NULL)
> +		goto error_put_reply;
> +
> +	nla_groups =3D nla_nest_start(reply_skb, WIMAX_GNL_IFINFO_MC_GROUPS);
> +	if (nla_groups =3D=3D NULL)
> +		goto error_groups_start;
> +
> +	list_for_each_entry(pipe_itr, &wimax_dev->pipe_list,
> +			    list_node) {
> +		nla_group =3D nla_nest_start(reply_skb,
> +					   WIMAX_GNL_IFINFO_MC_GROUP);
> +		if (nla_group =3D=3D NULL)
> +			goto error_group_start;
> +
> +		nla_put_u16(reply_skb, WIMAX_GNL_IFINFO_MC_ID,
> +			    pipe_itr->mcg.id);
> +		nla_put_string(reply_skb, WIMAX_GNL_IFINFO_MC_NAME,
> +			       pipe_itr->mcg.name);

All this is discoverable via the genl controller and a pipe naming
scheme, so I don't think you need this "open" command at all.

quoted text
> +		nla_nest_end(reply_skb, nla_group);
> +	}
> +	nla_nest_end(reply_skb, nla_groups);
> +	genlmsg_end(reply_skb, data);
> +
> +	result =3D genlmsg_reply(reply_skb, genl_info);
> +	if (result < 0)
> +		goto error_reply;
> +	return 0;
> +
> +error_group_start:
> +error_groups_start:
> +error_reply:
> +error_put_reply:
> +	nlmsg_free(reply_skb);
> +error_new:
> +	return result;
> +}
> +
> +
> +static
> +int wimax_gnl_doit_open(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int result;
> +	struct wimax_dev *wimax_dev;
> +
> +	d_fnstart(3, NULL, "(skb %p info %p)\n", skb, info);
> +	result =3D -EPERM;
> +	if (security_netlink_recv(skb, CAP_NET_ADMIN))
> +		goto error_perm;

Ehm, you can do that check by adding the flag below...

quoted text
> +	result =3D -ENODEV;
> +	wimax_dev =3D wimax_dev_get_by_genl_info(info);
> +	if (wimax_dev =3D=3D NULL)
> +		goto error_no_wimax_dev;
> +	mutex_lock(&wimax_dev->mutex);
> +	result =3D wimax_dev_is_ready(wimax_dev);
> +	if (result < 0)
> +		goto error_not_ready;
> +	result =3D __wimax_gnl_open_reply(wimax_dev, info);
> +error_not_ready:
> +	mutex_unlock(&wimax_dev->mutex);
> +	dev_put(wimax_dev->net_dev);
> +error_no_wimax_dev:
> +error_perm:
> +	d_fnend(3, NULL, "(skb %p info %p) =3D %d\n", skb, info, result);
> +	return result;
> +}
> +
> +
> +struct genl_ops wimax_gnl_open =3D {
> +	.cmd =3D WIMAX_GNL_OP_OPEN,
> +	.flags =3D 0,

GENL_ADMIN_PERM and remove the checks above?