[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-6.0 v2 4/8] hw/block/nvme: fix controller namespaces arra
From: |
Klaus Jensen |
Subject: |
Re: [PATCH for-6.0 v2 4/8] hw/block/nvme: fix controller namespaces array indexing |
Date: |
Tue, 6 Apr 2021 20:21:22 +0200 |
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.
signature.asc
Description: PGP signature
[PATCH for-6.0 v2 8/8] hw/block/nvme: add missing copyright headers, Klaus Jensen, 2021/04/05
[PATCH for-6.0 v2 7/8] hw/block/nvme: fix handling of private namespaces, Klaus Jensen, 2021/04/05
Re: [PATCH for-6.0 v2 0/8] hw/block/nvme: misc fixes, Keith Busch, 2021/04/05