Re: [Qemu-devel] [PATCH v2] iscsi: Refuse to open as writable if the LUN is write protected

[Peter Lieven <pl@kamp.de>]

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 <famz@redhat.com>
>>> ---
>>> 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 <pl@kamp.de>

and actually also

Tested-by: Peter Lieven <pl@kamp.de>

Peter