Re: [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> When we use BLK_MIG_BLOCK_SIZE in expressions like
> block_mig_state.submitted * BLK_MIG_BLOCK_SIZE, this multiplication
> is done as 32 bits, because both operands are 32 bits.  Coverity
> complains about possible overflows because we then accumulate that
> into a 64 bit variable.
> 
> Define BLK_MIG_BLOCK_SIZE as unsigned long long using the ULL suffix.
> The only two current uses of it with this problem are both in
> block_save_pending(), so we could just cast to uint64_t there, but
> using the ULL suffix is simpler and ensures that we don't
> accidentally introduce new variants of the same issue in future.
> 
> Resolves: Coverity CID 1487136, 1487175
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I haven't tried to analyse the code to see if the multiplications
> could ever actually end up overflowing, but I would assume
> probably not.
> 
>  migration/block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/block.c b/migration/block.c
> index 9e5aae58982..3577c815a94 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -28,7 +28,7 @@
>  #include "sysemu/block-backend.h"
>  #include "trace.h"
>  
> -#define BLK_MIG_BLOCK_SIZE           (1 << 20)
> +#define BLK_MIG_BLOCK_SIZE           (1ULL << 20)

Is it a problem that this is passed to bdrv_create_dirty_bitmap that
takes a uint32_t ?

Dave

>  #define BDRV_SECTORS_PER_DIRTY_CHUNK (BLK_MIG_BLOCK_SIZE >> BDRV_SECTOR_BITS)
>  
>  #define BLK_MIG_FLAG_DEVICE_BLOCK       0x01
> -- 
> 2.25.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK