WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [patch 20/20] XEN-paravirt: Add Xen virtual block device

To: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Subject: [Xen-devel] Re: [patch 20/20] XEN-paravirt: Add Xen virtual block device driver.
From: Arjan van de Ven <arjan@xxxxxxxxxxxxx>
Date: Sat, 13 Jan 2007 17:07:28 -0800
Cc: Andrew Morton <akpm@xxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxx, Ian Pratt <ian.pratt@xxxxxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, Chris Wright <chrisw@xxxxxxxxxxxx>, virtualization@xxxxxxxxxxxxxx, Christian Limpach <Christian.Limpach@xxxxxxxxxxxx>
Delivery-date: Mon, 15 Jan 2007 03:01:06 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <20070113014649.256179743@xxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Intel International BV
References: <20070113014539.408244126@xxxxxxxx> <20070113014649.256179743@xxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
> +#include "../../../arch/i386/paravirt-xen/events.h"
> +#include "../../../arch/i386/paravirt-xen/xen-page.h"

this shows the headers are clearly in the wrong place...
> +
> +     err = xenbus_printf(xbt, dev->nodename,
> +                         "ring-ref","%u", info->ring_ref);

why do you need your own printf?

> +static inline int GET_ID_FROM_FREELIST(

does this really need screaming?


> +
> +int blkif_ioctl(struct inode *inode, struct file *filep,
> +             unsigned command, unsigned long argument)
> +{
> +     int i;
> +
> +     DPRINTK_IOCTL("command: 0x%x, argument: 0x%lx, dev: 0x%04x\n",
> +                   command, (long)argument, inode->i_rdev);
> +
> +     switch (command) {
> +     case CDROMMULTISESSION:
> +             DPRINTK("FIXME: support multisession CDs later\n");
> +             for (i = 0; i < sizeof(struct cdrom_multisession); i++)
> +                     if (put_user(0, (char __user *)(argument + i)))
> +                             return -EFAULT;
> +             return 0;
> +
> +     default:
> +             /*printk(KERN_ALERT "ioctl %08x not supported by Xen blkdev\n",
> +               command);*/
> +             return -EINVAL; /* same return as native Linux */
> +     }

eh so you implement no ioctls.. why then implement the ioctl method at
all?


> +static struct xenbus_driver blkfront = {
> +     .name = "vbd",
> +     .owner = THIS_MODULE,
> +     .ids = blkfront_ids,
> +     .probe = blkfront_probe,
> +     .remove = blkfront_remove,
> +     .resume = blkfront_resume,
> +     .otherend_changed = backend_changed,
> +};

this can be const

> +
> +#define DPRINTK(_f, _a...) pr_debug(_f, ## _a)

why this silly abstraction? Just use pr_debug in the code directly




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

<Prev in Thread] Current Thread [Next in Thread>