On 29.07.19 15:16, Peter Maydell wrote: > On Mon, 22 Jul 2019 at 18:26, Max Reitz <mre...@redhat.com> wrote: >> >> From: Maxim Levitsky <mlevi...@redhat.com> >> >> 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 <mlevi...@redhat.com> >> Message-id: 20190716163020.13383-3-mlevi...@redhat.com >> Signed-off-by: Max Reitz <mre...@redhat.com> > >> +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.
Maxim, what do you think? How about we let nvme_identify() limit blkshift to something sane and then return a uint32_t here? In theory it would be limited by page_size, and that has a maximum value of 2^27. In practice, though, that limit is checked by another 32-bit shift... Max
signature.asc
Description: OpenPGP digital signature