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: 3581 bytes --]

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.

> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > ---
> >  hw/block/nvme.h | 8 ++++----
> >  hw/block/nvme.c | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index 9edc86d79e98..c610ab30dc5c 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -217,7 +217,7 @@ typedef struct NvmeCtrl {
> >       * Attached namespaces to this controller.  If subsys is not given, all
> >       * namespaces in this list will always be attached.
> >       */
> > -    NvmeNamespace   *namespaces[NVME_MAX_NAMESPACES];
> > +    NvmeNamespace   *namespaces[NVME_MAX_NAMESPACES + 1];
> >      NvmeSQueue      **sq;
> >      NvmeCQueue      **cq;
> >      NvmeSQueue      admin_sq;
> > @@ -232,7 +232,7 @@ static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid)
> >          return NULL;
> >      }
> >  
> > -    return n->namespaces[nsid - 1];
> > +    return n->namespaces[nsid];
> >  }
> >  
> >  static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns)
> > @@ -253,7 +253,7 @@ static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns)
> >      uint32_t nsid = nvme_nsid(ns);
> >      assert(nsid && nsid <= NVME_MAX_NAMESPACES);
> >  
> > -    n->namespaces[nsid - 1] = ns;
> > +    n->namespaces[nsid] = ns;
> >  }
> >  
> >  static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
> > @@ -261,7 +261,7 @@ static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
> >      uint32_t nsid = nvme_nsid(ns);
> >      assert(nsid && nsid <= NVME_MAX_NAMESPACES);
> >  
> > -    n->namespaces[nsid - 1] = NULL;
> > +    n->namespaces[nsid] = NULL;
> >  }
> >  
> >  static inline NvmeCQueue *nvme_cq(NvmeRequest *req)
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index c54ec3c9523c..6d31d5b17a0b 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -5915,7 +5915,7 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> >              return -1;
> >          }
> >      } else {
> > -        if (n->namespaces[nsid - 1]) {
> > +        if (n->namespaces[nsid]) {
> >              error_setg(errp, "namespace id '%d' is already in use", nsid);
> >              return -1;
> >          }
> > 
> 
> 

-- 
One of us - No more doubt, silence or taboo about mental illness.

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