From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54395) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TOxW7-0007Gp-7f for qemu-devel@nongnu.org; Thu, 18 Oct 2012 17:20:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TOxW6-0000th-4u for qemu-devel@nongnu.org; Thu, 18 Oct 2012 17:20:35 -0400 Received: from mail.avalus.com ([89.16.176.221]:41088) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TOxW5-0000tI-VH for qemu-devel@nongnu.org; Thu, 18 Oct 2012 17:20:34 -0400 Date: Thu, 18 Oct 2012 22:20:29 +0100 From: Alex Bligh Message-ID: In-Reply-To: <507EC493.2090308@redhat.com> References: <1350391578-1191-1-git-send-email-alex@alex.org.uk> <507EC493.2090308@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline Subject: Re: [Qemu-devel] [PATCHv4] qemu-img rebase: use empty string to rebase without backing file Reply-To: Alex Bligh List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Alex Bligh Kevin, --On 17 October 2012 16:45:39 +0200 Kevin Wolf wrote: > um_sectors) { >> @@ -1675,7 +1679,12 @@ static int img_rebase(int argc, char **argv) >> * backing file are overwritten in the COW file now, so the visible content >> * doesn't change when we switch the backing file. >> */ >> - ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt); >> + if (bs_new_backing) { > > I think this needs to be out_baseimg, otherwise -u is broken. I've > updated the patch, please check if you agree with the fix. I'm not sure I do agree. When -u is not specified, then unsafe=0. If the backing file is the empty string then bs_new_backing is 0 here, and the if condition evaluates to false, in the current patch. If you make that "if (outbase_img)" then it will still evaluate to true, because whilst outbase_img is non-zero, outbase_img[0] is zero. So I think you either need to do: if (bs_new_backing || unsafe) which replicates the existing behaviour, or if (out_baseimg && out_baseimg[0]) As it happens, we despite what Eric Blake said, we couldn't get an unsafe rebase to no backing file to work with the existing code (with our without our patch). The second option may fix this bug. Reading line 1497, is this because the semantic is not 'an empty string', but 'omit -b entirely'? This behaviour is undocumented in the manpage which specifies -b as a compulsory option. If so, that's a bit unfortunate as we now have different semantics with and without -u. Note if no -b parameter is supplied, there is also a possible null pointer exception at line 1693 (null passed to error_report). -- Alex Bligh