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

[Maxim Levitsky]

On Mon, 2019-07-29 at 14:16 +0100, Peter Maydell wrote:
> 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 ?

In theory, in the spec it is a 8 bit field, in practice, it should not be larger
that 12, because at least Linux doesn't support at all block devices that have
larger that 4K block size.

Best regards,
        Maxim Levitsky

> 
> 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
>