Re: [Qemu-devel] [PATCH v2 3/5] qcow2: Make copy_sectors() byte based

[Kevin Wolf]

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.
> > 
> > Also rename the function because it has nothing to do with sectors any
> > more.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  block/qcow2-cluster.c | 54 
> > +--
> >  1 file changed, 26 insertions(+), 28 deletions(-)
> > 
> 
> >  
> >  if (bs->encrypted) {
> >  Error *err = NULL;
> > +int sector = (cluster_offset + offset_in_cluster) >> 
> > BDRV_SECTOR_BITS;
> 
> 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, ) < 0) {
> > +assert((offset_in_cluster & BDRV_SECTOR_MASK) == 0);
> 
> 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) == 0);
> 
> This one looks correct, stating that the number of bytes to copy is a
> sector multiple.
> 
> > +if (qcow2_encrypt_sectors(s, sector, iov.iov_base, iov.iov_base,
> > +  bytes >> BDRV_SECTOR_BITS, true, ) < 
> > 0) {
> 
> ...since encryption allows a 64-bit sector number for the case where the
> image is larger than 2T.

Kevin


pgpqmCxfcRpAK.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 3/5] qcow2: Make copy_sectors() byte based

[Eric Blake]

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.
> 
> Also rename the function because it has nothing to do with sectors any
> more.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/qcow2-cluster.c | 54 
> +--
>  1 file changed, 26 insertions(+), 28 deletions(-)
> 

>  
>  if (bs->encrypted) {
>  Error *err = NULL;
> +int sector = (cluster_offset + offset_in_cluster) >> 
> BDRV_SECTOR_BITS;

Potentially the wrong type...

>  assert(s->cipher);
> -if (qcow2_encrypt_sectors(s, start_sect + n_start,
> -  iov.iov_base, iov.iov_base, n,
> -  true, ) < 0) {
> +assert((offset_in_cluster & BDRV_SECTOR_MASK) == 0);

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?

> +assert((bytes & ~BDRV_SECTOR_MASK) == 0);

This one looks correct, stating that the number of bytes to copy is a
sector multiple.

> +if (qcow2_encrypt_sectors(s, sector, iov.iov_base, iov.iov_base,
> +  bytes >> BDRV_SECTOR_BITS, true, ) < 
> 0) {

...since encryption allows a 64-bit sector number for the case where the
image is larger than 2T.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature