Re: [Qemu-devel] [PULL 2/5] block/nvme: support larger that 512 bytes sector devices

[Peter Maydell]

On Mon, 22 Jul 2019 at 18:26, Max Reitz <address@hidden> wrote:
>
> From: Maxim Levitsky <address@hidden>
>
> Currently the driver hardcodes the sector size to 512,
> and doesn't check the underlying device. Fix that.
>
> Also fail if underlying nvme device is formatted with metadata
> as this needs special support.
>
> Signed-off-by: Maxim Levitsky <address@hidden>
> Message-id: address@hidden
> Signed-off-by: Max Reitz <address@hidden>

> +static int64_t nvme_get_blocksize(BlockDriverState *bs)
> +{
> +    BDRVNVMeState *s = bs->opaque;
> +    assert(s->blkshift >= BDRV_SECTOR_BITS);
> +    return 1 << s->blkshift;
> +}

Hi -- Coverity points out here that we calculate the
"1 << s->blkshift" as a 32-bit shift, but then return an
int64_t type (CID 1403771).

Can the blkshift ever really be 31 or more ?

The types here seem weird anyway -- we return an int64_t,
but the only user of this is nvme_probe_blocksizes(),
which uses the value only to set BlockSizes::phys and ::log,
both of which are of type "uint32_t". That leads me to think
that the right return type for the function is uint32_t.

PS: this is the only Coverity issue currently outstanding so
if it's a trivial fix it might be nice to put it into rc3.

thanks
-- PMM