lkml.org 
[lkml]   [2009]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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 <dbaryshkov@gmail.com> 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...
    >
    > > + 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.

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

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

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

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

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

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

    > > ...
    > >
    > > +struct proto ieee802154_dgram_prot = {
    >
    > I suspect this could/should be const.

    No. proto_register changes the passed protocol struct during
    registration.

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



    \
     
     \ /
      Last update: 2009-06-04 13:19    [W:0.034 / U:32.284 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site