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

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

[Paolo Bonzini]

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

[Laurent Vivier]

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

[Paolo Bonzini]

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

[Eric Blake]

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

[Fam Zheng]

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

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

[Laurent Vivier]

On 21/03/2018 13:54, 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>
> Reviewed-by: Eric Blake <eblake@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(-)

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

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

[Paolo Bonzini]

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>
Reviewed-by: Eric Blake <eblake@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..3e29063a3f 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_errno(errp, -rc, "cannot get SG_IO version number");
+        if (rc != -EPERM) {
+            error_append_hint(errp, "Is this a SCSI device?\n");
+        }
         return;
     }
     if (sg_version < 30000) {
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 7414fe2d67..4753f8738f 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_errno(errp, -rc, "cannot get SG_IO version number");
+        if (rc != -EPERM) {
+            error_append_hint(errp, "Is this a SCSI device?\n");
+        }
         return;
     }
     if (sg_version < 30000) {
-- 
2.16.2