linux-kernel - Re: [PATCH 4/6] net: add NL802154 interface for configuration of 802.15.4 devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1244124749.22576.71.camel@johannes.local>
Date:	Thu, 04 Jun 2009 16:12:29 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>
Cc:	David Miller <davem@...emloft.net>, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, linux-wireless@...r.kernel.org,
	sfr@...b.auug.org.au, slapin@...fans.org
Subject: Re: [PATCH 4/6] net: add NL802154 interface for configuration of
 802.15.4 devices

On Wed, 2009-06-03 at 16:27 +0400, Dmitry Eremin-Solenikov wrote:

> > > +/* commands */
> > > +/* REQ should be responded with CONF
> > > + * and INDIC with RESP
> > > + */
> > > +enum {
> > 
> > kernel-doc explaining the commands would be immensely helpful.
> 
> What explanations whould you like to see? These commands are described
> in the IEEE 802.15.4 standard.

Well, for example which arguments they require...

> > > +#ifdef __KERNEL__
> > > +struct net_device;
> > > +
> > > +int ieee802154_nl_assoc_indic(struct net_device *dev, struct ieee802154_addr *addr, u8 cap);
> > > +int ieee802154_nl_assoc_confirm(struct net_device *dev, u16 short_addr, u8 status);
> > > +int ieee802154_nl_disassoc_indic(struct net_device *dev, struct ieee802154_addr *addr, u8 reason);
> > > +int ieee802154_nl_disassoc_confirm(struct net_device *dev, u8 status);
> > > +int ieee802154_nl_scan_confirm(struct net_device *dev, u8 status, u8 scan_type, u32 unscanned,
> > > +		u8 *edl/*, struct list_head *pan_desc_list */);
> > > +int ieee802154_nl_beacon_indic(struct net_device *dev, u16 panid, u16 coord_addr); /* TODO */
> > > +#endif
> > 
> > Why not just use two header files, one in net/ and one in linux/?
> 
> What would you suggest to put into the linux/ header and what in the
> net/ one?

userspace vs. in-kernel separation

> > > +static int ieee802154_nl_put_failure(struct sk_buff *msg)
> > > +{
> > > +	void *hdr = genlmsg_data(NLMSG_DATA(msg->data)); /* XXX: nlh is right at the start of msg */
> > > +	genlmsg_cancel(msg, hdr);
> > > +	nlmsg_free(msg);
> > > +	return -ENOBUFS;
> > > +}
> > 
> > This seems weird.
> 
> Why?

Because it's strange to cancel then free?

> > > +static int ieee802154_associate_req(struct sk_buff *skb, struct genl_info *info)
> > > +{
> > > +	struct net_device *dev;
> > > +	struct ieee802154_addr addr;
> > > +	int ret = -EINVAL;
> > > +
> > > +	if (!info->attrs[IEEE802154_ATTR_CHANNEL]
> > > +	 || !info->attrs[IEEE802154_ATTR_COORD_PAN_ID]
> > > +	 || (!info->attrs[IEEE802154_ATTR_COORD_HW_ADDR] && !info->attrs[IEEE802154_ATTR_COORD_SHORT_ADDR])
> > > +	 || !info->attrs[IEEE802154_ATTR_CAPABILITY])
> > > +		return -EINVAL;
> > 
> > That's some odd coding style.
> 
> Could you please elaborate this? What seems odd to you?

if (X
    && Y)

vs

if (X &&
    Y)

where the latter is used for all other sources files in the kernel. Try
applying that to _all_ your patches while you rework the sources to fit
into 80 columns.

johannes

Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)