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 <blauwir...@gmail.com> wrote: > On Mon, Dec 13, 2010 at 8:05 AM, Ronnie Sahlberg > <ronniesahlb...@gmail.com> 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 <ronniesahlb...@gmail.com> >> --- >> 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 <ronniesahlb...@gmail.com> >> + * >> + * 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() >