From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41009) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XhCqB-0004JT-I2 for qemu-devel@nongnu.org; Thu, 23 Oct 2014 03:29:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XhCq5-0000by-0A for qemu-devel@nongnu.org; Thu, 23 Oct 2014 03:29:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52891) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XhCq4-0000bm-Od for qemu-devel@nongnu.org; Thu, 23 Oct 2014 03:29:40 -0400 Message-ID: <5448AE60.5060608@redhat.com> Date: Thu, 23 Oct 2014 09:29:36 +0200 From: Max Reitz MIME-Version: 1.0 References: <1413993434-11816-1-git-send-email-mreitz@redhat.com> <1413993434-11816-3-git-send-email-mreitz@redhat.com> <5447E2BB.7070508@redhat.com> In-Reply-To: <5447E2BB.7070508@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 2/3] raw-posix: raw_co_get_block_status() return value List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , =?UTF-8?B?QmVub8OudCBDYW5ldA==?= , Stefan Hajnoczi On 2014-10-22 at 19:00, Eric Blake wrote: > On 10/22/2014 09:57 AM, Max Reitz wrote: >> Instead of generating the full return value thrice in try_fiemap(), >> try_seek_hole() and as a fall-back in raw_co_get_block_status() itself, >> generate the value only in raw_co_get_block_status(). >> >> While at it, also remove the pnum parameter from try_fiemap() and >> try_seek_hole(). >> >> Suggested-by: Kevin Wolf >> Signed-off-by: Max Reitz >> --- >> block/raw-posix.c | 28 ++++++++++++++-------------- >> 1 file changed, 14 insertions(+), 14 deletions(-) > Reviewed-by: Eric Blake > >> - ret = try_seek_hole(bs, start, &data, &hole, pnum); >> + ret = try_seek_hole(bs, start, &data, &hole); >> if (ret < 0) { >> - ret = try_fiemap(bs, start, &data, &hole, nb_sectors, pnum); >> + ret = try_fiemap(bs, start, &data, &hole, nb_sectors); >> if (ret < 0) { >> /* Assume everything is allocated. */ >> data = 0; >> hole = start + nb_sectors * BDRV_SECTOR_SIZE; >> - ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; >> + ret = 0; >> } >> } >> >> + assert(ret >= 0); > I'm not sure the assertion adds much (the lines above are fairly easy to > reason about ret always being set), but it does protect against later > code addition, so I'm not opposed to leaving it. I just liked it there to make clear that the previous code always goes into a fallback, so while some parts of it may throw errors, after everything is done, we're fine. The compiler will hopefully optimize it out anyway. Max