1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] scsi: turn "is this a SCSI device?" into a conditional hint

Paolo Bonzini posted 1 patch 7 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180321105830.22412-1-pbonzini@redhat.com
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test docker-quick@centos6 failed
Test s390x passed
hw/scsi/scsi-disk.c    | 7 ++++---
hw/scsi/scsi-generic.c | 7 ++++---
2 files changed, 8 insertions(+), 6 deletions(-)
[Qemu-devel] [PATCH] scsi: turn "is this a SCSI device?" into a conditional hint
Posted by Paolo Bonzini 7 years, 7 months ago 
If the user does not have permissions to send ioctls to the device (due to
SELinux or cgroups, for example), the output can look like

qemu-kvm: -device scsi-block,drive=disk: cannot get SG_IO version number:
  Operation not permitted.  Is this a SCSI device?

but this is confusing because the ioctl was blocked _before_ the device
even received the SG_GET_VERSION_NUM ioctl.  Therefore, for EPERM errors
the suggestion should be eliminated.  To make that simpler, change the
code to use error_append_hint.

Reported-by: Ala Hino <ahino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c    | 7 ++++---
 hw/scsi/scsi-generic.c | 7 ++++---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 94043ed024..ccc245589a 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2637,9 +2637,10 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
     /* check we are using a driver managing SG_IO (version 3 and after) */
     rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
     if (rc < 0) {
-        error_setg(errp, "cannot get SG_IO version number: %s.  "
-                     "Is this a SCSI device?",
-                     strerror(-rc));
+        error_setg(errp, "cannot get SG_IO version number: %s", strerror(-rc));
+        if (rc != -EPERM) {
+            error_append_hint(errp, "Is this a SCSI device?");
+        }
         return;
     }
     if (sg_version < 30000) {
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 7414fe2d67..b3de5df324 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -500,9 +500,10 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
     /* check we are using a driver managing SG_IO (version 3 and after */
     rc = blk_ioctl(s->conf.blk, SG_GET_VERSION_NUM, &sg_version);
     if (rc < 0) {
-        error_setg(errp, "cannot get SG_IO version number: %s.  "
-                         "Is this a SCSI device?",
-                         strerror(-rc));
+        error_setg(errp, "cannot get SG_IO version number: %s", strerror(-rc));
+        if (rc != -EPERM) {
+            error_append_hint(errp, "Is this a SCSI device?");
+        }
         return;
     }
     if (sg_version < 30000) {
-- 
2.16.2
Re: [Qemu-devel] [PATCH] scsi: turn "is this a SCSI device?" into a conditional hint
Posted by Laurent Vivier 7 years, 7 months ago 
On 21/03/2018 11:58, Paolo Bonzini wrote:
> If the user does not have permissions to send ioctls to the device (due to
> SELinux or cgroups, for example), the output can look like
> 
> qemu-kvm: -device scsi-block,drive=disk: cannot get SG_IO version number:
>   Operation not permitted.  Is this a SCSI device?
> 
> but this is confusing because the ioctl was blocked _before_ the device
> even received the SG_GET_VERSION_NUM ioctl.  Therefore, for EPERM errors
> the suggestion should be eliminated.  To make that simpler, change the
> code to use error_append_hint.
> 
> Reported-by: Ala Hino <ahino@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi/scsi-disk.c    | 7 ++++---
>  hw/scsi/scsi-generic.c | 7 ++++---
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 94043ed024..ccc245589a 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2637,9 +2637,10 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>      /* check we are using a driver managing SG_IO (version 3 and after) */
>      rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
>      if (rc < 0) {
> -        error_setg(errp, "cannot get SG_IO version number: %s.  "
> -                     "Is this a SCSI device?",
> -                     strerror(-rc));
> +        error_setg(errp, "cannot get SG_IO version number: %s", strerror(-rc));


You could use:

  error_setg_errno(errp, -rc, "cannot get SG_IO version number");

Thanks,
Laurent
Re: [Qemu-devel] [PATCH] scsi: turn "is this a SCSI device?" into a conditional hint
Posted by Paolo Bonzini 7 years, 7 months ago 
On 21/03/2018 13:17, Laurent Vivier wrote:
> On 21/03/2018 11:58, Paolo Bonzini wrote:
>> If the user does not have permissions to send ioctls to the device (due to
>> SELinux or cgroups, for example), the output can look like
>>
>> qemu-kvm: -device scsi-block,drive=disk: cannot get SG_IO version number:
>>   Operation not permitted.  Is this a SCSI device?
>>
>> but this is confusing because the ioctl was blocked _before_ the device
>> even received the SG_GET_VERSION_NUM ioctl.  Therefore, for EPERM errors
>> the suggestion should be eliminated.  To make that simpler, change the
>> code to use error_append_hint.
>>
>> Reported-by: Ala Hino <ahino@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/scsi/scsi-disk.c    | 7 ++++---
>>  hw/scsi/scsi-generic.c | 7 ++++---
>>  2 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index 94043ed024..ccc245589a 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -2637,9 +2637,10 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>>      /* check we are using a driver managing SG_IO (version 3 and after) */
>>      rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
>>      if (rc < 0) {
>> -        error_setg(errp, "cannot get SG_IO version number: %s.  "
>> -                     "Is this a SCSI device?",
>> -                     strerror(-rc));
>> +        error_setg(errp, "cannot get SG_IO version number: %s", strerror(-rc));
> 
> 
> You could use:
> 
>   error_setg_errno(errp, -rc, "cannot get SG_IO version number");

Nice, thanks.  Will do.

Paolo
Re: [Qemu-devel] [PATCH] scsi: turn "is this a SCSI device?" into a conditional hint
Posted by Eric Blake 7 years, 7 months ago 
On 03/21/2018 05:58 AM, Paolo Bonzini wrote:
> If the user does not have permissions to send ioctls to the device (due to
> SELinux or cgroups, for example), the output can look like
> 
> qemu-kvm: -device scsi-block,drive=disk: cannot get SG_IO version number:
>    Operation not permitted.  Is this a SCSI device?
> 
> but this is confusing because the ioctl was blocked _before_ the device
> even received the SG_GET_VERSION_NUM ioctl.  Therefore, for EPERM errors
> the suggestion should be eliminated.  To make that simpler, change the
> code to use error_append_hint.
> 
> Reported-by: Ala Hino <ahino@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/scsi/scsi-disk.c    | 7 ++++---
>   hw/scsi/scsi-generic.c | 7 ++++---
>   2 files changed, 8 insertions(+), 6 deletions(-)
> 

> +++ b/hw/scsi/scsi-disk.c
> @@ -2637,9 +2637,10 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>       /* check we are using a driver managing SG_IO (version 3 and after) */
>       rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
>       if (rc < 0) {
> -        error_setg(errp, "cannot get SG_IO version number: %s.  "
> -                     "Is this a SCSI device?",
> -                     strerror(-rc));
> +        error_setg(errp, "cannot get SG_IO version number: %s", strerror(-rc));
> +        if (rc != -EPERM) {
> +            error_append_hint(errp, "Is this a SCSI device?");

Missing the \n (error_append_hint does NOT automatically add one, 
because sometimes hints are pieced together but should still display in 
one line).

> +++ b/hw/scsi/scsi-generic.c
> @@ -500,9 +500,10 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
>       /* check we are using a driver managing SG_IO (version 3 and after */
>       rc = blk_ioctl(s->conf.blk, SG_GET_VERSION_NUM, &sg_version);
>       if (rc < 0) {
> -        error_setg(errp, "cannot get SG_IO version number: %s.  "
> -                         "Is this a SCSI device?",
> -                         strerror(-rc));
> +        error_setg(errp, "cannot get SG_IO version number: %s", strerror(-rc));
> +        if (rc != -EPERM) {
> +            error_append_hint(errp, "Is this a SCSI device?");

And again.  With that fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] scsi: turn "is this a SCSI device?" into a conditional hint
Posted by Fam Zheng 7 years, 7 months ago 
On Wed, 03/21 11:58, Paolo Bonzini wrote:
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 7414fe2d67..b3de5df324 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -500,9 +500,10 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
>      /* check we are using a driver managing SG_IO (version 3 and after */
>      rc = blk_ioctl(s->conf.blk, SG_GET_VERSION_NUM, &sg_version);
>      if (rc < 0) {
> -        error_setg(errp, "cannot get SG_IO version number: %s.  "
> -                         "Is this a SCSI device?",
> -                         strerror(-rc));
> +        error_setg(errp, "cannot get SG_IO version number: %s", strerror(-rc));
> +        if (rc != -EPERM) {
> +            error_append_hint(errp, "Is this a SCSI device?");
> +        }

Out of the scope of this patch but I find it a bit annoying that hints are not
propagated up in QMP. So that if you hot plug from virsh, the user friendliness
of the old message is lost. Not sure if that is a problem or not.

Fam

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.