Re: [PATCH v2 13/16] nvme: factor out namespace setup

[Philippe Mathieu-Daudé]

On 4/15/20 3:20 PM, Klaus Birkelund Jensen wrote:
> On Apr 15 15:16, Philippe Mathieu-Daudé wrote:
> > On 4/15/20 3:01 PM, Klaus Jensen wrote:
> > > From: Klaus Jensen <address@hidden>
> > >
> > > Signed-off-by: Klaus Jensen <address@hidden>
> > > ---
> > >    hw/block/nvme.c | 46 ++++++++++++++++++++++++++--------------------
> > >    1 file changed, 26 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index d5244102252c..2b007115c302 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -1358,6 +1358,27 @@ static void nvme_init_blk(NvmeCtrl *n, Error **errp)
> > >                                      false, errp);
> > >    }
> > > +static void nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> > > +{
> > > +    int64_t bs_size;
> > > +    NvmeIdNs *id_ns = &ns->id_ns;
> > > +
> > > +    bs_size = blk_getlength(n->conf.blk);
> > > +    if (bs_size < 0) {
> > > +        error_setg_errno(errp, -bs_size, "could not get backing file size");
> > > +        return;
> > > +    }
> > > +
> > > +    n->ns_size = bs_size;
> > > +
> > > +    id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
> > > +    id_ns->nsze = cpu_to_le64(nvme_ns_nlbas(n, ns));
> > > +
> > > +    /* no thin provisioning */
> > > +    id_ns->ncap = id_ns->nsze;
> > > +    id_ns->nuse = id_ns->ncap;
> > > +}
> > > +
> > >    static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> > >    {
> > >        NvmeCtrl *n = NVME(pci_dev);
> > > @@ -1365,7 +1386,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> > >        Error *err = NULL;
> > >        int i;
> > > -    int64_t bs_size;
> > >        uint8_t *pci_conf;
> > >        nvme_check_constraints(n, &err);
> > > @@ -1376,12 +1396,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
> > > **errp)
> > >        nvme_init_state(n);
> > > -    bs_size = blk_getlength(n->conf.blk);
> > > -    if (bs_size < 0) {
> > > -        error_setg(errp, "could not get backing file size");
> > > -        return;
> > > -    }
> > > -
> > >        nvme_init_blk(n, &err);
> > >        if (err) {
> > >            error_propagate(errp, err);
> > > @@ -1394,8 +1408,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> > >        pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS);
> > >        pcie_endpoint_cap_init(pci_dev, 0x80);
> > > -    n->ns_size = bs_size / (uint64_t)n->num_namespaces;
> >
> > Valid because currently 'n->num_namespaces' = 1, OK.
> >
> > Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>   
> Thank you for the reviews Philippe and the suggesting that I split up
> the series :)

No prob, thanks for the quick fixes. Let's see if Keith is happy with 
v2. Personally I don't have anymore comments.

> I'll get the v1.3 series ready next.

Cool. What really matters (to me) is seeing tests. If we can merge tests 
(without multiple namespaces) before the rest of your series, even 
better. Tests give reviewers/maintainers confidence that code isn't 
breaking ;)

Regards,

Phil.