From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50452) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XkESr-0005NJ-DE for qemu-devel@nongnu.org; Fri, 31 Oct 2014 11:51:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XjoXm-0008C1-9h for qemu-devel@nongnu.org; Thu, 30 Oct 2014 08:09:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34886) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XjoXm-0008Bx-1S for qemu-devel@nongnu.org; Thu, 30 Oct 2014 08:09:34 -0400 Message-ID: <54522A70.7070206@redhat.com> Date: Thu, 30 Oct 2014 13:09:20 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1414668226-11463-1-git-send-email-famz@redhat.com> <54522467.9060008@kamp.de> In-Reply-To: <54522467.9060008@kamp.de> Content-Type: text/plain; charset=windows-1252 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: Peter Lieven , Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi , Ronnie Sahlberg 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; } Paolo