From b93c94f7ec74a577a0f74c724e8d251f01eaf65a Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 6 Aug 2012 10:52:22 +0200 Subject: [PATCH 1/7] iscsi: do not leak initiator_name The argument of iscsi_create_context is never freed by libiscsi, which in fact calls strdup on it. Avoid a leak. Signed-off-by: Paolo Bonzini --- block/iscsi.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index 993a86d829..94063ab17d 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -943,7 +943,7 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags) error_report("Failed to parse URL : %s %s", filename, iscsi_get_error(iscsi)); ret = -EINVAL; - goto failed; + goto out; } memset(iscsilun, 0, sizeof(IscsiLun)); @@ -954,13 +954,13 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags) if (iscsi == NULL) { error_report("iSCSI: Failed to create iSCSI context."); ret = -ENOMEM; - goto failed; + goto out; } if (iscsi_set_targetname(iscsi, iscsi_url->target)) { error_report("iSCSI: Failed to set target name."); ret = -EINVAL; - goto failed; + goto out; } if (iscsi_url->user != NULL) { @@ -969,7 +969,7 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags) if (ret != 0) { error_report("Failed to set initiator username and password"); ret = -EINVAL; - goto failed; + goto out; } } @@ -977,13 +977,13 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags) if (parse_chap(iscsi, iscsi_url->target) != 0) { error_report("iSCSI: Failed to set CHAP user/password"); ret = -EINVAL; - goto failed; + goto out; } if (iscsi_set_session_type(iscsi, ISCSI_SESSION_NORMAL) != 0) { error_report("iSCSI: Failed to set session type to normal."); ret = -EINVAL; - goto failed; + goto out; } iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C); @@ -1004,7 +1004,7 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags) != 0) { error_report("iSCSI: Failed to start async connect."); ret = -EINVAL; - goto failed; + goto out; } while (!task.complete) { @@ -1015,11 +1015,7 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags) error_report("iSCSI: Failed to connect to LUN : %s", iscsi_get_error(iscsi)); ret = -EINVAL; - goto failed; - } - - if (iscsi_url != NULL) { - iscsi_destroy_url(iscsi_url); + goto out; } /* Medium changer or tape. We dont have any emulation for this so this must @@ -1031,19 +1027,22 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags) bs->sg = 1; } - return 0; + ret = 0; -failed: +out: if (initiator_name != NULL) { g_free(initiator_name); } if (iscsi_url != NULL) { iscsi_destroy_url(iscsi_url); } - if (iscsi != NULL) { - iscsi_destroy_context(iscsi); + + if (ret) { + if (iscsi != NULL) { + iscsi_destroy_context(iscsi); + } + memset(iscsilun, 0, sizeof(IscsiLun)); } - memset(iscsilun, 0, sizeof(IscsiLun)); return ret; } From f2ef4a6dd9f008d4cb30bccfc0491c01b69f1ead Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 6 Aug 2012 10:54:41 +0200 Subject: [PATCH 2/7] iscsi: reorganize code for parse_initiator_name Merge the occurrences of the "iqn.2008-11.org.linux-kvm" string to avoid duplication. Signed-off-by: Paolo Bonzini --- block/iscsi.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index 94063ab17d..fd954d4c1f 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -898,24 +898,21 @@ static char *parse_initiator_name(const char *target) const char *name = NULL; list = qemu_find_opts("iscsi"); - if (!list) { - return g_strdup("iqn.2008-11.org.linux-kvm"); - } - - opts = qemu_opts_find(list, target); - if (opts == NULL) { - opts = QTAILQ_FIRST(&list->head); + if (list) { + opts = qemu_opts_find(list, target); if (!opts) { - return g_strdup("iqn.2008-11.org.linux-kvm"); + opts = QTAILQ_FIRST(&list->head); + } + if (opts) { + name = qemu_opt_get(opts, "initiator-name"); } } - name = qemu_opt_get(opts, "initiator-name"); - if (!name) { + if (name) { + return g_strdup(name); + } else { return g_strdup("iqn.2008-11.org.linux-kvm"); } - - return g_strdup(name); } /* From 31459f463a32dc6c1818fa1aaa3d1f56c367b718 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Mon, 6 Aug 2012 18:24:55 +1000 Subject: [PATCH 3/7] iscsi: Pick default initiator-name based on the name of the VM This patch updates the iscsi layer to automatically pick a 'unique' initiator-name based on the name of the vm in case the user has not set an explicit iqn-name to use. Create a new function qemu_get_vm_name() that returns the name of the VM, if specified. This way we can thus create default names to use as the initiator name based on the guest session. If the VM is not named via the '-name' command line argument, the iscsi initiator-name used wiull simply be iqn.2008-11.org.linux-kvm If a name for the VM was specified with the '-name' option, iscsi will use a default initiatorname of iqn.2008-11.org.linux-kvm: These names are just the default iscsi initiator name that qemu will generate/use only when the user has not set an explicit initiator name via the commandlines or config files. Signed-off-by: Ronnie Sahlberg --- block/iscsi.c | 5 ++++- qemu-common.h | 1 + qemu-doc.texi | 5 +++++ qemu-options.hx | 8 ++++++++ qemu-tool.c | 5 +++++ vl.c | 5 +++++ 6 files changed, 28 insertions(+), 1 deletion(-) diff --git a/block/iscsi.c b/block/iscsi.c index fd954d4c1f..219f927823 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -896,6 +896,7 @@ static char *parse_initiator_name(const char *target) QemuOptsList *list; QemuOpts *opts; const char *name = NULL; + const char *iscsi_name = qemu_get_vm_name(); list = qemu_find_opts("iscsi"); if (list) { @@ -911,7 +912,9 @@ static char *parse_initiator_name(const char *target) if (name) { return g_strdup(name); } else { - return g_strdup("iqn.2008-11.org.linux-kvm"); + return g_strdup_printf("iqn.2008-11.org.linux-kvm%s%s", + iscsi_name ? ":" : "", + iscsi_name ? iscsi_name : ""); } } diff --git a/qemu-common.h b/qemu-common.h index f16079f432..f9deca6f86 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -376,6 +376,7 @@ bool buffer_is_zero(const void *buf, size_t len); void qemu_progress_init(int enabled, float min_skip); void qemu_progress_end(void); void qemu_progress_print(float delta, int max); +const char *qemu_get_vm_name(void); #define QEMU_FILE_TYPE_BIOS 0 #define QEMU_FILE_TYPE_KEYMAP 1 diff --git a/qemu-doc.texi b/qemu-doc.texi index f32e9e2fb9..35cabbcb9e 100644 --- a/qemu-doc.texi +++ b/qemu-doc.texi @@ -734,6 +734,11 @@ Various session related parameters can be set via special options, either in a configuration file provided via '-readconfig' or directly on the command line. +If the initiator-name is not specified qemu will use a default name +of 'iqn.2008-11.org.linux-kvm[:'] where is the name of the +virtual machine. + + @example Setting a specific initiator name to use when logging in to the target -iscsi initiator-name=iqn.qemu.test:my-initiator diff --git a/qemu-options.hx b/qemu-options.hx index 5e7d0dc035..47cb5bd311 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1897,6 +1897,11 @@ images for the guest storage. Both disk and cdrom images are supported. Syntax for specifying iSCSI LUNs is ``iscsi://[:]//'' +By default qemu will use the iSCSI initiator-name +'iqn.2008-11.org.linux-kvm[:]' but this can also be set from the command +line or a configuration file. + + Example (without authentication): @example qemu-system-i386 -iscsi initiator-name=iqn.2001-04.com.example:my-initiator \ @@ -1926,6 +1931,9 @@ DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi, " iSCSI session parameters\n", QEMU_ARCH_ALL) STEXI +iSCSI parameters such as username and password can also be specified via +a configuration file. See qemu-doc for more information and examples. + @item NBD QEMU supports NBD (Network Block Devices) both using TCP protocol as well as Unix Domain Sockets. diff --git a/qemu-tool.c b/qemu-tool.c index 318c5fcbca..64b5e88bc7 100644 --- a/qemu-tool.c +++ b/qemu-tool.c @@ -30,6 +30,11 @@ struct QEMUBH void *opaque; }; +const char *qemu_get_vm_name(void) +{ + return NULL; +} + Monitor *cur_mon; int monitor_cur_is_qmp(void) diff --git a/vl.c b/vl.c index e71cb30ecf..065aec290e 100644 --- a/vl.c +++ b/vl.c @@ -293,6 +293,11 @@ static struct { { .driver = "qxl-vga", .flag = &default_vga }, }; +const char *qemu_get_vm_name(void) +{ + return qemu_name; +} + static void res_free(void) { if (boot_splash_filedata != NULL) { From 4dd7c82cdbabe54386ef31939f865469a095c9c3 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 8 Aug 2012 16:26:16 +0200 Subject: [PATCH 4/7] virtio-scsi: do not compare 32-bit QEMU tags against 64-bit virtio-scsi tags This patch fixes a problem in handling task management functions in virtio-scsi. The cause of the problem is a mismatch between the size of the tag in QEMU (32-bit) and virtio-scsi (64-bit). Changing the QEMU size is hard because the migration format uses 32 bits to store the tag; so just don't use the QEMU tag (virtio-scsi only uses the tag for task management functions anyway) and look up the full 64-bit tag in the hba_private field. The reproducer is a bit obscure. If you cause an I/O timeout (for example with rerror=stop and doing 'cont' on the monitor continuously without fixing the error), sooner or later the guest will try to abort the command and reissue it. At this point, QEMU will report _two_ errors instead of one when you hit 'c', because the first error has not been canceled correctly. Signed-off-by: Paolo Bonzini --- hw/virtio-scsi.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c index c4a5b22f94..5f737acd97 100644 --- a/hw/virtio-scsi.c +++ b/hw/virtio-scsi.c @@ -305,11 +305,17 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) goto incorrect_lun; } QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) { - if (r->tag == req->req.tmf->tag) { + VirtIOSCSIReq *cmd_req = r->hba_private; + if (cmd_req && cmd_req->req.cmd->tag == req->req.tmf->tag) { break; } } - if (r && r->hba_private) { + if (r) { + /* + * Assert that the request has not been completed yet, we + * check for it in the loop above. + */ + assert(r->hba_private); if (req->req.tmf->subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK) { /* "If the specified command is present in the task set, then * return a service response set to FUNCTION SUCCEEDED". From 46e3f30e3c81e23c07f16b2193dfb6928646c205 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 9 Aug 2012 11:33:26 +0200 Subject: [PATCH 5/7] scsi-disk: more assertions and resets for aiocb Leaving the aiocb to a non-NULL value leads to an assertion failure when rerror/werror are set to stop or enospc, and the operation is retried. scsi-disk checks that the aiocb member is NULL before filling it. This patch correctly resets the aiocb to NULL values everywhere, and adds the dual assertion that the aiocb was non-NULL before calling bdrv_acct_done. Signed-off-by: Paolo Bonzini --- hw/scsi-disk.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index a9c727905a..9af9d18fad 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -175,6 +175,8 @@ static void scsi_aio_complete(void *opaque, int ret) SCSIDiskReq *r = (SCSIDiskReq *)opaque; SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev); + assert(r->req.aiocb != NULL); + r->req.aiocb = NULL; bdrv_acct_done(s->qdev.conf.bs, &r->acct); if (ret < 0) { @@ -238,10 +240,9 @@ static void scsi_dma_complete(void *opaque, int ret) SCSIDiskReq *r = (SCSIDiskReq *)opaque; SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev); - if (r->req.aiocb != NULL) { - r->req.aiocb = NULL; - bdrv_acct_done(s->qdev.conf.bs, &r->acct); - } + assert(r->req.aiocb != NULL); + r->req.aiocb = NULL; + bdrv_acct_done(s->qdev.conf.bs, &r->acct); if (ret < 0) { if (scsi_handle_rw_error(r, -ret)) { @@ -270,10 +271,9 @@ static void scsi_read_complete(void * opaque, int ret) SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev); int n; - if (r->req.aiocb != NULL) { - r->req.aiocb = NULL; - bdrv_acct_done(s->qdev.conf.bs, &r->acct); - } + assert(r->req.aiocb != NULL); + r->req.aiocb = NULL; + bdrv_acct_done(s->qdev.conf.bs, &r->acct); if (ret < 0) { if (scsi_handle_rw_error(r, -ret)) { From a084a703df9ab896c9d30ac479e1388e5e4cafb0 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 9 Aug 2012 13:34:53 +0200 Subject: [PATCH 6/7] scsi-disk: improve out-of-range LBA detection for WRITE SAME Signed-off-by: Paolo Bonzini --- hw/scsi-disk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index 9af9d18fad..584aec13fb 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -1712,7 +1712,8 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf) scsi_check_condition(r, SENSE_CODE(WRITE_PROTECTED)); return 0; } - if (r->req.cmd.lba > s->qdev.max_lba) { + if (r->req.cmd.lba > r->req.cmd.lba + nb_sectors || + r->req.cmd.lba + nb_sectors - 1 > s->qdev.max_lba) { goto illegal_lba; } From 5222aaf223e52961cabeb7cabc579892ccd8bc59 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 9 Aug 2012 11:00:22 +0200 Subject: [PATCH 7/7] scsi-disk: add support for the UNMAP command The unmap command can reuse the same infrastructure as MODE SELECT for reading the descriptor list into memory. The descriptors are processed sequentially. Signed-off-by: Paolo Bonzini --- hw/scsi-disk.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 92 insertions(+), 1 deletion(-) diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index 584aec13fb..e71809e091 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -637,7 +637,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) { buflen = 8; outbuf[4] = 0; - outbuf[5] = 0x60; /* write_same 10/16 supported */ + outbuf[5] = 0xe0; /* unmap & write_same 10/16 all supported */ outbuf[6] = s->qdev.conf.discard_granularity ? 2 : 1; outbuf[7] = 0; break; @@ -1449,6 +1449,89 @@ invalid_field: return; } +typedef struct UnmapCBData { + SCSIDiskReq *r; + uint8_t *inbuf; + int count; +} UnmapCBData; + +static void scsi_unmap_complete(void *opaque, int ret) +{ + UnmapCBData *data = opaque; + SCSIDiskReq *r = data->r; + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev); + uint64_t sector_num; + uint32 nb_sectors; + + r->req.aiocb = NULL; + if (ret < 0) { + if (scsi_handle_rw_error(r, -ret)) { + goto done; + } + } + + if (data->count > 0 && !r->req.io_canceled) { + sector_num = ldq_be_p(&data->inbuf[0]); + nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL; + if (sector_num > sector_num + nb_sectors || + sector_num + nb_sectors - 1 > s->qdev.max_lba) { + scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE)); + goto done; + } + + r->req.aiocb = bdrv_aio_discard(s->qdev.conf.bs, + sector_num * (s->qdev.blocksize / 512), + nb_sectors * (s->qdev.blocksize / 512), + scsi_unmap_complete, data); + data->count--; + data->inbuf += 16; + return; + } + +done: + if (data->count == 0) { + scsi_req_complete(&r->req, GOOD); + } + if (!r->req.io_canceled) { + scsi_req_unref(&r->req); + } + g_free(data); +} + +static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf) +{ + uint8_t *p = inbuf; + int len = r->req.cmd.xfer; + UnmapCBData *data; + + if (len < 8) { + goto invalid_param_len; + } + if (len < lduw_be_p(&p[0]) + 2) { + goto invalid_param_len; + } + if (len < lduw_be_p(&p[2]) + 8) { + goto invalid_param_len; + } + if (lduw_be_p(&p[2]) & 15) { + goto invalid_param_len; + } + + data = g_new0(UnmapCBData, 1); + data->r = r; + data->inbuf = &p[8]; + data->count = lduw_be_p(&p[2]) >> 4; + + /* The matching unref is in scsi_unmap_complete, before data is freed. */ + scsi_req_ref(&r->req); + scsi_unmap_complete(data, 0); + return; + +invalid_param_len: + scsi_check_condition(r, SENSE_CODE(INVALID_PARAM_LEN)); + return; +} + static void scsi_disk_emulate_write_data(SCSIRequest *req) { SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req); @@ -1468,6 +1551,10 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req) scsi_disk_emulate_mode_select(r, r->iov.iov_base); break; + case UNMAP: + scsi_disk_emulate_unmap(r, r->iov.iov_base); + break; + default: abort(); } @@ -1702,6 +1789,9 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf) case MODE_SELECT_10: DPRINTF("Mode Select(10) (len %lu)\n", (long)r->req.cmd.xfer); break; + case UNMAP: + DPRINTF("Unmap (len %lu)\n", (long)r->req.cmd.xfer); + break; case WRITE_SAME_10: nb_sectors = lduw_be_p(&req->cmd.buf[7]); goto write_same; @@ -2067,6 +2157,7 @@ static const SCSIReqOps *const scsi_disk_reqops_dispatch[256] = { [SEEK_10] = &scsi_disk_emulate_reqops, [MODE_SELECT] = &scsi_disk_emulate_reqops, [MODE_SELECT_10] = &scsi_disk_emulate_reqops, + [UNMAP] = &scsi_disk_emulate_reqops, [WRITE_SAME_10] = &scsi_disk_emulate_reqops, [WRITE_SAME_16] = &scsi_disk_emulate_reqops,