Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI

[Christoph Hellwig]

On Mon, Sep 12, 2011 at 10:14:08AM +0100, Stefan Hajnoczi wrote:
> > +static void
> > +iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
> > +{
> > +    IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
> > +    IscsiLun *iscsilun = acb->iscsilun;
> > +
> > +    acb->status = -ECANCELED;
> > +    acb->common.cb(acb->common.opaque, acb->status);
> > +    acb->canceled = 1;
> > +
> > +    iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
> > +                                     iscsi_abort_task_cb, NULL);
> > +}
> 
> The asynchronous abort task call looks odd.  If a caller allocates a
> buffer and issues a read request, then we need to make sure that the
> request is really aborted by the time .bdrv_aio_cancel() returns.

Shouldn't new drivers use coroutines instead of aio instead?

(just poking around, I don't fully understand the new scheme myself yet
 either)

> BDRV_O_NOCACHE is just for local files and sets the O_DIRECT hint.  It
> doesn't affect the cache semantics that the guest sees.

Now that I fixed it it doesn't.  But that's been a fairly recent change,
which isn't always easy to pick up people having external patches.

> 
> > +/*
> > + * We support iscsi url's on the form
> > + * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>
> > + */

Is having username + password on the command line really a that good idea?
Also what about the more complicated iSCSI authentification schemes?

> What will happen if BDRV_SECTOR_SIZE is not a multiple of 512?

hell will break lose for all of qemu.