[gPXE-devel] [PATCH] 802.1Q support for gpxe

[Stefan Hajnoczi]

+static const char * vid_ntoa ( const void *net_addr ) {
+	static char buf[2]; /* "00" */
+        const uint8_t eth_addr = * (uint8_t*)net_addr;
+
+        sprintf ( buf, "%02x", eth_addr);
+        return buf;
+}
The array should be buf[3] so there is space for the NUL terminator.

How about something like the following to eliminate memory management
and simplify things (I have not tried compiling this code):

struct vlan_device {
	struct net_device* target;	/* the underlying raw device */
	struct net_device* netdev;	/* the vlan device */
	struct list_head list;		/* vlan devices */

	uint16_t vlanid;
	uint8_t prio;

	struct ll_protocol ll_protocol;
};

static LIST_HEAD ( ieee8021q_devices );

struct net_device * ieee8021q_create ( struct net_device* target,
uint16_t vid, uint8_t prio ) {
	struct vlan_device *vdev;
	struct net_device* netdev;

	netdev = alloc_netdev ( sizeof ( *vdev ) );
	if ( !netdev )
		return NULL;

	netdev_init ( netdev, &ieee8021q_operations );
	vdev = netdev->priv;
	vdev->target = target;

	memcpy ( &vdev->ll_protocol, target->ll_protocol, sizeof (
*target->ll_protocol ) );
	vdev->ll_protocol.push = ieee8021q_push;
	netdev->ll_protocol = &vdev->ll_protocol;

	netdev->ll_broadcast = target->ll_broadcast;
	netdev->max_pkt_len = target->max_pkt_len;
	memcpy(netdev->hw_addr,target->hw_addr,sizeof(netdev->hw_addr));
	netdev->state = target->state;

	snprintf ( netdev->name, sizeof ( netdev->name ), "%s.%d", target->name, vid);

	/* Mark as link up; we don't yet handle link state */
	netdev_link_up ( netdev );

	/* Register network device */
	if ( register_netdev ( netdev ) != 0 )
		goto err_register_netdev;

	/* write-back ll_addr init */
	memcpy(netdev->ll_addr,target->ll_addr,sizeof(netdev->ll_addr));

	/* add netdev to list */
	vdev->netdev = netdev;
	list_add(&vdev->list, &ieee8021q_devices);

	/* set target device */
	vdev->target = netdev_get(target);
	vdev->vlanid = vid;
	vdev->prio = prio;

	DBG("Added device %s.\n", netdev->name);

	return netdev;

 err_register_netdev:
	netdev_nullify ( netdev );
	netdev_put ( netdev );
	return NULL;
}

int ieee8021q_remove ( struct net_device* netdev) {
	struct vlan_device* vdev = netdev->priv;

	if (netdev->op->open != ieee8021q_open) {
		DBG("Sorry, but this does not look like a ieee8021q device.\n"
				"So I won't remove it!\n");
		return -EINVAL;
	}

	unregister_netdev ( netdev );
	netdev_put ( vdev->target );
	netdev_nullify ( netdev );
	netdev_put ( netdev );
	return 0;
}

Some minor changes also added above were using NULL instead of 0 for a pointer
value to follow gPXE coding style.  I also suggest adding error code to
ieee8021q_remove() so that the user can get error feedback if they tried to
remove a non-802.1q device.

As for the zero-copy transmit approach, I think it could be done with
a iob_orphan()
function which calls list_del() on the io_buffer.  It might require
always returning
success from the transmit function though.

Another side effect is that transmit statistics will be missing for
the vlan netdevice.
I think we already don't collect receive statistics on the vlan device
because the
io_buffers are received by the target device.  So perhaps that is not
a big loss.

Stefan