Patchwork [v2,06/13] scsi: Reject unimplemented error actions

login
register
mail settings
Submitter Markus Armbruster
Date July 6, 2010, 12:08 p.m.
Message ID <1278418136-24556-7-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/58005/
State New
Headers show

Comments

Markus Armbruster - July 6, 2010, 12:08 p.m.
drive_init() doesn't permit rerror for if=scsi, but that's worthless:
we get it via if=none and -device.

Moreover, scsi-generic doesn't support werror.  Since drive_init()
doesn't catch that, option werror was silently ignored even with
if=scsi.

Wart: unlike drive_init(), we don't reject the default action when
it's explicitly specified.  That's because we can't distinguish "no
rerror option" from "rerror=report", or "no werror" from
"rerror=enospc".  Left for another day.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/scsi-disk.c    |    5 +++++
 hw/scsi-generic.c |    9 +++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)
Christoph Hellwig - July 7, 2010, 1:26 a.m.
On Tue, Jul 06, 2010 at 02:08:49PM +0200, Markus Armbruster wrote:
> drive_init() doesn't permit rerror for if=scsi, but that's worthless:
> we get it via if=none and -device.
> 
> Moreover, scsi-generic doesn't support werror.  Since drive_init()
> doesn't catch that, option werror was silently ignored even with
> if=scsi.
> 
> Wart: unlike drive_init(), we don't reject the default action when
> it's explicitly specified.  That's because we can't distinguish "no
> rerror option" from "rerror=report", or "no werror" from
> "rerror=enospc".  Left for another day.

I can't see a good reason that scsi doesn't support the rerror option,
and implementing is trivial.  So while this patch looks correct I'd
rather see rerror implemented for scsi than hacking around the lack of
it.
Markus Armbruster - July 7, 2010, 9:41 a.m.
Christoph Hellwig <hch@lst.de> writes:

> On Tue, Jul 06, 2010 at 02:08:49PM +0200, Markus Armbruster wrote:
>> drive_init() doesn't permit rerror for if=scsi, but that's worthless:
>> we get it via if=none and -device.
>> 
>> Moreover, scsi-generic doesn't support werror.  Since drive_init()
>> doesn't catch that, option werror was silently ignored even with
>> if=scsi.
>> 
>> Wart: unlike drive_init(), we don't reject the default action when
>> it's explicitly specified.  That's because we can't distinguish "no
>> rerror option" from "rerror=report", or "no werror" from
>> "rerror=enospc".  Left for another day.
>
> I can't see a good reason that scsi doesn't support the rerror option,
> and implementing is trivial.  So while this patch looks correct I'd
> rather see rerror implemented for scsi than hacking around the lack of
> it.

You got a point there.  Same for fdc.

However, this is the best *I* can do in time for .13.  Let's add the
missin error action support after the release, okay?

Patch

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 3e41011..c30709c 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1059,6 +1059,11 @@  static int scsi_disk_initfn(SCSIDevice *dev)
     s->bs = s->qdev.conf.bs;
     is_cd = bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM;
 
+    if (bdrv_get_on_error(s->bs, 1) != BLOCK_ERR_REPORT) {
+        error_report("Device doesn't support drive option rerror");
+        return -1;
+    }
+
     if (!s->serial) {
         /* try to fall back to value set with legacy -drive serial=... */
         dinfo = drive_get_by_blockdev(s->bs);
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 3915e78..a8b4176 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -474,6 +474,15 @@  static int scsi_generic_initfn(SCSIDevice *dev)
         return -1;
     }
 
+    if (bdrv_get_on_error(s->bs, 0) != BLOCK_ERR_STOP_ENOSPC) {
+        error_report("Device doesn't support drive option werror");
+        return -1;
+    }
+    if (bdrv_get_on_error(s->bs, 1) != BLOCK_ERR_REPORT) {
+        error_report("Device doesn't support drive option rerror");
+        return -1;
+    }
+
     /* check we are using a driver managing SG_IO (version 3 and after */
     if (bdrv_ioctl(s->bs, SG_GET_VERSION_NUM, &sg_version) < 0 ||
         sg_version < 30000) {