[Qemu-devel] [PATCH 0/2] disable the scsi pass-through eject from monitor

[Qemu-devel] [PATCH 0/2] disable the scsi pass-through eject from monitor

[Pavel Hrdina]

This series do two things, at first it fixes the bug introduced by
Paolo's patch series. The bug is that he accidentally removes the check
after the bdrv_aio_ioctl() function which still could return NULL.

The second patch fixes the issue that you could eject the scsi pass-through
device from the guest. Probably it should be later fixed in the way that
the eject command will not remove the device from guest, but send the eject
request to the real device in case of removable device.

Pavel Hrdina (2):
  scsi-generic: check the return value of bdrv_aio_ioctl in
    execute_command
  scsi-disk: scsi-block device for scsi pass-through should not be
    removable

 hw/scsi/scsi-disk.c    | 10 +++++++---
 hw/scsi/scsi-generic.c |  4 ++++
 2 files changed, 11 insertions(+), 3 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/2] scsi-generic: check the return value of bdrv_aio_ioctl in execute_command

[Pavel Hrdina]

This fixes the bug introduced by this commit ad54ae80c73f.
The bdrv_aio_ioctl() still could return null and we should return an error
in that case.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hw/scsi/scsi-generic.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 2a9a561..8a4dae2 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -174,6 +174,10 @@ static int execute_command(BlockDriverState *bdrv,
     r->io_header.flags |= SG_FLAG_DIRECT_IO;
 
     r->req.aiocb = bdrv_aio_ioctl(bdrv, SG_IO, &r->io_header, complete, r);
+    if (r->req.aiocb == NULL) {
+        BADF("execute_command: read failed !\n");
+        return -EIO;
+    }
 
     return 0;
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/2] scsi-disk: scsi-block device for scsi pass-through should not be removable

[Pavel Hrdina]

This patch adds a new SCSI_DISK_F_MONITOR_NOT_REMOVABLE feature. By this
feature we can set that the scsi-block (scsi pass-through) device will still
be removable from the guest side, but from monitor it cannot be removed.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hw/scsi/scsi-disk.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index c8d2a99..190c3ad 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -61,8 +61,9 @@ typedef struct SCSIDiskReq {
     BlockAcctCookie acct;
 } SCSIDiskReq;
 
-#define SCSI_DISK_F_REMOVABLE   0
-#define SCSI_DISK_F_DPOFUA      1
+#define SCSI_DISK_F_REMOVABLE               0
+#define SCSI_DISK_F_DPOFUA                  1
+#define SCSI_DISK_F_MONITOR_NOT_REMOVABLE   2
 
 struct SCSIDiskState
 {
@@ -2107,7 +2108,8 @@ static int scsi_initfn(SCSIDevice *dev)
         return -1;
     }
 
-    if (s->features & (1 << SCSI_DISK_F_REMOVABLE)) {
+    if ((s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
+            !(s->features & (1 << SCSI_DISK_F_MONITOR_NOT_REMOVABLE))) {
         bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_disk_removable_block_ops, s);
     } else {
         bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_disk_block_ops, s);
@@ -2463,6 +2465,8 @@ static const TypeInfo scsi_cd_info = {
 static Property scsi_block_properties[] = {
     DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.bs),
     DEFINE_PROP_INT32("bootindex", SCSIDiskState, qdev.conf.bootindex, -1),
+    DEFINE_PROP_BIT("monitor_not_removable", SCSIDiskState, features,
+                    SCSI_DISK_F_MONITOR_NOT_REMOVABLE, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/2] scsi-generic: check the return value of bdrv_aio_ioctl in execute_command

[Paolo Bonzini]

Il 29/05/2013 14:12, Pavel Hrdina ha scritto:
> This fixes the bug introduced by this commit ad54ae80c73f.
> The bdrv_aio_ioctl() still could return null and we should return an error
> in that case.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hw/scsi/scsi-generic.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 2a9a561..8a4dae2 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -174,6 +174,10 @@ static int execute_command(BlockDriverState *bdrv,
>      r->io_header.flags |= SG_FLAG_DIRECT_IO;
>  
>      r->req.aiocb = bdrv_aio_ioctl(bdrv, SG_IO, &r->io_header, complete, r);
> +    if (r->req.aiocb == NULL) {
> +        BADF("execute_command: read failed !\n");
> +        return -EIO;
> +    }
>  
>      return 0;
>  }
> 

Applied to scsi branch with the BADF removed.  Thanks.

Paolo

Re: [Qemu-devel] [PATCH 2/2] scsi-disk: scsi-block device for scsi pass-through should not be removable

[Paolo Bonzini]

Il 29/05/2013 14:12, Pavel Hrdina ha scritto:
> This patch adds a new SCSI_DISK_F_MONITOR_NOT_REMOVABLE feature. By this
> feature we can set that the scsi-block (scsi pass-through) device will still
> be removable from the guest side, but from monitor it cannot be removed.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hw/scsi/scsi-disk.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index c8d2a99..190c3ad 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -61,8 +61,9 @@ typedef struct SCSIDiskReq {
>      BlockAcctCookie acct;
>  } SCSIDiskReq;
>  
> -#define SCSI_DISK_F_REMOVABLE   0
> -#define SCSI_DISK_F_DPOFUA      1
> +#define SCSI_DISK_F_REMOVABLE               0
> +#define SCSI_DISK_F_DPOFUA                  1
> +#define SCSI_DISK_F_MONITOR_NOT_REMOVABLE   2
>  
>  struct SCSIDiskState
>  {
> @@ -2107,7 +2108,8 @@ static int scsi_initfn(SCSIDevice *dev)
>          return -1;
>      }
>  
> -    if (s->features & (1 << SCSI_DISK_F_REMOVABLE)) {
> +    if ((s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
> +            !(s->features & (1 << SCSI_DISK_F_MONITOR_NOT_REMOVABLE))) {
>          bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_disk_removable_block_ops, s);
>      } else {
>          bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_disk_block_ops, s);
> @@ -2463,6 +2465,8 @@ static const TypeInfo scsi_cd_info = {
>  static Property scsi_block_properties[] = {
>      DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.bs),
>      DEFINE_PROP_INT32("bootindex", SCSIDiskState, qdev.conf.bootindex, -1),
> +    DEFINE_PROP_BIT("monitor_not_removable", SCSIDiskState, features,
> +                    SCSI_DISK_F_MONITOR_NOT_REMOVABLE, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> 

I think the right fix is simply to remove

    if (buf[1] & 0x80) {
        s->features |= 1 << SCSI_DISK_F_REMOVABLE;
    }

from get_device_type.

Paolo

Re: [Qemu-devel] [PATCH 2/2] scsi-disk: scsi-block device for scsi pass-through should not be removable

[Pavel Hrdina]

On 29.5.2013 14:24, Paolo Bonzini wrote:
> Il 29/05/2013 14:12, Pavel Hrdina ha scritto:
>> This patch adds a new SCSI_DISK_F_MONITOR_NOT_REMOVABLE feature. By this
>> feature we can set that the scsi-block (scsi pass-through) device will still
>> be removable from the guest side, but from monitor it cannot be removed.
>>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>   hw/scsi/scsi-disk.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index c8d2a99..190c3ad 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -61,8 +61,9 @@ typedef struct SCSIDiskReq {
>>       BlockAcctCookie acct;
>>   } SCSIDiskReq;
>>
>> -#define SCSI_DISK_F_REMOVABLE   0
>> -#define SCSI_DISK_F_DPOFUA      1
>> +#define SCSI_DISK_F_REMOVABLE               0
>> +#define SCSI_DISK_F_DPOFUA                  1
>> +#define SCSI_DISK_F_MONITOR_NOT_REMOVABLE   2
>>
>>   struct SCSIDiskState
>>   {
>> @@ -2107,7 +2108,8 @@ static int scsi_initfn(SCSIDevice *dev)
>>           return -1;
>>       }
>>
>> -    if (s->features & (1 << SCSI_DISK_F_REMOVABLE)) {
>> +    if ((s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
>> +            !(s->features & (1 << SCSI_DISK_F_MONITOR_NOT_REMOVABLE))) {
>>           bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_disk_removable_block_ops, s);
>>       } else {
>>           bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_disk_block_ops, s);
>> @@ -2463,6 +2465,8 @@ static const TypeInfo scsi_cd_info = {
>>   static Property scsi_block_properties[] = {
>>       DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.bs),
>>       DEFINE_PROP_INT32("bootindex", SCSIDiskState, qdev.conf.bootindex, -1),
>> +    DEFINE_PROP_BIT("monitor_not_removable", SCSIDiskState, features,
>> +                    SCSI_DISK_F_MONITOR_NOT_REMOVABLE, true),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>
>>
>
> I think the right fix is simply to remove
>
>      if (buf[1] & 0x80) {
>          s->features |= 1 << SCSI_DISK_F_REMOVABLE;
>      }
>
> from get_device_type.
>
> Paolo
>

That was my first approach, but without this option it fails to start 
the guest if there was no media in the real CD-ROM.

These are the errors:
qemu-system-x86_64: -device 
scsi-block,drive=drive-cd-disk,bus=scsi0.0,id=scsi_cd: Device needs 
media, but drive is empty
qemu-system-x86_64: -device 
scsi-block,drive=drive-cd-disk,bus=scsi0.0,id=scsi_cd: Device 
initialization failed.
qemu-system-x86_64: -device 
scsi-block,drive=drive-cd-disk,bus=scsi0.0,id=scsi_cd: Device 
'scsi-block' could not be initialized

Pavel

Re: [Qemu-devel] [PATCH 2/2] scsi-disk: scsi-block device for scsi pass-through should not be removable

[Paolo Bonzini]

Il 29/05/2013 14:33, Pavel Hrdina ha scritto:
> On 29.5.2013 14:24, Paolo Bonzini wrote:
>> Il 29/05/2013 14:12, Pavel Hrdina ha scritto:
>>> This patch adds a new SCSI_DISK_F_MONITOR_NOT_REMOVABLE feature. By this
>>> feature we can set that the scsi-block (scsi pass-through) device
>>> will still
>>> be removable from the guest side, but from monitor it cannot be removed.
>>>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> ---
>>>   hw/scsi/scsi-disk.c | 10 +++++++---
>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>>> index c8d2a99..190c3ad 100644
>>> --- a/hw/scsi/scsi-disk.c
>>> +++ b/hw/scsi/scsi-disk.c
>>> @@ -61,8 +61,9 @@ typedef struct SCSIDiskReq {
>>>       BlockAcctCookie acct;
>>>   } SCSIDiskReq;
>>>
>>> -#define SCSI_DISK_F_REMOVABLE   0
>>> -#define SCSI_DISK_F_DPOFUA      1
>>> +#define SCSI_DISK_F_REMOVABLE               0
>>> +#define SCSI_DISK_F_DPOFUA                  1
>>> +#define SCSI_DISK_F_MONITOR_NOT_REMOVABLE   2
>>>
>>>   struct SCSIDiskState
>>>   {
>>> @@ -2107,7 +2108,8 @@ static int scsi_initfn(SCSIDevice *dev)
>>>           return -1;
>>>       }
>>>
>>> -    if (s->features & (1 << SCSI_DISK_F_REMOVABLE)) {
>>> +    if ((s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
>>> +            !(s->features & (1 <<
>>> SCSI_DISK_F_MONITOR_NOT_REMOVABLE))) {
>>>           bdrv_set_dev_ops(s->qdev.conf.bs,
>>> &scsi_disk_removable_block_ops, s);
>>>       } else {
>>>           bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_disk_block_ops, s);
>>> @@ -2463,6 +2465,8 @@ static const TypeInfo scsi_cd_info = {
>>>   static Property scsi_block_properties[] = {
>>>       DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.bs),
>>>       DEFINE_PROP_INT32("bootindex", SCSIDiskState,
>>> qdev.conf.bootindex, -1),
>>> +    DEFINE_PROP_BIT("monitor_not_removable", SCSIDiskState, features,
>>> +                    SCSI_DISK_F_MONITOR_NOT_REMOVABLE, true),
>>>       DEFINE_PROP_END_OF_LIST(),
>>>   };
>>>
>>>
>>
>> I think the right fix is simply to remove
>>
>>      if (buf[1] & 0x80) {
>>          s->features |= 1 << SCSI_DISK_F_REMOVABLE;
>>      }
>>
>> from get_device_type.
>>
>> Paolo
>>
> 
> That was my first approach, but without this option it fails to start
> the guest if there was no media in the real CD-ROM.
> 
> These are the errors:
> qemu-system-x86_64: -device
> scsi-block,drive=drive-cd-disk,bus=scsi0.0,id=scsi_cd: Device needs
> media, but drive is empty
> qemu-system-x86_64: -device
> scsi-block,drive=drive-cd-disk,bus=scsi0.0,id=scsi_cd: Device
> initialization failed.
> qemu-system-x86_64: -device
> scsi-block,drive=drive-cd-disk,bus=scsi0.0,id=scsi_cd: Device
> 'scsi-block' could not be initialized

Right.  This test does not make sense for passthrough.  So if we add
another feature bit (e.g. SCSI_DISK_F_PASSTHROUGH) we can simply skip it.

Alternatively, the optimal test in the SCSI_DISK_F_PASSTHROUGH case
would be something like:

bool bdrv_is_opened(BlockDriverState *bs)
{
    return bs->drv != NULL;
}

Kevin, would you be okay with adding such a function?

Paolo

Re: [Qemu-devel] [PATCH 2/2] scsi-disk: scsi-block device for scsi pass-through should not be removable

[Paolo Bonzini]

Il 29/05/2013 14:12, Pavel Hrdina ha scritto:
> This patch adds a new SCSI_DISK_F_MONITOR_NOT_REMOVABLE feature. By this
> feature we can set that the scsi-block (scsi pass-through) device will still
> be removable from the guest side, but from monitor it cannot be removed.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hw/scsi/scsi-disk.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index c8d2a99..190c3ad 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -61,8 +61,9 @@ typedef struct SCSIDiskReq {
>      BlockAcctCookie acct;
>  } SCSIDiskReq;
>  
> -#define SCSI_DISK_F_REMOVABLE   0
> -#define SCSI_DISK_F_DPOFUA      1
> +#define SCSI_DISK_F_REMOVABLE               0
> +#define SCSI_DISK_F_DPOFUA                  1
> +#define SCSI_DISK_F_MONITOR_NOT_REMOVABLE   2
>  
>  struct SCSIDiskState
>  {
> @@ -2107,7 +2108,8 @@ static int scsi_initfn(SCSIDevice *dev)
>          return -1;
>      }
>  
> -    if (s->features & (1 << SCSI_DISK_F_REMOVABLE)) {
> +    if ((s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
> +            !(s->features & (1 << SCSI_DISK_F_MONITOR_NOT_REMOVABLE))) {
>          bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_disk_removable_block_ops, s);
>      } else {
>          bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_disk_block_ops, s);
> @@ -2463,6 +2465,8 @@ static const TypeInfo scsi_cd_info = {
>  static Property scsi_block_properties[] = {
>      DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.bs),
>      DEFINE_PROP_INT32("bootindex", SCSIDiskState, qdev.conf.bootindex, -1),
> +    DEFINE_PROP_BIT("monitor_not_removable", SCSIDiskState, features,
> +                    SCSI_DISK_F_MONITOR_NOT_REMOVABLE, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> 

Kevin convinced me offlist to take this patch, but I'll still ask for
two changes:

1) rename the feature bit to SCSI_DISK_F_NO_REMOVABLE_DEVOPS

2) set the feature in scsi_block_initfn instead of using a bit, because
the user should not be able to toggle this.

Thanks!

Paolo

Re: [Qemu-devel] [PATCH 2/2] scsi-disk: scsi-block device for scsi pass-through should not be removable

[Pavel Hrdina]

On 29.5.2013 15:30, Paolo Bonzini wrote:
> Il 29/05/2013 14:12, Pavel Hrdina ha scritto:
>> This patch adds a new SCSI_DISK_F_MONITOR_NOT_REMOVABLE feature. By this
>> feature we can set that the scsi-block (scsi pass-through) device will still
>> be removable from the guest side, but from monitor it cannot be removed.
>>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>   hw/scsi/scsi-disk.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index c8d2a99..190c3ad 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -61,8 +61,9 @@ typedef struct SCSIDiskReq {
>>       BlockAcctCookie acct;
>>   } SCSIDiskReq;
>>
>> -#define SCSI_DISK_F_REMOVABLE   0
>> -#define SCSI_DISK_F_DPOFUA      1
>> +#define SCSI_DISK_F_REMOVABLE               0
>> +#define SCSI_DISK_F_DPOFUA                  1
>> +#define SCSI_DISK_F_MONITOR_NOT_REMOVABLE   2
>>
>>   struct SCSIDiskState
>>   {
>> @@ -2107,7 +2108,8 @@ static int scsi_initfn(SCSIDevice *dev)
>>           return -1;
>>       }
>>
>> -    if (s->features & (1 << SCSI_DISK_F_REMOVABLE)) {
>> +    if ((s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
>> +            !(s->features & (1 << SCSI_DISK_F_MONITOR_NOT_REMOVABLE))) {
>>           bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_disk_removable_block_ops, s);
>>       } else {
>>           bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_disk_block_ops, s);
>> @@ -2463,6 +2465,8 @@ static const TypeInfo scsi_cd_info = {
>>   static Property scsi_block_properties[] = {
>>       DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.bs),
>>       DEFINE_PROP_INT32("bootindex", SCSIDiskState, qdev.conf.bootindex, -1),
>> +    DEFINE_PROP_BIT("monitor_not_removable", SCSIDiskState, features,
>> +                    SCSI_DISK_F_MONITOR_NOT_REMOVABLE, true),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>
>>
>
> Kevin convinced me offlist to take this patch, but I'll still ask for
> two changes:
>
> 1) rename the feature bit to SCSI_DISK_F_NO_REMOVABLE_DEVOPS
>
> 2) set the feature in scsi_block_initfn instead of using a bit, because
> the user should not be able to toggle this.
>
> Thanks!
>
> Paolo
>

I've red that list and OK, I'll update that patch.

Pavel