Christoph Hellwig wrote:
>> source "drivers/s390/block/Kconfig"
>> +source "drivers/block/xen/Kconfig"
>>
>
> Please don't add a new subdirectory for a tiny new driver that
> really should be only a single .c file.
>
Yup.
>> +config XEN_BLKDEV_FRONTEND
>> + tristate "Block device frontend driver"
>> + depends on XEN
>> + default y
>>
>
> Please kill the default statement. We really should have script that
> autorejects every new driver that wants to be default y..
>
I guess. But if you've already decided to enable Xen, it seems to me
that the default option should reflect the common case. But I don't
feel strongly about it either way.
>> +#define BLKIF_STATE_DISCONNECTED 0
>> +#define BLKIF_STATE_CONNECTED 1
>> +#define BLKIF_STATE_SUSPENDED 2
>>
>
> enum, please.
>
OK.
> Please get rid of all the forward-declarations by putting the code
> into proper order.
>
Yep. I'll de-generic the names too.
>> +static inline int GET_ID_FROM_FREELIST(
>> + struct blkfront_info *info)
>> +{
>> + unsigned long free = info->shadow_free;
>> + BUG_ON(free > BLK_RING_SIZE);
>> + info->shadow_free = info->shadow[free].req.id;
>> + info->shadow[free].req.id = 0x0fffffee; /* debug */
>> + return free;
>> +}
>> +
>> +static inline void ADD_ID_TO_FREELIST(
>> + struct blkfront_info *info, unsigned long id)
>> +{
>> + info->shadow[id].req.id = info->shadow_free;
>> + info->shadow[id].request = 0;
>> + info->shadow_free = id;
>> +}
>>
>
> Please give these proper lowercased names, and while you're at it
> please kill all the inline statements you have and let the compiler
> do it's work.
>
OK.
>> +static irqreturn_t blkif_int(int irq, void *dev_id)
>>
>
> Please call interrupt handlers foo_interrupt, that makes peopl see what's
> going on much more easily.
>
OK.
>> +static void blkif_recover(struct blkfront_info *info)
>> +{
>> + int i;
>> + struct blkif_request *req;
>> + struct blk_shadow *copy;
>> + int j;
>> +
>> + /* Stage 1: Make a safe copy of the shadow state. */
>> + copy = kmalloc(sizeof(info->shadow), GFP_KERNEL | __GFP_NOFAIL);
>>
>
> Please don't use __GFP_NOFAIL in any new code.
>
OK.
[...]
The rest looks sound. I'll go through it in detail, but a lot of your
points things I've been meaning to get to anyway.
Thanks,
J
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/virtualization
