From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47088) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAD4F-0000pz-WC for qemu-devel@nongnu.org; Tue, 07 Jun 2016 05:13:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bAD4E-0007Op-T2 for qemu-devel@nongnu.org; Tue, 07 Jun 2016 05:12:59 -0400 Date: Tue, 7 Jun 2016 11:12:47 +0200 From: Kevin Wolf Message-ID: <20160607091247.GA4684@noname.str.redhat.com> References: <1465225155-18661-1-git-send-email-kwolf@redhat.com> <1465225155-18661-4-git-send-email-kwolf@redhat.com> <575643D7.3090807@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="x+6KMIRAuhnl3hBn" Content-Disposition: inline In-Reply-To: <575643D7.3090807@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 3/5] qcow2: Make copy_sectors() byte based List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-block@nongnu.org, mreitz@redhat.com, jsnow@redhat.com, qemu-devel@nongnu.org --x+6KMIRAuhnl3hBn Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 07.06.2016 um 05:47 hat Eric Blake geschrieben: > On 06/06/2016 08:59 AM, Kevin Wolf wrote: > > This will allow copy on write operations where the overwritten part of > > the cluster is not aligned to sector boundaries. > >=20 > > Also rename the function because it has nothing to do with sectors any > > more. > >=20 > > Signed-off-by: Kevin Wolf > > --- > > block/qcow2-cluster.c | 54 +++++++++++++++++++++++++------------------= -------- > > 1 file changed, 26 insertions(+), 28 deletions(-) > >=20 >=20 > > =20 > > if (bs->encrypted) { > > Error *err =3D NULL; > > + int sector =3D (cluster_offset + offset_in_cluster) >> BDRV_SE= CTOR_BITS; >=20 > Potentially the wrong type... Yes, thanks for catching that. > > assert(s->cipher); > > - if (qcow2_encrypt_sectors(s, start_sect + n_start, > > - iov.iov_base, iov.iov_base, n, > > - true, &err) < 0) { > > + assert((offset_in_cluster & BDRV_SECTOR_MASK) =3D=3D 0); >=20 > Why is this one true? If I have a cluster of 4 sectors, why must > offset_in_cluster fall within only the first of those sectors? Are you > missing a ~, to instead be asserting that offset_in_cluster is > sector-aligned? You mean I should actually test encrypted images? *cough* (I know I did test something with encryption, but maybe that was only after converting reads.) Anyway, missing ~ indeed. > > + assert((bytes & ~BDRV_SECTOR_MASK) =3D=3D 0); >=20 > This one looks correct, stating that the number of bytes to copy is a > sector multiple. >=20 > > + if (qcow2_encrypt_sectors(s, sector, iov.iov_base, iov.iov_bas= e, > > + bytes >> BDRV_SECTOR_BITS, true, &er= r) < 0) { >=20 > ...since encryption allows a 64-bit sector number for the case where the > image is larger than 2T. Kevin --x+6KMIRAuhnl3hBn Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJXVpAPAAoJEH8JsnLIjy/W58kQAKs4p8eMc8tezYBurzG37REb ygCmOf+cPwnxlKwoJePlgAr49YLtcARWR5OVQVZ5VgyxTD1EC3/uNYxThc4NndZH B4VIl4oqercYchLiNQUllBYKfdT92Or75QmFT94X+8AjygY6pimXoTGmF2sGSjgO CERG4yAVlN1zitlXM3EtYfaTcjX3jec+Rs4hgaOFCR1h1FrqZOfz5azoKASzaJsQ +uuJ3CXsHElSKgM6tlaQ58YCwMd/ow5Ir33fhL6zyQ4ixTF9cf6+7rYax75xmeew Xuz7Mgi8Mr6pc95hsIBQnbuODT9kC7gGm2Xw4Jpc8IkPh5DMQkwZEw+5riUX93G6 3ASDxui0otaHcuu61fo3wxGTpLI/n05cu8/7l7PPfjKeksiG+Y+emoWU0c7DjHJ4 kyUH+o6v1VTjvhd0v/RAVgFEYP94qXfzp01pIc2qr4erGpubikrG5CDGsYau6nbo mnSyK/KwlC9SPpPiaOV4+DX8DVRw4s8XiAyFrA9pfjtY1cAJIUKknT+HWtmMxnm4 wM7NQ7n3gUZEkgNHO82e/aUwLVxFOsDA0UpEd0KAmVlFMwvDhIUz6Evy7+6eYlDe bDcTt8Gg18RO2XOoTgwVxAEFTzLblAQHTgvEnrUl7IMrHvNsrp60ORoyBRW0sLuy 2y7jZVHctMVjcwDgZwzR =8eZ6 -----END PGP SIGNATURE----- --x+6KMIRAuhnl3hBn--