From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50975) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TKBil-00076S-O3 for qemu-devel@nongnu.org; Fri, 05 Oct 2012 13:29:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TKBik-00064E-DX for qemu-devel@nongnu.org; Fri, 05 Oct 2012 13:29:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:22463) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TKBik-00064A-50 for qemu-devel@nongnu.org; Fri, 05 Oct 2012 13:29:54 -0400 Message-ID: <506F190F.90100@redhat.com> Date: Fri, 05 Oct 2012 11:29:51 -0600 From: Eric Blake MIME-Version: 1.0 References: <1348855033-17174-1-git-send-email-kwolf@redhat.com> <1348855033-17174-13-git-send-email-kwolf@redhat.com> In-Reply-To: <1348855033-17174-13-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enigD1476D80E9BFF3DE4BBF3B59" Subject: Re: [Qemu-devel] [PATCH 12/30] QAPI: add command for live block commit, 'block-commit' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, anthony@codemonkey.ws This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigD1476D80E9BFF3DE4BBF3B59 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 09/28/2012 11:56 AM, Kevin Wolf wrote: > From: Jeff Cody >=20 > The command for live block commit is added, which has the following > arguments: >=20 > device: the block device to perform the commit on (mandatory) > base: the base image to commit into; optional (if not specified, > it is the underlying original image) > top: the top image of the commit - all data from inside top down > to base will be committed into base (mandatory for now; see > note, below) We will need a followup patch, for this to work on chains with relative backing file names. > + if (base && has_base) { > + base_bs =3D bdrv_find_backing_image(bs, base); > + } else { > + base_bs =3D bdrv_find_base(bs); > + } > + > + if (base_bs =3D=3D NULL) { > + error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL"); > + return; > + } > + > + /* default top_bs is the active layer */ > + top_bs =3D bs; > + > + if (top) { > + if (strcmp(bs->filename, top) !=3D 0) { > + top_bs =3D bdrv_find_backing_image(bs, top); > + } > + } Right now, if I have 'base' <- 'snap1'(base) <- 'snap2'(snap1) <- 'active'(snap2), then base_bs and top_bs will be found, but later on in commit_start, we have: >=20 > + /* top and base may be valid, but let's make sure that base is rea= chable > + * from top */ > + if (bdrv_find_backing_image(top, base->filename) !=3D base) { > + error_setg(errp, > + "Base (%s) is not reachable from top (%s)", > + base->filename, top->filename); > + return; > + } which uses the absolute file name base->filename and fails to find base, making it impossible to commit into a base file that was referenced via relative name in the chain. I think that bdrv_find_backing_image needs to canonicalize any relative name passed in on input relative to the directory of the bs where it is starting the search, and then search for absolute name matches rather than the current approach of searching for exact string matches (even if the exact string is a relative name). Also, consider this case of mixed relative and absolute backing files in the chain: /dir1/base <- /dir1/file1(base) <- /dir2/base(/dir1/file1) <- /dir2/file2(base) The relative name 'base' appears twice in the chain, so you either have to declare it ambiguous, or else declare that relative names are canonicalized relative to the starting point (such that bdrv_find_backing_image(/dir1/file1, "base") gives a different result than bdrv_find_backing_image(/dir2/file2, "base"). Which means, if I request block-commit "top":"/dir1/file1", "base":"base", am I requesting a commit into /dir1/base (good) or into /dir2/base (swapped arguments)? Fortunately, it looks like if I have 'base' <- 'snap1'(/path/to/base) <- 'snap2'(/path/to/snap1) <- 'active'(/path/to/snap2), then things work for committing snap2 into snap1, when I specify absolute file names for top and base. But here, it would be convenient if I could pass names relative to the location of active and have them canonicalized into absolute, so the same fix for relative chains will make absolute chains easier to use. Sounds like we need more tests to cover these scenarios. --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enigD1476D80E9BFF3DE4BBF3B59 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iQEcBAEBCAAGBQJQbxkPAAoJEKeha0olJ0Nq2pYH/jqwhhVY9U8zl0WTP3M5gIJc 4vFWqrrcB8XWLQmmGXIdoCWt5nGyuMKF/EJb7whTqKmeJdfrDMIWbpMLOMR4o/YX r9pBdzyhhgfCIOHbE559aRk/mS0tZOrON0jXS2pEcqWVTt6WZ2ZqgFVrN4Ac4EaP pcajd56ui37AL6IPgK5zXNEVPHaGRvRrRTHHkwSo0+GK3H9Tp0cFcDIFHqH0cRa4 VhiwKeabot2FUxagSCo5byiYM0A+Vt8wWaiQgHZlNCBf0NYITlmNH0ofRbYD7OPl Q66At5It5WYbqjfPQZaSekfcMx9rWsC+eTJHb9lfRYPD5oTGbYnw3ldSbzlEYG0= =3JLp -----END PGP SIGNATURE----- --------------enigD1476D80E9BFF3DE4BBF3B59--