Re: [Xen-devel] [PATCH] multi-page blkfront/blkback patch

["Jan Beulich" <jbeulich@xxxxxxxxxx>]

Some comments on infrastructural things, not the actual functionality:

>+      config 1_PAGE
>+              bool "1 page"
>+      config 2_PAGE
>+              bool "2 pages"
>+      config 4_PAGE
>+              bool "4 pages"

These names are too generic. At least XEN_ (and perhaps also BLKFRONT or
some such) should also appear in them.

>-static int blkif_reqs = 64;
>+static int blkif_reqs = 128;

Is this really related to the functionality introduced here?

>+      BUG_ON(BLK_NUM_RING_PAGES != 1 && 
>+             BLK_NUM_RING_PAGES != 2 && 
>+             BLK_NUM_RING_PAGES != 4);

This should be BUILD_BUG_ON().

>+#ifndef CONFIG_XEN_BLKDEV_NUM_RING_PAGES
>+#error "CONFIG_XEN_BLKDEV_NUM_RING_PAGES undefined!"
>+#endif

This won't work when building unmodified_drivers - I'm not sure what value
to default to here, though. But for compatibility it should probably be 1,
and the unmodified drivers' Kbuild could override it if desired.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel