On 07/04/2017 02:06 AM, Fam Zheng wrote: > On Mon, 07/03 17:14, Eric Blake wrote: >> @@ -1717,6 +1718,10 @@ int64_t coroutine_fn >> bdrv_co_get_block_status_from_backing(BlockDriverState *bs, >> * Drivers not implementing the functionality are assumed to not support >> * backing files, hence all their sectors are reported as allocated. >> * >> + * If 'allocation' is true, the caller only cares about allocation >> + * status; this is a hint that a larger 'pnum' result is more >> + * important than including BDRV_BLOCK_OFFSET_VALID in the return. >> + * > > This is slightly unintuitive. I would guess "allocation == false" means "I > don't > care about the allocation status" but it actually is "I don't care about the > consecutiveness". The "only" semantics is missing in the parameter name. > > Maybe rename it to "consecutive" and invert the logic? I.e. "consecutive == > true" means BDRV_BLOCK_OFFSET_VALID is wanted.
Reasonable idea; other [shorter] names I've been toying with: strict mapping precise any of which, if true (set true by bdrv_get_block_status), means that I care more about BDRV_BLOCK_OFFSET_VALID and validity for learning host offsets, if false it means I'm okay getting a larger *pnum even if it extends over disjoint host offsets; or: fast which if true (set true by bdrv_is_allocated) means I want a larger *pnum even if I have to sacrifice accuracy by omitting BDRV_BLOCK_OFFSET_VALID, because I'm trying to reduce effort spent. > > Sorry for bikeshedding. Not a problem, I also had some double-takes in writing my own code trying to remember which way I wanted the 'allocation' boolean to be set, so coming up with a more intuitive name/default state in order to help legibility is worth it. Do any of my above suggestions sound better? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature