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

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

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

...

+#define MAC_CB(skb)	((struct ieee802154_mac_cb *)(skb)->cb)

At present this macro can be passed a variable of any type at all.

It would be better to implement this as a (probably inlined) C
function, so the compiler can check that it was indeed passed a `struct
sk_buff *' (or whatever type it's supposed to be).

And regular C functions are typically in lower case..


I have a feeling that this unnecessary macro pattern is used in other
places in networking, and there's an argument that new code should copy
the old code.  It's not a terribly good argument, IMO - the defeating
of type-safety does rather suck.

These didn't need to be implemented as macros either.

Nor did this.

...

+	int i; \
+	pr_debug("file %s: function: %s: data: len %d:\n", __FILE__, __func__, len); \
+	for (i = 0; i < len; i++) {\
+		pr_debug("%02x: %02x\n", i, (data)[i]); \
+	} \
+}

Could perhaps use lib/hexdump.c

Will do weird things if passed a pointer whcih has type other than char*.

You must use very wide xterms :(

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

?

+	sk->sk_family = PF_IEEE802154;
+
+	/* Checksums on by default */
+	sock_set_flag(sk, SOCK_ZAPPED);
+
+	if (sk->sk_prot->hash)
+		sk->sk_prot->hash(sk);
+
+	if (sk->sk_prot->init) {
+		rc = sk->sk_prot->init(sk);
+		if (rc)
+			sk_common_release(sk);
+	}
+out:
+	return rc;
+}
+

...

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

+{
+	dev_remove_pack(&ieee802154_packet_type);
+	sock_unregister(PF_IEEE802154);
+	proto_unregister(&ieee802154_dgram_prot);
+	proto_unregister(&ieee802154_raw_prot);
+}
+
+module_init(af_ieee802154_init);
+module_exit(af_ieee802154_remove);

...

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

+}

...

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

+	struct dgram_sock *ro = dgram_sk(sk);
+	int err = 0;
+	struct net_device *dev;
+
+	ro->bound = 0;
+
+	if (len < sizeof(*addr))
+		return -EINVAL;
+
+	if (addr->family != AF_IEEE802154)
+		return -EINVAL;
+
+	lock_sock(sk);
+
+	dev = ieee802154_get_dev(sock_net(sk), &addr->addr);
+	if (!dev) {
+		err = -ENODEV;
+		goto out;
+	}
+
+	if (dev->type != ARPHRD_IEEE802154) {
+		err = -ENODEV;
+		goto out_put;
+	}
+
+	memcpy(&ro->src_addr, &addr->addr, sizeof(struct ieee802154_addr));
+
+	ro->bound = 1;
+out_put:
+	dev_put(dev);
+out:
+	release_sock(sk);
+
+	return err;
+}
+

...

+static int dgram_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
+		       size_t size)
+{
+	struct net_device *dev;
+	unsigned mtu;
+	struct sk_buff *skb;
+	struct dgram_sock *ro = dgram_sk(sk);
+	int err;
+
+	if (msg->msg_flags & MSG_OOB) {
+		pr_debug("msg->msg_flags = 0x%x\n", msg->msg_flags);
+		return -EOPNOTSUPP;
+	}
+
+	if (!ro->bound)
+		dev = dev_getfirstbyhwtype(sock_net(sk), ARPHRD_IEEE802154);
+	else
+		dev = ieee802154_get_dev(sock_net(sk), &ro->src_addr);
+
+	if (!dev) {
+		pr_debug("no dev\n");
+		return -ENXIO;
+	}
+	mtu = dev->mtu;
+	pr_debug("name = %s, mtu = %u\n", dev->name, mtu);
+
+	skb = sock_alloc_send_skb(sk, LL_ALLOCATED_SPACE(dev) + size, msg->msg_flags & MSG_DONTWAIT,
+				  &err);
+	if (!skb) {
+		dev_put(dev);
+		return err;
+	}
+	skb_reserve(skb, LL_RESERVED_SPACE(dev));
+
+	skb_reset_network_header(skb);
+
+	MAC_CB(skb)->flags = IEEE802154_FC_TYPE_DATA | MAC_CB_FLAG_ACKREQ;
+	MAC_CB(skb)->seq = IEEE802154_MLME_OPS(dev)->get_dsn(dev);
+	err = dev_hard_header(skb, dev, ETH_P_IEEE802154, &ro->dst_addr, ro->bound ? &ro->src_addr : NULL, size);
+	if (err < 0) {
+		kfree_skb(skb);
+		dev_put(dev);
+		return err;
+	}
+
+	skb_reset_mac_header(skb);
+
+	err = memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size);
+	if (err < 0) {
+		kfree_skb(skb);
+		dev_put(dev);
+		return err;
+	}

It would be better to convert this function (and any similar
occurences) to the `goto foo;' unwinding approach.  The
multiple-return-statements-per-C-function is not a favoured approach. 
It leads to code duplication and it leads to bugs.

See, a bug.

+	}
+
+	skb->dev = dev;
+	skb->sk  = sk;
+	skb->protocol = htons(ETH_P_IEEE802154);
+
+	err = dev_queue_xmit(skb);
+
+	dev_put(dev);
+
+	if (err)
+		return err;
+
+	return size;
+}
+

...

+struct proto ieee802154_dgram_prot = {

I suspect this could/should be const.

+	.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,
+};
+

...

+struct proto ieee802154_raw_prot = {

const?

...