From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50917) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XkEQv-0005tc-Iz for qemu-devel@nongnu.org; Fri, 31 Oct 2014 11:52:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XjpGE-0007F2-AA for qemu-devel@nongnu.org; Thu, 30 Oct 2014 08:55:36 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:56756 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XjpGE-0007Ew-06 for qemu-devel@nongnu.org; Thu, 30 Oct 2014 08:55:30 -0400 Message-ID: <5452353C.4060706@kamp.de> Date: Thu, 30 Oct 2014 13:55:24 +0100 From: Peter Lieven MIME-Version: 1.0 References: <1414668226-11463-1-git-send-email-famz@redhat.com> <54522467.9060008@kamp.de> <54522A70.7070206@redhat.com> In-Reply-To: <54522A70.7070206@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] iscsi: Refuse to open as writable if the LUN is write protected List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi , Ronnie Sahlberg On 30.10.2014 13:09, Paolo Bonzini wrote: > > On 10/30/2014 12:43 PM, Peter Lieven wrote: >> On 30.10.2014 12:23, Fam Zheng wrote: >>> Before, when a write protected iSCSI target is attached as scsi-disk >>> with BDRV_O_RDWR, we report it as writable, while in fact all writes >>> will fail. >>> >>> One way to improve this is to report write protect flag as true to >>> guest, but a even better way is to refuse using a write protected LUN to >>> guest. >>> >>> Target write protect flag is checked with a mode sense query. >>> >>> Signed-off-by: Fam Zheng >>> --- >>> v2: Improve error message. >>> Fall back to a warning if mode sense failed. >>> Check unmarshal return value. >>> --- >>> block/iscsi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 46 insertions(+) >>> >>> diff --git a/block/iscsi.c b/block/iscsi.c >>> index 233f462..dcacbca 100644 >>> --- a/block/iscsi.c >>> +++ b/block/iscsi.c >>> @@ -1219,6 +1219,44 @@ static void >>> iscsi_attach_aio_context(BlockDriverState *bs, >>> qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL); >>> } >>> +static bool iscsi_is_write_protected(IscsiLun *iscsilun) >>> +{ >>> + struct scsi_task *task; >>> + struct scsi_mode_sense *ms = NULL; >>> + >>> + task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun, >>> + 1, SCSI_MODESENSE_PC_CURRENT, >>> + 0x3F, >>> + 0, 255); >>> + >>> + if (task == NULL) { >>> + error_report("Failed to send MODE_SENSE6 command: %s", >>> + iscsi_get_error(iscsilun->iscsi)); >>> + goto out; >>> + } >>> + >>> + if (task->status != SCSI_STATUS_GOOD) { >>> + error_report("MODE_SENSE6 failed: %s", >>> + iscsi_get_error(iscsilun->iscsi)); >>> + goto out; >>> + } >>> + ms = scsi_datain_unmarshall(task); >>> + if (!ms) { >>> + error_report("MODE_SENSE6 failed: %s", >>> + iscsi_get_error(iscsilun->iscsi)); >>> + goto out; >>> + } >>> +out: >>> + if (task) { >>> + scsi_free_scsi_task(task); >>> + } >>> + if (!ms) { >> ms points to freed memory after scsi_free_scsi_task. >> furthermore the requests likely fails with task->status != SCSI_STATUS_GOOD >> if the modesense implementation is broken etc. > This is a mix of your and Fam's code. Looks good? > > static bool iscsi_is_write_protected(IscsiLun *iscsilun) > { > struct scsi_task *task; > struct scsi_mode_sense *ms = NULL; > bool wrprotected = false; > > task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun, > 1, SCSI_MODESENSE_PC_CURRENT, > 0x3F, 0, 255); > if (task == NULL) { > error_report("Failed to send MODE_SENSE(6) command: %s", > iscsi_get_error(iscsilun->iscsi)); > goto out; > } > > if (task->status != SCSI_STATUS_GOOD) { > error_report("MODE_SENSE(6) failed: %s", > iscsi_get_error(iscsilun->iscsi)); > goto out; > } > ms = scsi_datain_unmarshall(task); > if (!ms) { > error_report("Failed to unmarshall MODE_SENSE(6) data: %s", > iscsi_get_error(iscsilun->iscsi)); > goto out; > } > wrprotected = ms->device_specific_parameter & 0x80; > > out: > if (task) { > scsi_free_scsi_task(task); > } > return wrprotected; > } i would add the prefix "iSCSI: " to the error_reports as we have it for other outputs. (noticed this after writing my mail). Otherwise looks good. Reviewed-by: Peter Lieven and actually also Tested-by: Peter Lieven Peter