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

Submitted by Markus Armbruster on July 6, 2010, 12:08 p.m.

Details

Message ID 1278418136-24556-7-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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) {