Re: [PATCH 03/10] net: add IEEE 802.15.4 socket family implementation

On Wed, Jun 03, 2009 at 05:32:14PM -0700, Andrew Morton wrote:

On Mon,  1 Jun 2009 18:54:44 +0400
Dmitry Eremin-Solenikov [off-list ref] wrote:

quoted

Add support for communication over IEEE 802.15.4 networks. This implementation
is neither certified nor complete, but aims to that goal. This commit contains
only the socket interface for communication over IEEE 802.15.4 networks.
One can either send RAW datagrams or use SOCK_DGRAM to encapsulate data
inside normal IEEE 802.15.4 packets.

Configuration interface, drivers and software MAC 802.15.4 implementation will
follow.

Initial implementation was done by Maxim Gorbachyov, Maxim Osipov and Pavel
Smolensky as a research project at Siemens AG. Later the stack was heavily
reworked to better suit the linux networking model, and is now maitained
as an open project partially sponsored by Siemens.

Some drive-by nitpicking, and I saw a bug...

quoted

+	switch (addr->addr_type) {
+	case IEEE802154_ADDR_LONG:
+		rtnl_lock();
+		dev = dev_getbyhwaddr(net, ARPHRD_IEEE802154, addr->hwaddr);
+		if (dev)
+			dev_hold(dev);
+		rtnl_unlock();
+		break;
+	case IEEE802154_ADDR_SHORT:
+		if (addr->pan_id != 0xffff && addr->short_addr != IEEE802154_ADDR_UNDEF && addr->short_addr != 0xffff) {
+			struct net_device *tmp;
+
+			rtnl_lock();
+
+			for_each_netdev(net, tmp) {
+				if (tmp->type == ARPHRD_IEEE802154) {
+					if (IEEE802154_MLME_OPS(tmp)->get_pan_id(tmp) == addr->pan_id
+					  && IEEE802154_MLME_OPS(tmp)->get_short_addr(tmp) == addr->short_addr) {

You must use very wide xterms :(

~120 chars in width :)  We prefer to have a single code line split between
several screen lines, rather than split it manually in some weird places
just to justify width of 80 chars.

quoted

+						dev = tmp;
+						dev_hold(dev);
+						break;
+					}
+				}
+			}
+
+			rtnl_unlock();
+		}
+		break;
+	default:
+		pr_warning("Unsupported ieee802154 address type: %d\n", addr->addr_type);
+		break;
+	}
+
+	return dev;
+}
+

...

+static int ieee802154_create(struct net *net, struct socket *sock, int protocol)
+{
+	struct sock *sk;
+	int rc;
+	struct proto *proto;
+	const struct proto_ops *ops;
+
+	/* FIXME: init_net */
+	if (net != &init_net)
+		return -EAFNOSUPPORT;

yeah, I was going to ask about that.  What's the problem here?

The FIXME was dedicated to the fact that I didn't understand what is
this. The code fragment is c&p from lots of examples of similar code
(check can, appletalk, etc. for example)

quoted

+	switch (sock->type) {
+	case SOCK_RAW:
+		proto = &ieee802154_raw_prot;
+		ops = &ieee802154_raw_ops;
+		break;
+	case SOCK_DGRAM:
+		proto = &ieee802154_dgram_prot;
+		ops = &ieee802154_dgram_ops;
+		break;
+	default:
+		rc = -ESOCKTNOSUPPORT;
+		goto out;
+	}
+
+	rc = -ENOMEM;
+	sk = sk_alloc(net, PF_IEEE802154, GFP_KERNEL, proto);
+	if (!sk)
+		goto out;
+	rc = 0;
+
+	sock->ops = ops;
+
+	sock_init_data(sock, sk);
+	/* FIXME: sk->sk_destruct */

?

Oh... I nearly forgot about this. When writing this code, I examined
several existing address families. Usually (but not always) the sk_destruct
callback will purge sk_receive_queue and sk_write_queue. However I
didn't understand why and in which case should these queues (or others)
be purged. Could please one clarify this?

quoted

...

+static void af_ieee802154_remove(void)

Could be __exit, althugh __exit doesn't do much (it used to be
implemented on UML and might still be).

Added. BTW: I thought, that __exit functions aren't added to (or are
stripped from) the final vmlinux image. Am I wrong?

quoted

+static inline struct dgram_sock *dgram_sk(const struct sock *sk)
+{
+	return (struct dgram_sock *)sk;

Better to use container_of() - it's clearer and doesn't assume that
dgram_sock.sk is the first member.

Fixed.

quoted

+}

...

+static int dgram_bind(struct sock *sk, struct sockaddr *uaddr, int len)
+{
+	struct sockaddr_ieee802154 *addr = (struct sockaddr_ieee802154 *)uaddr;

sigh, more casting.  Often these things can be done in ways which are
nicer to the C type system.

Unfortunately sockaddr things can't be done in a more clean way IMO.

quoted

+	struct dgram_sock *ro = dgram_sk(sk);
+	int err = 0;
+	struct net_device *dev;
+
+	ro->bound = 0;
+

quoted

...

+struct proto ieee802154_dgram_prot = {

I suspect this could/should be const.

No. proto_register changes the passed protocol struct during
registration.

quoted

+	.name		= "IEEE-802.15.4-MAC",
+	.owner		= THIS_MODULE,
+	.obj_size	= sizeof(struct dgram_sock),
+	.init		= dgram_init,
+	.close		= dgram_close,
+	.bind		= dgram_bind,
+	.sendmsg	= dgram_sendmsg,
+	.recvmsg	= dgram_recvmsg,
+	.hash		= dgram_hash,
+	.unhash		= dgram_unhash,
+	.connect	= dgram_connect,
+	.disconnect	= dgram_disconnect,
+	.ioctl		= dgram_ioctl,
+};
+

-- 
With best wishes
Dmitry