Re: [Qemu-devel] [PATCH] libiscsi

[ronnie sahlberg]

Thanks.

I have implemented all of your suggestions and will submit version 2
of the patch shortly.

regards
ronnie sahlberg

On Tue, Dec 14, 2010 at 7:47 AM, Blue Swirl <address@hidden> wrote:
> On Mon, Dec 13, 2010 at 8:05 AM, Ronnie Sahlberg
> <address@hidden> wrote:
>> This patch adds a new block driver : block.iscsi.c
>> This driver interfaces with the multiplatform posix library
>> for iscsi initiator/client access to iscsi devices hosted at
>> git://github.com/sahlberg/libiscsi.git
>>
>> The patch adds the driver to interface with the iscsi library.
>> It also updated the configure script to
>> * by default, probe is libiscsi is available and if so, build
>>  qemu against libiscsi.
>> * --enable-libiscsi
>>  Force a build against libiscsi. If libiscsi is not available
>>  the build will fail.
>> * --disable-libiscsi
>>  Do not link against libiscsi, even if it is available.
>>
>> When linked with libiscsi, qemu gains support to access iscsi resources
>> such as disks and cdrom directly, without having to make the devices visible
>> to the host.
>>
>> You can specify devices using a iscsi url of the form :
>> iscsi://<host>[:<port]/<target-iqn-name>/<lun>
>>
>> Example:
>> -drive file=iscsi://10.1.1.1:3260/iqn.ronnie.test/1
>>
>> -cdrom iscsi://10.1.1.1:3260/iqn.ronnie.test/2
>>
>> Signed-off-by: Ronnie Sahlberg <address@hidden>
>> ---
>>  Makefile.objs |    2 +-
>>  block/iscsi.c |  528 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  configure     |   29 +++
>>  3 files changed, 558 insertions(+), 1 deletions(-)
>>  create mode 100644 block/iscsi.c
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index cebb945..81731c5 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -22,7 +22,7 @@ block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o 
>> dmg.o bochs.o vpc.o vv
>>  block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o
>>  block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
>>  block-nested-$(CONFIG_WIN32) += raw-win32.o
>> -block-nested-$(CONFIG_POSIX) += raw-posix.o
>> +block-nested-$(CONFIG_POSIX) += raw-posix.o iscsi.o
>
> Please use CONFIG_ISCSI...
>
>>  block-nested-$(CONFIG_CURL) += curl.o
>>
>>  block-obj-y +=  $(addprefix block/, $(block-nested-y))
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> new file mode 100644
>> index 0000000..fba5ee6
>> --- /dev/null
>> +++ b/block/iscsi.c
>> @@ -0,0 +1,528 @@
>> +/*
>> + * QEMU Block driver for iSCSI images
>> + *
>> + * Copyright (c) 2010 Ronnie Sahlberg <address@hidden>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>> copy
>> + * of this software and associated documentation files (the "Software"), to 
>> deal
>> + * in the Software without restriction, including without limitation the 
>> rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included 
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
>> FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "config-host.h"
>> +#ifdef CONFIG_LIBISCSI
>
> ... then this is not needed.
>
>> +
>> +#include <poll.h>
>> +#include "sysemu.h"
>> +#include "qemu-common.h"
>> +#include "qemu-error.h"
>> +#include "block_int.h"
>> +
>> +#include <iscsi/iscsi.h>
>> +#include <iscsi/scsi-lowlevel.h>
>> +
>> +
>> +typedef struct ISCSILUN {
>> +    struct iscsi_context *iscsi;
>> +    int lun;
>> +    int block_size;
>> +    unsigned long num_blocks;
>> +} ISCSILUN;
>> +
>> +typedef struct ISCSIAIOCB {
>> +    BlockDriverAIOCB common;
>> +    QEMUIOVector *qiov;
>> +    QEMUBH *bh;
>> +    ISCSILUN *iscsilun;
>> +    int canceled;
>> +    int status;
>> +    size_t read_size;
>> +} ISCSIAIOCB;
>> +
>> +struct iscsi_task {
>> +    ISCSILUN *iscsilun;
>> +    int status;
>> +    int complete;
>> +};
>
> Please see CODING_STYLE for struct naming and use of typedefs.
>
>> +
>> +static int
>> +iscsi_is_inserted(BlockDriverState *bs)
>> +{
>> +    ISCSILUN *iscsilun = bs->opaque;
>> +    struct iscsi_context *iscsi = iscsilun->iscsi;
>> +
>> +    return iscsi_is_logged_in(iscsi);
>> +}
>> +
>> +
>> +static void
>> +iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
>> +{
>> +    ISCSIAIOCB *acb = (ISCSIAIOCB *)blockacb;
>> +
>> +    acb->status = -EIO;
>> +    acb->common.cb(acb->common.opaque, acb->status);
>> +    acb->canceled = 1;
>> +}
>> +
>> +static AIOPool iscsi_aio_pool = {
>> +    .aiocb_size         = sizeof(ISCSIAIOCB),
>> +    .cancel             = iscsi_aio_cancel,
>> +};
>> +
>> +
>> +static void iscsi_process_read(void *arg);
>> +static void iscsi_process_write(void *arg);
>> +
>> +static void
>> +iscsi_set_events(ISCSILUN *iscsilun)
>> +{
>> +    struct iscsi_context *iscsi = iscsilun->iscsi;
>> +
>> +    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), iscsi_process_read,
>> +                           (iscsi_which_events(iscsi)&POLLOUT)
>
> Please insert space around operators.
>
>> +                           ?iscsi_process_write:NULL,
>
> Also around ?:.
>
>> +                           NULL, NULL, iscsilun);
>> +}
>> +
>> +static void
>> +iscsi_process_read(void *arg)
>> +{
>> +    ISCSILUN *iscsilun = arg;
>> +    struct iscsi_context *iscsi = iscsilun->iscsi;
>> +
>> +    iscsi_service(iscsi, POLLIN);
>> +    iscsi_set_events(iscsilun);
>> +}
>> +
>> +static void
>> +iscsi_process_write(void *arg)
>> +{
>> +    ISCSILUN *iscsilun = arg;
>> +    struct iscsi_context *iscsi = iscsilun->iscsi;
>> +
>> +    iscsi_service(iscsi, POLLOUT);
>> +    iscsi_set_events(iscsilun);
>> +}
>> +
>> +
>> +static int
>> +iscsi_schedule_bh(QEMUBHFunc *cb, ISCSIAIOCB *acb)
>> +{
>> +    acb->bh = qemu_bh_new(cb, acb);
>> +    if (!acb->bh) {
>> +       error_report("oom: could not create iscsi bh");
>> +       return -EIO;
>> +    }
>> +
>> +    qemu_bh_schedule(acb->bh);
>> +    return 0;
>> +}
>> +
>> +static void
>> +iscsi_readv_writev_bh_cb(void *p)
>> +{
>> +    ISCSIAIOCB *acb = p;
>> +
>> +    acb->common.cb(acb->common.opaque, acb->status);
>> +    qemu_aio_release(acb);
>> +}
>> +
>> +static void
>> +iscsi_aio_write10_cb(struct iscsi_context *iscsi, int status,
>> +                    void *command_data, void *private_data)
>> +{
>> +    ISCSIAIOCB *acb = private_data;
>> +
>> +    if (acb->canceled != 0) {
>> +       qemu_aio_release(acb);
>> +       return;
>> +    }
>> +
>> +    acb->status = 0;
>> +    if (status < 0) {
>> +       error_report("Failed to write10 data to iSCSI lun. %s",
>> +                    iscsi_get_error(iscsi));
>> +       acb->status = -EIO;
>> +    }
>> +
>> +    iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
>> +}
>> +
>> +static BlockDriverAIOCB *
>> +iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num,
>> +                QEMUIOVector *qiov, int nb_sectors,
>> +                BlockDriverCompletionFunc *cb,
>> +                void *opaque)
>> +{
>> +    ISCSILUN *iscsilun = bs->opaque;
>> +    struct iscsi_context *iscsi = iscsilun->iscsi;
>> +    ISCSIAIOCB *acb;
>> +    size_t size;
>> +    unsigned char *buf;
>
> Why unsigned? Maybe uint8_t or plain char instead?
>
>> +
>> +    acb = qemu_aio_get(&iscsi_aio_pool, bs, cb, opaque);
>> +    if (!acb) {
>> +       return NULL;
>> +    }
>> +
>> +    acb->iscsilun = iscsilun;
>> +    acb->qiov     = qiov;
>> +
>> +    acb->canceled   = 0;
>> +
>> +    /* XXX we should pass the iovec to write10 to avoid the extra copy */
>> +    /* this will allow us to get rid of 'buf' completely */
>> +    size = nb_sectors * BDRV_SECTOR_SIZE;
>> +    buf = qemu_malloc(size);
>> +    qemu_iovec_to_buffer(acb->qiov, buf);
>> +    if (iscsi_write10_async(iscsi, iscsilun->lun, buf, size,
>> +                           sector_num * BDRV_SECTOR_SIZE
>> +                           / iscsilun->block_size,
>> +                           0, 0, iscsilun->block_size,
>> +                           iscsi_aio_write10_cb, acb) != 0) {
>
> When the function call is this long, perhaps it would be more readable
> to assign a temporary variable with the result and use that in if
> expression.
>
> The expression sector_num * BDRV_SECTOR_SIZE / iscsilun->block_size is
> used often, refactoring this to a function could make the code a bit
> clearer.
>
>> +       error_report("iSCSI: Failed to send write10 command. %s",
>> +                    iscsi_get_error(iscsi));
>> +       qemu_free(buf);
>> +       qemu_aio_release(acb);
>> +       return NULL;
>> +    }
>> +    qemu_free(buf);
>> +    iscsi_set_events(iscsilun);
>> +
>> +    return &acb->common;
>> +}
>> +
>> +static void
>> +iscsi_aio_read10_cb(struct iscsi_context *iscsi, int status,
>> +                   void *command_data, void *private_data)
>> +{
>> +    ISCSIAIOCB *acb = private_data;
>> +    struct scsi_task *scsi = command_data;
>> +
>> +    if (acb->canceled != 0) {
>> +       qemu_aio_release(acb);
>> +       return;
>> +    }
>> +
>> +    acb->status = 0;
>> +    if (status < 0) {
>> +       error_report("Failed to read10 data from iSCSI lun. %s",
>> +                    iscsi_get_error(iscsi));
>> +       acb->status = -EIO;
>> +    } else {
>> +       qemu_iovec_from_buffer(acb->qiov, scsi->datain.data, acb->read_size);
>> +    }
>> +
>> +    iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
>> +}
>> +
>> +static BlockDriverAIOCB *
>> +iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num,
>> +               QEMUIOVector *qiov, int nb_sectors,
>> +               BlockDriverCompletionFunc *cb,
>> +               void *opaque)
>> +{
>> +    ISCSILUN *iscsilun = bs->opaque;
>> +    struct iscsi_context *iscsi = iscsilun->iscsi;
>> +    ISCSIAIOCB *acb;
>> +    size_t qemu_read_size, lun_read_size;
>> +
>> +    qemu_read_size = BDRV_SECTOR_SIZE * (size_t)nb_sectors;
>> +    lun_read_size  = (qemu_read_size + iscsilun->block_size - 1)
>> +      / iscsilun->block_size * iscsilun->block_size;
>> +
>> +    acb = qemu_aio_get(&iscsi_aio_pool, bs, cb, opaque);
>> +    if (!acb) {
>> +       return NULL;
>> +    }
>> +
>> +    acb->iscsilun = iscsilun;
>> +    acb->qiov     = qiov;
>> +
>> +    acb->canceled   = 0;
>> +    acb->read_size  = qemu_read_size;
>> +
>> +    if (iscsi_read10_async(iscsi, iscsilun->lun,
>> +                          sector_num * BDRV_SECTOR_SIZE
>> +                          / iscsilun->block_size,
>> +                          lun_read_size, iscsilun->block_size,
>> +                          iscsi_aio_read10_cb, acb) != 0) {
>> +       error_report("iSCSI: Failed to send read10 command. %s",
>> +                    iscsi_get_error(iscsi));
>> +       qemu_aio_release(acb);
>> +       return NULL;
>> +    }
>> +    iscsi_set_events(iscsilun);
>> +
>> +    return &acb->common;
>> +}
>> +
>> +
>> +static void synccache10_cb(struct iscsi_context *iscsi, int status,
>> +                          void *command_data, void *private_data)
>
> Usually 'opaque' is used instead of 'private_data'.
>
>> +{
>> +    struct iscsi_task *task = private_data;
>> +
>> +    task->status   = status;
>> +    task->complete = 1;
>> +}
>> +
>> +static int
>> +iscsi_flush(BlockDriverState *bs)
>> +{
>> +    ISCSILUN *iscsilun = bs->opaque;
>> +    struct iscsi_context *iscsi = iscsilun->iscsi;
>> +    struct iscsi_task task;
>> +
>> +    task.iscsilun = iscsilun;
>> +    task.status = 0;
>> +    task.complete = 0;
>> +
>> +    if (iscsi_synchronizecache10_async(iscsi, iscsilun->lun,
>> +                                      0, 0, 0, 0,
>> +                                      synccache10_cb,
>> +                                      &task) != 0) {
>> +       error_report("iSCSI: Failed to send SYNCHRONIZECACHE10. %s",
>> +                    iscsi_get_error(iscsi));
>> +        return -1;
>> +    }
>> +
>> +    async_context_push();
>> +    while (!task.complete) {
>> +       iscsi_set_events(iscsilun);
>> +       qemu_aio_wait();
>> +    }
>> +    async_context_pop();
>> +    if (task.status != 0) {
>> +       error_report("iSCSI: Failed to send SYNCHRONIZECACHE10: %s",
>> +                    iscsi_get_error(iscsi));
>> +       return -EIO;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int64_t
>> +iscsi_getlength(BlockDriverState *bs)
>> +{
>> +    ISCSILUN *iscsilun = bs->opaque;
>> +    int64_t len;
>> +
>> +    len  = iscsilun->num_blocks;
>> +    len *= iscsilun->block_size;
>> +
>> +    return len;
>> +}
>> +
>> +static void
>> +iscsi_readcapacity10_cb(struct iscsi_context *iscsi, int status,
>> +                       void *command_data, void *private_data)
>> +{
>> +    struct iscsi_task *task = private_data;
>> +    struct scsi_readcapacity10 *rc10;
>> +    struct scsi_task *scsi = command_data;
>> +
>> +    if (status != 0) {
>> +       error_report("iSCSI: Failed to read capacity of iSCSI lun. %s",
>> +                    iscsi_get_error(iscsi));
>> +       task->status   = 1;
>> +       task->complete = 1;
>> +       return;
>> +    }
>> +
>> +    rc10 = scsi_datain_unmarshall(scsi);
>> +    if (rc10 == NULL) {
>> +       error_report("iSCSI: Failed to unmarshall readcapacity10 data.");
>> +       task->status   = 1;
>> +       task->complete = 1;
>> +       return;
>> +    }
>> +
>> +    task->iscsilun->block_size = rc10->block_size;
>> +    task->iscsilun->num_blocks = rc10->lba;
>> +
>> +    task->status   = 0;
>> +    task->complete = 1;
>> +}
>> +
>> +
>> +static void
>> +iscsi_connect_cb(struct iscsi_context *iscsi, int status, void 
>> *command_data,
>> +                void *private_data)
>> +{
>> +    struct iscsi_task *task = private_data;
>> +
>> +    if (status != 0) {
>> +       task->status   = 1;
>> +       task->complete = 1;
>> +       return;
>> +    }
>> +
>> +    if (iscsi_readcapacity10_async(iscsi, task->iscsilun->lun, 0, 0,
>> +                                  iscsi_readcapacity10_cb, private_data)
>> +       != 0) {
>> +       error_report("iSCSI: failed to send readcapacity command.");
>> +       task->status   = 1;
>> +       task->complete = 1;
>> +    }
>> +}
>> +
>> +/*
>> + * We support iscsi url's on the form
>> + * iscsi://<host>[:<port>]/<targetname>/<lun>
>> + */
>> +static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
>> +{
>> +    ISCSILUN *iscsilun = bs->opaque;
>> +    struct iscsi_context *iscsi = NULL;
>> +    struct iscsi_task task;
>> +    char *ptr, *host, *target, *url;
>> +    char *tmp;
>> +    int ret, lun;
>> +
>> +    bzero(iscsilun, sizeof(ISCSILUN));
>
> memset()
>
>> +
>> +    url = qemu_strdup(filename);
>> +
>> +    if (strncmp(url, "iscsi://", 8)) {
>> +       error_report("iSCSI: url does not start with 'iscsi://'");
>> +       ret = -EINVAL;
>> +       goto failed;
>> +    }
>> +
>> +    host = url + 8;
>> +
>> +    ptr = index(host, '/');
>
> strchr()
>