From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48781) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YG7k0-0006zV-2R for qemu-devel@nongnu.org; Tue, 27 Jan 2015 10:07:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YG7jv-0006fo-Tg for qemu-devel@nongnu.org; Tue, 27 Jan 2015 10:07:44 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46495) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YG7jv-0006bD-Ld for qemu-devel@nongnu.org; Tue, 27 Jan 2015 10:07:39 -0500 Message-ID: <54C7A9B4.6090201@redhat.com> Date: Tue, 27 Jan 2015 10:07:32 -0500 From: Max Reitz MIME-Version: 1.0 References: <1422284444-12529-1-git-send-email-mreitz@redhat.com> <1422284444-12529-8-git-send-email-mreitz@redhat.com> <54C7081D.9030706@redhat.com> In-Reply-To: <54C7081D.9030706@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 07/14] qemu-img: Use BlockBackend as far as possible List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Markus Armbruster , Stefan Hajnoczi , Stefano Stabellini On 2015-01-26 at 22:38, Eric Blake wrote: > On 01/26/2015 08:00 AM, Max Reitz wrote: >> Although qemu-img already creates BlockBackends, it does not do accesses >> to the images through them. This patch converts all of the bdrv_* calls >> for which this is currently possible to blk_* calls. Most of the >> remaining calls will probably stay bdrv_* calls because they really do >> operate on the BDS level instead of the BB level. >> >> Signed-off-by: Max Reitz >> --- >> qemu-img.c | 98 ++++++++++++++++++++++++++++++++++---------------------------- >> 1 file changed, 54 insertions(+), 44 deletions(-) >> >> diff --git a/qemu-img.c b/qemu-img.c >> index 0b23c87..8b4139e 100644 >> --- a/qemu-img.c >> @@ -1130,22 +1130,26 @@ static int img_compare(int argc, char **argv) >> } >> bs2 = blk_bs(blk2); >> >> - buf1 = qemu_blockalign(bs1, IO_BUF_SIZE); >> - buf2 = qemu_blockalign(bs2, IO_BUF_SIZE); >> - total_sectors1 = bdrv_nb_sectors(bs1); >> + buf1 = blk_blockalign(blk1, IO_BUF_SIZE); >> + buf2 = blk_blockalign(blk2, IO_BUF_SIZE); >> + total_sectors1 = blk_getlength(blk1); >> if (total_sectors1 < 0) { >> error_report("Can't get size of %s: %s", >> filename1, strerror(-total_sectors1)); >> ret = 4; >> goto out; >> } >> - total_sectors2 = bdrv_nb_sectors(bs2); >> + total_sectors2 = blk_getlength(blk2); > The naming feels awkward; your conversion is now using bytes while the > old code was using sectors, so 'total_sectors2' feels weird... > >> if (total_sectors2 < 0) { >> error_report("Can't get size of %s: %s", >> filename2, strerror(-total_sectors2)); >> ret = 4; >> goto out; >> } >> + >> + total_sectors1 /= BDRV_SECTOR_SIZE; >> + total_sectors2 /= BDRV_SECTOR_SIZE; > ...at least you end up converting to sectors after all. But it makes me > wonder if you should have blk_nb_sectors(), and/or temporary > intermediate variables to avoid cross-unit confusion. > >> @@ -1476,13 +1480,14 @@ static int img_convert(int argc, char **argv) >> goto out; >> } >> bs[bs_i] = blk_bs(blk[bs_i]); >> - bs_sectors[bs_i] = bdrv_nb_sectors(bs[bs_i]); >> + bs_sectors[bs_i] = blk_getlength(blk[bs_i]); >> if (bs_sectors[bs_i] < 0) { >> error_report("Could not get size of %s: %s", >> argv[optind + bs_i], strerror(-bs_sectors[bs_i])); >> ret = -1; >> goto out; >> } >> + bs_sectors[bs_i] /= BDRV_SECTOR_SIZE; > Another instance of the confusion. > >> total_sectors += bs_sectors[bs_i]; >> } >> >> @@ -1625,16 +1630,19 @@ static int img_convert(int argc, char **argv) >> out_bs->bl.discard_alignment)) >> ); >> >> - buf = qemu_blockalign(out_bs, bufsectors * BDRV_SECTOR_SIZE); >> + buf = blk_blockalign(out_blk, bufsectors * BDRV_SECTOR_SIZE); >> >> if (skip_create) { >> - int64_t output_sectors = bdrv_nb_sectors(out_bs); >> + int64_t output_sectors = blk_getlength(out_blk); >> if (output_sectors < 0) { >> error_report("unable to get output image length: %s\n", >> strerror(-output_sectors)); >> ret = -1; >> goto out; >> - } else if (output_sectors < total_sectors) { >> + } >> + >> + output_sectors /= BDRV_SECTOR_SIZE; >> + if (output_sectors < total_sectors) { > And another. > >> @@ -2585,17 +2591,17 @@ static int img_rebase(int argc, char **argv) >> uint8_t * buf_new; >> float local_progress = 0; >> >> - buf_old = qemu_blockalign(bs, IO_BUF_SIZE); >> - buf_new = qemu_blockalign(bs, IO_BUF_SIZE); >> + buf_old = blk_blockalign(blk, IO_BUF_SIZE); >> + buf_new = blk_blockalign(blk, IO_BUF_SIZE); >> >> - num_sectors = bdrv_nb_sectors(bs); >> + num_sectors = blk_getlength(blk); >> if (num_sectors < 0) { > ... > >> - if (bs_new_backing) { >> - new_backing_num_sectors = bdrv_nb_sectors(bs_new_backing); >> + >> + num_sectors /= BDRV_SECTOR_SIZE; >> + old_backing_num_sectors /= BDRV_SECTOR_SIZE; > and another. > > I did not closely audit if there were any other conversions that should > have been made. Also, I suspect that a blk_nb_sectors() as a pre-req > patch would make this one feel cleaner if you respin and rebase. The reason why I don't want to add blk_nb_sectors() is simply that we want to convert all the code to using bytes at a later point anyway, so I felt like it'd be a step backwards to introduce blk_nb_sectors(). However, I do see your point and intermediate variables probably don't make this any nicer. Also, if we get to convert the code to bytes, finding all instances of {blk,bdrv}_nb_sectors() will be one of the easier tasks, so I'll just introduce blk_nb_sectors(). Max > But if > we don't add blk_nb_sectors(), at least this version of the patch > appears to be clean with what it does, so you can consider this to be a > rather weak: > > Reviewed-by: Eric Blake >