From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39131) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TPFic-0001Pw-JJ for qemu-devel@nongnu.org; Fri, 19 Oct 2012 12:46:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TPFiS-00005I-P6 for qemu-devel@nongnu.org; Fri, 19 Oct 2012 12:46:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30573) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TPFiS-00004w-F2 for qemu-devel@nongnu.org; Fri, 19 Oct 2012 12:46:32 -0400 Message-ID: <508183E0.3000106@redhat.com> Date: Fri, 19 Oct 2012 18:46:24 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1350391578-1191-1-git-send-email-alex@alex.org.uk> <507EC493.2090308@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv4] qemu-img rebase: use empty string to rebase without backing file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Bligh Cc: qemu-devel@nongnu.org Am 18.10.2012 23:20, schrieb 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]) Good point, I changed it. > 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). Right. I think not passing -b at all or passing an empty string should have the same meaning, namely removing the backing file reference. I won't try to modify this patch to do this, though, we can do it on top. Kevin