On Tue, Apr 18, 2017 at 10:08:20AM -0500, Eric Blake wrote: > On 04/18/2017 08:57 AM, Stefan Hajnoczi wrote: > > bdrv_measure() provides a conservative maximum for the size of a new > > image. This information is handy if storage needs to be allocated (e.g. > > a SAN or an LVM volume) ahead of time. > > > > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> > > Reviewed-by: Alberto Garcia <be...@igalia.com> > > --- > > qapi/block-core.json | 25 +++++++++++++++++++++++++ > > include/block/block.h | 4 ++++ > > include/block/block_int.h | 2 ++ > > block.c | 35 +++++++++++++++++++++++++++++++++++ > > 4 files changed, 66 insertions(+) > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > index 033457c..1ea5536 100644 > > --- a/qapi/block-core.json > > +++ b/qapi/block-core.json > > @@ -463,6 +463,31 @@ > > '*dirty-bitmaps': ['BlockDirtyInfo'] } } > > > > ## > > +# @BlockMeasureInfo: > > +# > > +# Image size calculation information. This structure describes the size > > +# requirements for creating a new image file. > > +# > > +# The size requirements depend on the new image file format. File size > > always > > +# equals virtual disk size for the 'raw' format. Compact formats such as > > +# 'qcow2' represent unallocated and zero regions efficiently so file size > > may > > +# be smaller than virtual disk size. > > I guess that implies that holes due to a file system that supports them > is NOT considered a compact format under qemu's control. Or maybe > another way of wording it is that we are reporting the size of all guest > contents that are associated with a host offset by this layer (all > sectors of a raw format have a host offset, even if that offset falls in > the hole of a sparse file; but sectors of qcow2 do not necessarily have > a host offset if they are unallocated or zero). > > But I'm not sure my alternative wording adds anything, so I'm also fine > if you don't feel like tweaking it any further.
Thanks for the feedback. I'll clarify what "size" means.
> > +#
> > +# The values are upper bounds that are guaranteed to fit the new image
> > file.
> > +# Subsequent modification, such as internal snapshot or bitmap creation,
> > may
> > +# require additional space and is not covered here.
> > +#
> > +# @required: Size required for a new image file, in bytes.
> > +#
> > +# @fully-allocated: Image file size, in bytes, once data has been written
> > +# to all sectors.
> > +#
> > +# Since: 2.10
> > +##
> > +{ 'struct': 'BlockMeasureInfo',
> > + 'data': {'required': 'int', 'fully-allocated': 'int'} }
> > +
>
> > +++ b/include/block/block.h
> > @@ -298,6 +298,10 @@ int bdrv_truncate(BdrvChild *child, int64_t offset);
> > int64_t bdrv_nb_sectors(BlockDriverState *bs);
> > int64_t bdrv_getlength(BlockDriverState *bs);
> > int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
> > +void bdrv_measure(BlockDriver *drv, QemuOpts *opts,
> > + BlockDriverState *in_bs,
> > + BlockMeasureInfo *info,
> > + Error **errp);
>
> Would it be any better to have BlockMeasureInfo* be the return value (or
> NULL on error), instead of an output-only parameter? Of course, that
> changes the allocation scheme (as written, a caller can stack-allocate
> or malloc the pointer it passes in, but with a return value of a
> pointer, it will always be malloc'd); on the other hand, the allocation
> scheme may matter down the road if the struct ever gains a non-scalar
> member where stack-allocation becomes harder to clean up than just
> calling qapi_free_BlockMeasureInfo().
Yes, that makes the function more self-explanatory.
> > +++ b/include/block/block_int.h
> > @@ -201,6 +201,8 @@ struct BlockDriver {
> > int64_t (*bdrv_getlength)(BlockDriverState *bs);
> > bool has_variable_length;
> > int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
> > + void (*bdrv_measure)(QemuOpts *opts, BlockDriverState *in_bs,
> > + BlockMeasureInfo *info, Error **errp);
>
> I know we haven't done a good job in the past, but should we start
> trying to do better at documenting callback constraints of new things
> added in this header?
.bdrv_measure() is a 1:1 pass-through of the public bdrv_measure()
function. All the public function does is to dereference
drv->bdrv_measure.
I think that's why many of the other callbacks also have no
documentation - they inherit semantics from the public function. We
don't need to duplicate the doc comments.
signature.asc
Description: PGP signature
