Am 26.03.24 um 10:06 schrieb Markus Armbruster: >> @@ -365,7 +368,13 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, >> BdrvChild *target, >> >> GLOBAL_STATE_CODE(); >> >> - cluster_size = block_copy_calculate_cluster_size(target->bs, errp); >> + if (min_cluster_size && !is_power_of_2(min_cluster_size)) { > > min_cluster_size is int64_t, is_power_of_2() takes uint64_t. Bad if > min_cluster_size is negative. Could this happen? >
No, because it comes in as a uint32_t via the QAPI (the internal caller added by patch 2/2 from the backup code also gets the value via QAPI and there uint32_t is used too). ---snip--- >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 0a72c590a8..85c8f88f6e 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -4625,12 +4625,18 @@ >> # @on-cbw-error parameter will decide how this failure is handled. >> # Default 0. (Since 7.1) >> # >> +# @min-cluster-size: Minimum size of blocks used by copy-before-write >> +# operations. Has to be a power of 2. No effect if smaller than >> +# the maximum of the target's cluster size and 64 KiB. Default 0. >> +# (Since 9.0) >> +# >> # Since: 6.2 >> ## >> { 'struct': 'BlockdevOptionsCbw', >> 'base': 'BlockdevOptionsGenericFormat', >> 'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap', >> - '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } } >> + '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32', >> + '*min-cluster-size': 'uint32' } } > > Elsewhere in the schema, we use either 'int' or 'size' for cluster-size. > Why the difference? > The motivation was to disallow negative values up front and have it work with block_copy_calculate_cluster_size(), whose result is an int64_t. If I go with 'int', I'll have to add a check to disallow negative values. If I go with 'size', I'll have to add a check for to disallow too large values. Which approach should I go with? Best Regards, Fiona