[Qemu-devel] [PATCH] scsi: Make device scsi-disk reject /dev/sg*

[Qemu-devel] [PATCH] scsi: Make device scsi-disk reject /dev/sg*

[Markus Armbruster]

You're supposed to use scsi-generic for that.  Which rejects anything
but /dev/sg*.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/scsi-disk.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index b2f61fe..ad8eb24 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1027,6 +1027,11 @@ static int scsi_disk_initfn(SCSIDevice *dev)
     }
     s->bs = s->qdev.conf.dinfo->bdrv;
 
+    if (bdrv_is_sg(s->bs)) {
+        qemu_error("scsi-disk: unwanted /dev/sg*\n");
+        return -1;
+    }
+
     if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
         s->cluster_size = 4;
     } else {
-- 
1.6.6

[Qemu-devel] Re: [PATCH] scsi: Make device scsi-disk reject /dev/sg*

[Gerd Hoffmann]

On 02/25/10 11:23, Markus Armbruster wrote:
> You're supposed to use scsi-generic for that.  Which rejects anything
> but /dev/sg*.

Well, it isn't *that* easy.  The SG_IO ioctl used by scsi-generic works 
on tons of devices in linux, not only /dev/sg*.  I've seen patches 
floading around which change the check bdrv_is_sg() into "try SG_IO and 
see if it works", which would allow to use /dev/sda with both scsi-disk 
and scsi-generic depending on what you want.  Which makes alot of sense.

Making that change needs some extra care though to avoid existing 
configurations switching from scsi-disk to scsi-generic unnoticed.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH] scsi: Make device scsi-disk reject /dev/sg*

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On 02/25/10 11:23, Markus Armbruster wrote:
>> You're supposed to use scsi-generic for that.  Which rejects anything
>> but /dev/sg*.
>
> Well, it isn't *that* easy.  The SG_IO ioctl used by scsi-generic
> works on tons of devices in linux, not only /dev/sg*.  I've seen

Yes, the existing check in scsi-generic is pretty cheesy.  My patch just
makes scsi-disk consistently cheesy ;)

> patches floading around which change the check bdrv_is_sg() into "try
> SG_IO and see if it works", which would allow to use /dev/sda with
> both scsi-disk and scsi-generic depending on what you want.  Which
> makes alot of sense.
>
> Making that change needs some extra care though to avoid existing
> configurations switching from scsi-disk to scsi-generic unnoticed.

Would be nice to have some day.

Re: [Qemu-devel] Re: [PATCH] scsi: Make device scsi-disk reject /dev/sg*

[Paul Brook]

> On 02/25/10 11:23, Markus Armbruster wrote:
> > You're supposed to use scsi-generic for that.  Which rejects anything
> > but /dev/sg*.
> 
> Well, it isn't *that* easy.  The SG_IO ioctl used by scsi-generic works
> on tons of devices in linux, not only /dev/sg*.  I've seen patches
> floading around which change the check bdrv_is_sg() into "try SG_IO and
> see if it works", which would allow to use /dev/sda with both scsi-disk
> and scsi-generic depending on what you want.  Which makes alot of sense.
> 
> Making that change needs some extra care though to avoid existing
> configurations switching from scsi-disk to scsi-generic unnoticed.

Don't we really want to be testing !bdrv_is_block() ?

Paul

Re: [Qemu-devel] Re: [PATCH] scsi: Make device scsi-disk reject /dev/sg*

[Gerd Hoffmann]

On 02/28/10 02:45, Paul Brook wrote:
>> On 02/25/10 11:23, Markus Armbruster wrote:
>>> You're supposed to use scsi-generic for that.  Which rejects anything
>>> but /dev/sg*.
>>
>> Well, it isn't *that* easy.  The SG_IO ioctl used by scsi-generic works
>> on tons of devices in linux, not only /dev/sg*.  I've seen patches
>> floading around which change the check bdrv_is_sg() into "try SG_IO and
>> see if it works", which would allow to use /dev/sda with both scsi-disk
>> and scsi-generic depending on what you want.  Which makes alot of sense.
>>
>> Making that change needs some extra care though to avoid existing
>> configurations switching from scsi-disk to scsi-generic unnoticed.
>
> Don't we really want to be testing !bdrv_is_block() ?

That would work for linux.  Dunno about the BSD.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH] scsi: Make device scsi-disk reject /dev/sg*

[Michael S. Tsirkin]

On Thu, Feb 25, 2010 at 11:23:52AM +0100, Markus Armbruster wrote:
> You're supposed to use scsi-generic for that.  Which rejects anything
> but /dev/sg*.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/scsi-disk.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index b2f61fe..ad8eb24 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -1027,6 +1027,11 @@ static int scsi_disk_initfn(SCSIDevice *dev)
>      }
>      s->bs = s->qdev.conf.dinfo->bdrv;
>  
> +    if (bdrv_is_sg(s->bs)) {
> +        qemu_error("scsi-disk: unwanted /dev/sg*\n");
> +        return -1;

Can we make the error message a bit more verbose?
E.g. "scsi-disk does not support /dev/sg*, please use *** instead".
Where *** tells user what to do.

> +    }
> +
>      if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
>          s->cluster_size = 4;
>      } else {
> -- 
> 1.6.6
> 
> 

Re: [Qemu-devel] [PATCH] scsi: Make device scsi-disk reject /dev/sg*

[Anthony Liguori]

On 02/25/2010 04:23 AM, Markus Armbruster wrote:
> You're supposed to use scsi-generic for that.  Which rejects anything
> but /dev/sg*.
>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
> ---
>    

Applied.  Thanks.

Regards,

Anthony Liguori

>   hw/scsi-disk.c |    5 +++++
>   1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index b2f61fe..ad8eb24 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -1027,6 +1027,11 @@ static int scsi_disk_initfn(SCSIDevice *dev)
>       }
>       s->bs = s->qdev.conf.dinfo->bdrv;
>
> +    if (bdrv_is_sg(s->bs)) {
> +        qemu_error("scsi-disk: unwanted /dev/sg*\n");
> +        return -1;
> +    }
> +
>       if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
>           s->cluster_size = 4;
>       } else {
>