| Message ID | 20180321105830.22412-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: turn "is this a SCSI device?" into a conditional hint | expand |
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
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
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>
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
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) {
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(-)