Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP

[Kevin Wolf]

Am 13.11.2014 um 12:45 hat Max Reitz geschrieben:
> On 2014-11-13 at 12:40, Kevin Wolf wrote:
> >Am 13.11.2014 um 00:25 hat Eric Blake geschrieben:
> >>On 11/12/2014 01:27 PM, Markus Armbruster wrote:
> >>>+    /* in hole, end not yet known */
> >>>+    offs = lseek(s->fd, start, SEEK_DATA);
> >>>+    if (offs < 0) {
> >>>+        /* no idea where the hole ends, give up (unlikely to happen) */
> >>>+        goto dunno;
> >>>+    }
> >>>+    assert(offs >= start);
> >>>+    *hole = start;
> >>>+    *data = offs;
> >>This assertion feels like an off-by-one.  The same offset cannot be both
> >>a hole and data (except in some racy situation where some other process
> >>is writing data to that offset in between our two lseek calls, but
> >>that's already in no-man's land because no one else should be writing
> >>the file while qemu has it open).  Is it worth using 'assert(offs >
> >>start)' instead?
> >As soon as you say "except", it's wrong to assert this at all. We can't
> >guarantee that the condition is true and it's not a programming error
> >in qemu if it's false. Sounds to me as if it should be a normal error
> >check rather than an assertion.
> >
> >Also, what happens after EOF? I haven't read the patch yet, maybe it
> >handles the situation already earlier, but if it doesn't, won't we get
> >offset == start then?
> 
> raw_co_get_block_status() already bails out if start is at or beyond EOF.

Okay, so that's basically the same "except" as above.

Except that the window for the race is much larger because the
raw_co_get_block_status() check uses the cached value, so any file size
change in the background after qemu has opened the image would trigger
an assertion failure.

Kevin