[U-Boot] [PATCH RFC] sandbox: Add tap based networking

[Mike Frysinger <vapier@gentoo.org>]

On Tuesday 29 November 2011 09:09:03 Matthias Weisser wrote:
> This patch adds support for networking to sandbox architecture using tap. A
> tap device "tap0" has to be created e.g. using openvpn
> ....

this info should be in the changelog as it's useful

> As sandbox is build using the native compiler, which is in my case x86_64,
> ulong is 64 bit in size. This caused non-working IP stuff as IPaddr_t is
> typedefed to ulong. I changed this typedef to u32 which helped but is
> quite invasive to all network related stuff. This is the main reason why
> this patched is marked as RFC.

could you elaborate on "non-working IP stuff" ?

> --- a/arch/sandbox/cpu/cpu.c
> +++ b/arch/sandbox/cpu/cpu.c
>
> +#if defined(CONFIG_DRIVER_TAP)

i think Wolfgang NAK-ed the idea of using "DRIVER" in the config name.  so 
let's use something like CONFIG_NET_TAP.

> --- a/arch/sandbox/cpu/os.c
> +++ b/arch/sandbox/cpu/os.c
> 
> +#include <sys/ioctl.h>
> +#include <sys/socket.h>
> +#include <linux/if.h>
> +#include <linux/if_tun.h>

these should be behind the new CONFIG name, as should the func itself

> +int os_tap_open(char *dev)

const char *dev

> +	if (*dev)
> +		strncpy(ifr.ifr_name, dev, IFNAMSIZ);

let's just make it required

> --- /dev/null
> +++ b/drivers/net/tap.c
>
> +static int tap_send(struct eth_device *dev, void *packet, int length)
> ...
> +	os_write(priv->fd, (void *)packet, length);

packet is already void*, so no need to cast

> +static int tap_recv(struct eth_device *dev)
> +{
> +	struct tap_priv *priv = dev->priv;
> +	uchar buf[PKTSIZE];

there are already common network buffers for you to use: NetRxPackets[0]

> +	int length;
> +
> +	length = os_read(priv->fd, buf, PKTSIZE);

os_read returns ssize_t, not int

> +static int tap_set_hwaddr(struct eth_device *dev)
> +{
> +	/* Nothing to be done here */
> +	return 0;
> +}

isn't there an ioctl that lets you control this ?

> +int tap_initialize(bd_t *bd)
> ...
> +	struct eth_device *edev;

this should be named "dev" to stay consistent

> +	edev = (struct eth_device *)malloc(sizeof(struct eth_device));
> +	tap = (struct tap_priv *)malloc(sizeof(struct tap_priv));

no need for the casts

> +	if (!edev) {
> +		puts("tap: not enough malloc memory for eth_device\n");
> +		res = -ENOMEM;
> +		goto err1;
> +	}
> +
> +	if (!tap) {
> +		puts("tap: not enough malloc memory for tap_priv\n");
> +		res = -ENOMEM;
> +		goto err2;
> +	}

drop the puts(), and return 0, not -ENOMEM

> +	strncpy(tap->dev, "tap0", sizeof(tap->dev));

no reason we couldn't support more than one tap, is there ?  make the device 
name an argument to tap_initialize(), and then the board caller can do:
	tap_initialize("tap0");
	tap_initialize("tap1");
	tap_initialize("fooiefoofoo");

> +	tap->fd = os_tap_open(tap->dev);
> +	if (tap->fd < 0) {
> +		res = -EIO;
> +		goto err3;
> +	}

this should return -1

> +	sprintf(edev->name, "TAP");

use the same name as tap->dev.  in fact, i'd just chuck tap->dev and only use 
dev->name.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20111129/fee5b043/attachment.pgp>