Patchwork [v2,03/13] raw-posix: Fix test for host CD-ROM

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

Comments

Markus Armbruster - July 6, 2010, 12:08 p.m.
raw_pread_aligned() retries up to two times if the block device backs
a virtual CD-ROM (a drive with media=cdrom and if=ide, scsi, xen or
none).  This makes no sense.  Whether retrying reads can correct read
errors can only depend on what we're reading, not on how the result
gets used.  We need to check what whether we're reading from a
physical CD-ROM or floppy here.

I doubt retrying is useful even then.  Left for another day.

Impact:

* Virtual CD-ROM backed by host_cdrom behaves the same.

* Virtual CD-ROM backed by file or host_device no longer retries.

* A drive backed by host_cdrom now retries even if it's not a virtual
  CD-ROM.

* Any drive backed by host_floppy now retries.

While there, clean up gratuitous use of goto.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/raw-posix.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)
Christoph Hellwig - July 7, 2010, 1:23 a.m.
On Tue, Jul 06, 2010 at 02:08:46PM +0200, Markus Armbruster wrote:
> * Any drive backed by host_floppy now retries.

I would really prefer not to change the behaviour for this case, it'll
just confuse people looking at the history when finally removing this
hack.
Markus Armbruster - July 7, 2010, 9:34 a.m.
Christoph Hellwig <hch@lst.de> writes:

> On Tue, Jul 06, 2010 at 02:08:46PM +0200, Markus Armbruster wrote:
>> * Any drive backed by host_floppy now retries.
>
> I would really prefer not to change the behaviour for this case, it'll
> just confuse people looking at the history when finally removing this
> hack.

I'm fine either way.  Kevin?
Kevin Wolf - July 7, 2010, 10:06 a.m.
Am 07.07.2010 11:34, schrieb Markus Armbruster:
> Christoph Hellwig <hch@lst.de> writes:
> 
>> On Tue, Jul 06, 2010 at 02:08:46PM +0200, Markus Armbruster wrote:
>>> * Any drive backed by host_floppy now retries.
>>
>> I would really prefer not to change the behaviour for this case, it'll
>> just confuse people looking at the history when finally removing this
>> hack.
> 
> I'm fine either way.  Kevin?

I don't really care about floppies, and I doubt anyone does. Retaining
old behaviour should never be wrong, so if you prefer, I'm okay with it.

Kevin

Patch

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 3f0701b..291699f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -242,15 +242,14 @@  static int raw_pread_aligned(BlockDriverState *bs, int64_t offset,
 
     ret = pread(s->fd, buf, count, offset);
     if (ret == count)
-        goto label__raw_read__success;
+        return ret;
 
     /* Allow reads beyond the end (needed for pwrite) */
     if ((ret == 0) && bs->growable) {
         int64_t size = raw_getlength(bs);
         if (offset >= size) {
             memset(buf, 0, count);
-            ret = count;
-            goto label__raw_read__success;
+            return count;
         }
     }
 
@@ -260,13 +259,13 @@  static int raw_pread_aligned(BlockDriverState *bs, int64_t offset,
                       bs->total_sectors, ret, errno, strerror(errno));
 
     /* Try harder for CDrom. */
-    if (bs->type == BDRV_TYPE_CDROM) {
+    if (s->type != FTYPE_FILE) {
         ret = pread(s->fd, buf, count, offset);
         if (ret == count)
-            goto label__raw_read__success;
+            return ret;
         ret = pread(s->fd, buf, count, offset);
         if (ret == count)
-            goto label__raw_read__success;
+            return ret;
 
         DEBUG_BLOCK_PRINT("raw_pread(%d:%s, %" PRId64 ", %p, %d) [%" PRId64
                           "] retry read failed %d : %d = %s\n",
@@ -274,8 +273,6 @@  static int raw_pread_aligned(BlockDriverState *bs, int64_t offset,
                           bs->total_sectors, ret, errno, strerror(errno));
     }
 
-label__raw_read__success:
-
     return  (ret < 0) ? -errno : ret;
 }
 
@@ -298,15 +295,13 @@  static int raw_pwrite_aligned(BlockDriverState *bs, int64_t offset,
 
     ret = pwrite(s->fd, buf, count, offset);
     if (ret == count)
-        goto label__raw_write__success;
+        return ret;
 
     DEBUG_BLOCK_PRINT("raw_pwrite(%d:%s, %" PRId64 ", %p, %d) [%" PRId64
                       "] write failed %d : %d = %s\n",
                       s->fd, bs->filename, offset, buf, count,
                       bs->total_sectors, ret, errno, strerror(errno));
 
-label__raw_write__success:
-
     return  (ret < 0) ? -errno : ret;
 }