From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55121) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YORfs-0002WU-EL for qemu-devel@nongnu.org; Thu, 19 Feb 2015 09:01:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YORfp-0003Ze-1p for qemu-devel@nongnu.org; Thu, 19 Feb 2015 09:01:52 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34041) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YORfo-0003ZR-QS for qemu-devel@nongnu.org; Thu, 19 Feb 2015 09:01:49 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t1JE1mMq030577 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 19 Feb 2015 09:01:48 -0500 Message-ID: <54E5ECCA.7080807@redhat.com> Date: Thu, 19 Feb 2015 09:01:46 -0500 From: Max Reitz MIME-Version: 1.0 References: <1424299260-1763-1-git-send-email-mreitz@redhat.com> <1424299260-1763-2-git-send-email-mreitz@redhat.com> <54E54F99.4030301@redhat.com> In-Reply-To: <54E54F99.4030301@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 11/25] qcow2: refcount_order parameter for qcow2_create2 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi On 2015-02-18 at 21:51, Eric Blake wrote: > On 02/18/2015 03:40 PM, Max Reitz wrote: >> Add a refcount_order parameter to qcow2_create2(), use that value for >> the image header and for calculating the size required for >> preallocation. >> >> For now, always pass 4. >> >> This addition requires changes to the calculation of the file size for >> the "full" and "falloc" preallocation modes. That in turn is a nice >> opportunity to add a comment about that calculation not necessarily >> being exact (and that being intentional). >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2.c | 47 ++++++++++++++++++++++++++++++++++++----------- >> 1 file changed, 36 insertions(+), 11 deletions(-) >> >> @@ -2010,6 +2022,8 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp) >> size_t cluster_size = DEFAULT_CLUSTER_SIZE; >> PreallocMode prealloc; >> int version = 3; >> + uint64_t refcount_bits = 16; >> + int refcount_order; >> Error *local_err = NULL; >> int ret; >> >> @@ -2064,8 +2078,19 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp) >> goto finish; >> } >> >> + if (version < 3 && refcount_bits != 16) { >> + error_setg(errp, "Different refcount widths than 16 bits require " >> + "compatibility level 1.1 or above (use compat=1.1 or " >> + "greater)"); >> + ret = -EINVAL; >> + goto finish; >> + } >> + >> + refcount_order = ffs(refcount_bits) - 1; > ffs() doesn't work on uint64_t (it gives the wrong answer for > 0x100000000, for example); you want to use ffsll(). But ffsll() isn't > portable. But we have include/qemu/host-utils.h that gives us ctz64() > which is what we want (where ctz==ffs-1 other than for the special case > of 0). So this should be: > > refcount_order = ctz64(refcount_bits); > > With that change, > Reviewed-by: Eric Blake I intentionally left the ffs() here, because after this patch, refcount_bits is guaranteed to be 16, and after patch 14, it's guaranteed to be 64 or less (it's more obvious after patch 14 than here). So I would be fine with ctz64(), but in my opinion, ffs() is just fine, too. Max > (Hmm, that means that at least qcow2.c:qcow2_create2() has a bug for > calling ffs(size_t), and we probably ought to eradicate other uses of > ffs from the tree - but as a separate followup). >