[Lguest] [PATCH 2/3] lguest: Simplify device descriptor helpers

[Rusty Russell]

On Friday 13 June 2008 23:04:59 Mark McLoughlin wrote:
> Confusingly, we use device_config() in add_virtqueue() to
> obtain a pointer to the next lguest_vqconfig pointer in
> the device descriptor, under the assumption that we haven't
> yet set up the feature bitmasks or device config.
>
> There's enough going on here that one might get horribly
> confused by this for a second.
>
> Simplify the code by adding helpers similar to lg_vq(),
> lg_features() and lg_config() functions from the kernel's
> guest device code.
>
> That way it's a bit more clear which config is which.

Hi Mark,

    Like the idea behind this, there are a couple of minor issues though.

> -/* The device virtqueue descriptors are followed by feature bitmasks. */
> -static u8 *get_feature_bits(struct device *dev)
> -{
> -	return (u8 *)(dev->desc + 1)
> -		+ dev->desc->num_vq * sizeof(struct lguest_vqconfig);
> -}
> -
...
>
> +static u8 *device_features(const struct device *dev);

I hate pre-declarations... perhaps hoist the other helpers upwards instead?

>  		/* Clear any features they've acked. */
> -		memset(get_feature_bits(dev) + dev->desc->feature_len, 0,
> +		memset(device_features(dev) + dev->desc->feature_len, 0,
>  		       dev->desc->feature_len);

Perhaps we should stick with get_feature_bits() to reduce churn.  But don't 
really mind.

> + * array of configuration bytes.  This routine returns a pointer to the
> + * virtqueue descriptors. */
> +static struct lguest_vqconfig *device_vq(const struct device *dev)
> +{
> +	return (void *)(dev->desc + 1);
> +}

Probably not the greatest name: device_vqconfig?

>  	vring_init(&vq->vring, num_descs, p, getpagesize());
>
> -	/* Append virtqueue to this device's descriptor.  We use
> -	 * device_config() to get the end of the device's current virtqueues;
> -	 * we check that we haven't added any config or feature information
> -	 * yet, otherwise we'd be overwriting them. */
> -	assert(dev->desc->config_len == 0 && dev->desc->feature_len == 0);
> -	memcpy(device_config(dev), &vq->config, sizeof(vq->config));
> +	/* Append virtqueue to this device's descriptor. */
> +	memcpy(device_vq(dev) + dev->desc->num_vq,
> +	       &vq->config, sizeof(vq->config));
>  	dev->desc->num_vq++;

Removing this assertion is incorrect.  Because of the simple layout we use, we 
really can't add virtqueues after we've added feature bits or config 
variables.

But clearer code is better than that comment :)

Cheers,
Rusty.