Am 09.02.2018 um 00:29 hat Eric Blake geschrieben:
> On 02/08/2018 01:23 PM, Kevin Wolf wrote:
> > All of the simple options are now passed to qcow2_create2() in a
> > BlockdevCreateOptions object. Still missing: node-name and the
> > encryption options.
> >
> > Signed-off-by: Kevin Wolf <kw...@redhat.com>
> > ---
> > block/qcow2.c | 190
> > ++++++++++++++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 152 insertions(+), 38 deletions(-)
> >
>
> > -static size_t qcow2_opt_get_cluster_size_del(QemuOpts *opts, Error **errp)
> > +static bool validate_cluster_size(size_t cluster_size, Error **errp)
> > {
> > - size_t cluster_size;
> > - int cluster_bits;
> > -
> > - cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
> > - DEFAULT_CLUSTER_SIZE);
> > - cluster_bits = ctz32(cluster_size);
> > + int cluster_bits = ctz32(cluster_size);
> > if (cluster_bits < MIN_CLUSTER_BITS || cluster_bits >
> > MAX_CLUSTER_BITS ||
> > (1 << cluster_bits) != cluster_size)
>
> Pre-existing, but why are we manually calling ctz32() instead of using
> is_power_of_2()?
Probably because is_power_of_2() is newer than this code.
Also, if we don't call ctz32(), we'd have to use (1 << MIN_CLUSTER_BITS)
and (1 << MAX_CLUSTER_BITS), so I'm not sure if that would be better.
> > @@ -2720,10 +2726,92 @@ static int qcow2_create2(BlockDriverState *bs,
> > int64_t total_size,
> > */
> > BlockBackend *blk;
> > QCowHeader *header;
> > + size_t cluster_size;
> > + int version;
> > + int refcount_order;
> > uint64_t* refcount_table;
> > Error *local_err = NULL;
> > int ret;
> > + /* Validate options and set default values */
> > + assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2);
> > + qcow2_opts = &create_options->u.qcow2;
> > +
> > + if (!QEMU_IS_ALIGNED(qcow2_opts->size, BDRV_SECTOR_SIZE)) {
> > + error_setg(errp, "Image size must be a multiple of 512 bytes");
> > + ret = -EINVAL;
> > + goto out;
> > + }
>
> This check looks new. Does it really belong in this patch? And it does NOT
> match what qemu-img can currently do, nor the fact that qcow2 supports
> byte-based addressing:
>
> $ qemu-img create -f qcow2 tmp 12345
> Formatting 'tmp', fmt=qcow2 size=12345 cluster_size=65536 lazy_refcounts=off
> refcount_bits=16
You're ignoring the result of this command:
$ ./qemu-img create -f qcow2 /tmp/test.qcow2 12345
Formatting '/tmp/test.qcow2', fmt=qcow2 size=12345 cluster_size=65536
lazy_refcounts=off refcount_bits=16
$ ./qemu-img info -f qcow2 /tmp/test.qcow2
image: /tmp/test.qcow2
file format: qcow2
virtual size: 13K (12800 bytes)
disk size: 196K
cluster_size: 65536
Format specific information:
compat: 1.1
lazy refcounts: false
refcount bits: 16
corrupt: false
As you can see, the size got silently rounded up to the next 512 bytes.
qcow2_create() still does the same even after this patch, but I chose
not to extend the magic rounding to the QMP command.
In the protocol drivers, I generally just allow byte granularity image
sizes, but qcow2 does use sectors internally, so it felt safer to
require 512 byte alignment in qcow2.
> > + if (!qcow2_opts->has_lazy_refcounts) {
> > + qcow2_opts->lazy_refcounts = false;
> > + }
> > + if (version < 3 && qcow2_opts->lazy_refcounts) {
> > + error_setg(errp, "Lazy refcounts only supported with compatibility
> > "
> > + "level 1.1 and above (use compat=1.1 or greater)");
>
> Do we want to reword this error message at all, now that QMP spells it 'v3'?
> Should qemu-img be taught to accept 'compat=v3' as a synonym to
> 'compat=1.1'?
Actually, it does accept 'v3' when the whole series is applied, and the
message is changed in a later patch that enables this.
Kevin
