Re: [Qemu-block] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

[Kevin Wolf]

Am 05.09.2017 um 12:05 hat Daniel P. Berrange geschrieben:
> On Tue, Sep 05, 2017 at 11:52:15AM +0200, Kevin Wolf wrote:
> > Am 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben:
> > > Signed-off-by: Daniel P. Berrange <address@hidden>
> > > ---
> > >  block/crypto.c | 32 ++++++++++++++++----------------
> > >  1 file changed, 16 insertions(+), 16 deletions(-)
> > 
> > I'm actually not sure about this one. Anything that is left after
> > patch 3 is probably not the arbitrary unit that qemu uses internally
> > for some interfaces, but the unit in which data is encrypted.
> > 
> > Basically, if just for fun we ever changed the unit of things like
> > bdrv_write() from 512 to 4096, then everything that needs to or at least
> > can be changed to use 4096 is BDRV_SECTOR_SIZE. But everything that
> > needs to stay on 512 (like I suspect most of the occurrences in the
> > crypto driver) is a different constant really (QCRYPTO_SECTOR_SIZE?).
> 
> Yeah for sure LUKSv1 & legacy qcow2 encryption need to stay 512
> forever, so if BDRV_SECTOR_SIZE were to change that would be a
> problem.
> 
> I wrote this on the assumption that we would never change BDRV_SECTOR_SIZE
> though. If we did need different sector sizes in the block layer I figured
> it would surely end up being a dynamic property per disk, rather than just
> changing the compile time constant. So from that POV I thought it ok to
> use BDRV_SECTOR_SIZE.

Yes, I agree that we'll never BDRV_SECTOR_SIZE in practice. I just use
this as a way to check whether this is really BDRV_SECTOR_SIZE, or
whether we have two different quantities that just happen to be both
512.

For example, QDICT_BUCKET_MAX is also 512, but that doesn't make it
right to use here, even though it works and is unlikely to change.

What we may actually want to do at some point (won't be in the near
future, though) is removing BDRV_SECTOR_SIZE because it refers to an
arbitrary unit in APIs and we're in the process of making everything
byte-based instead. This is another way to check whether some value is
BDRV_SECTOR_SIZE: If it will remain even after removing APIs with
sector-based parameters, it's probably something different.

> Perhaps I should instead add a qcrypto_block_get_sector_size() API though
> and use that, so we can fetch the sector size per encryption scheme in
> case we ever get a scheme using a non-512 sector size for encryption.

If you have a use for it, sure. Otherwise, I'd just suggest introducing
another constant for now.

Kevin