From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38068) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xot4m-0001E4-G1 for qemu-devel@nongnu.org; Thu, 13 Nov 2014 07:00:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xot4g-00047G-1N for qemu-devel@nongnu.org; Thu, 13 Nov 2014 07:00:36 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37918) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xot4f-00046s-LE for qemu-devel@nongnu.org; Thu, 13 Nov 2014 07:00:29 -0500 Date: Thu, 13 Nov 2014 13:00:22 +0100 From: Kevin Wolf Message-ID: <20141113120022.GC3933@noname.redhat.com> References: <1415820422-17796-1-git-send-email-armbru@redhat.com> <1415820422-17796-3-git-send-email-armbru@redhat.com> <5463EC70.1030107@redhat.com> <20141113114030.GA3933@noname.redhat.com> <546499F6.4050201@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <546499F6.4050201@redhat.com> Subject: Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, Markus Armbruster , tony@bakeyournoodle.com, stefanha@redhat.com, pbonzini@redhat.com 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