Re: [PATCH for-6.0 v2 4/8] hw/block/nvme: fix controller namespaces array indexing

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 1805 bytes --]

On Apr  6 09:28, Klaus Jensen wrote:
> On Apr  6 09:01, Philippe Mathieu-Daudé wrote:
> > On 4/5/21 7:54 PM, Klaus Jensen wrote:
> > > From: Klaus Jensen <k.jensen@samsung.com>
> > > 
> > > The controller namespaces array being 0-indexed requires 'nsid - 1'
> > > everywhere. Something that is easy to miss. Align the controller
> > > namespaces array with the subsystem namespaces array such that both are
> > > 1-indexed.
> > 
> > TBH I don't understand the justification.
> 
> Justification is mostly to align with the subsystem device. I like the
> '1-indexed' approach better. And the -1 causes Coverity to complain
> before the assert was added.
> 
> > Assuming you hit a
> > bug and try to protect yourself, maybe now you should also
> > check for
> > 
> >   assert(n->namespaces[0] == NULL);
> > 
> > somewhere. In nvme_ns() maybe?
> > 
> 
> That is definitely a state that should always hold, I guess we can do
> that, but we do already guard all "insertions" into the namespace array
> by an assert on the nsid. Then again, asserting here makes sure that we
> don't introduce something else that inserts on this invalid position.
> 
> So, good point, I'll add it.
> 

Then again again.

I don't see the reason for the assert. Even if something ends up there 
by mistake we will never return it. If something ends up there due to 
new code, that nvme_ns() will always return NULL when nsid is zero and 
that should weed out the bug easily.

I'll update the commit message to make it clear that this is about 
making both the subsystem and controller namespaces arrays 1-indexed. 
Them being indexed differently is a recipe for disaster I'd say.

In anycase, I it actually a stretch to call this a bug fix, so I'll drop 
it and queue it up for v6.1.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]