RE: [PATCH v4 1/2] hw/nvme: Support for Namespaces Management from guest OS - create-ns

[Michael Kropaczek (CW)]

Hi Keith,

Thank you for the review and I agree with you, the issues will be addressed.

Regards,
  Michael

-----Original Message-----
From: Keith Busch <kbusch@kernel.org> 
Sent: Wednesday, December 28, 2022 12:10 PM
To: Jonathan Derrick <jonathan.derrick@linux.dev>
Cc: qemu-devel@nongnu.org; Michael Kropaczek (CW) 
<Michael.Kropaczek@solidigm.com>; qemu-block@nongnu.org; Klaus Jensen 
<its@irrelevant.dk>; Kevin Wolf <kwolf@redhat.com>; Hanna Reitz 
<hreitz@redhat.com>
Subject: Re: [PATCH v4 1/2] hw/nvme: Support for Namespaces Management from 
guest OS - create-ns

[You don't often get email from kbusch@kernel.org. Learn why this is important 
at https://aka.ms/LearnAboutSenderIdentification ]

Caution: External Email


On Wed, Dec 28, 2022 at 01:41:40PM -0600, Jonathan Derrick wrote:
> +static uint16_t nvme_ns_mgmt(NvmeCtrl *n, NvmeRequest *req) {
> +    NvmeCtrl *n_p = NULL;     /* primary controller */
> +    NvmeIdCtrl *id = &n->id_ctrl;
> +    NvmeNamespace *ns;
> +    NvmeIdNsMgmt id_ns = {};
> +    uint8_t flags = req->cmd.flags;
> +    uint32_t nsid = le32_to_cpu(req->cmd.nsid);
> +    uint32_t dw10 = le32_to_cpu(req->cmd.cdw10);
> +    uint32_t dw11 = le32_to_cpu(req->cmd.cdw11);
> +    uint8_t sel = dw10 & 0xf;
> +    uint8_t csi = (dw11 >> 24) & 0xf;
> +    uint16_t i;
> +    uint16_t ret;
> +    Error *local_err = NULL;
> +
> +    trace_pci_nvme_ns_mgmt(nvme_cid(req), nsid, sel, csi, 
> + NVME_CMD_FLAGS_PSDT(flags));
> +
> +    if (!(le16_to_cpu(id->oacs) & NVME_OACS_NS_MGMT)) {
> +        return NVME_NS_ATTACH_MGMT_NOTSPRD | NVME_DNR;
> +    }
> +
> +    if (n->cntlid && !n->subsys) {
> +        error_setg(&local_err, "Secondary controller without subsystem");
> +        return NVME_NS_ATTACH_MGMT_NOTSPRD | NVME_DNR;

Leaking local_err. Any time you call error_setg(), the error needs to be 
reported or freed at some point.

> +    }
> +
> +    n_p = n->subsys->ctrls[0];
> +
> +    switch (sel) {
> +    case NVME_NS_MANAGEMENT_CREATE:
> +        switch (csi) {
> +        case NVME_CSI_NVM:

The following case is sufficiently large enough that the implementation should 
be its own function.

> +            if (nsid) {
> +                return NVME_INVALID_FIELD | NVME_DNR;
> +            }
> +
> +            ret = nvme_h2c(n, (uint8_t *)&id_ns, sizeof(id_ns), req);
> +            if (ret) {
> +                return ret;
> +            }
> +
> +            uint64_t nsze = le64_to_cpu(id_ns.nsze);
> +            uint64_t ncap = le64_to_cpu(id_ns.ncap);

Please don't mix declarations with code; declare these local variables at the 
top of the scope.

> +
> +            if (ncap > nsze) {
> +                return NVME_INVALID_FIELD | NVME_DNR;
> +            } else if (ncap != nsze) {
> +                return NVME_THIN_PROVISION_NOTSPRD | NVME_DNR;
> +            }
> +
> +            nvme_validate_flbas(id_ns.flbas, &local_err);
> +            if (local_err) {
> +                error_report_err(local_err);
> +                return NVME_INVALID_FORMAT | NVME_DNR;
> +            }
> +
> +            for (i = 1; i <= NVME_MAX_NAMESPACES; i++) {
> +                if (nvme_ns(n_p, (uint32_t)i) || nvme_subsys_ns(n_p->subsys, 
> (uint32_t)i)) {
> +                    continue;
> +                }
> +                break;
> +            }
> +
> +
> +            if (i >  le32_to_cpu(n_p->id_ctrl.nn) || i >  
> NVME_MAX_NAMESPACES) {
> +               return NVME_NS_IDNTIFIER_UNAVAIL | NVME_DNR;
> +            }
> +            nsid = i;
> +
> +            /* create ns here */
> +            ns = nvme_ns_mgmt_create(n, nsid, &id_ns, &local_err);
> +            if (!ns || local_err) {
> +                if (local_err) {
> +                    error_report_err(local_err);
> +                }
> +                return NVME_INVALID_FIELD | NVME_DNR;
> +            }
> +
> +            if (nvme_cfg_update(n, ns->size, NVME_NS_ALLOC_CHK)) {
> +                /* place for delete-ns */
> +                error_setg(&local_err, "Insufficient capacity, an orphaned 
> ns[%"PRIu32"] will be left behind", nsid);
> +                return NVME_NS_INSUFFICIENT_CAPAC | NVME_DNR;

Leaked local_err.

> +            }
> +            (void)nvme_cfg_update(n, ns->size, NVME_NS_ALLOC);
> +            if (nvme_cfg_save(n)) {
> +                (void)nvme_cfg_update(n, ns->size, NVME_NS_DEALLOC);
> +                /* place for delete-ns */
> +                error_setg(&local_err, "Cannot save conf file, an orphaned 
> ns[%"PRIu32"] will be left behind", nsid);
> +                return NVME_INVALID_FIELD | NVME_DNR;

Another leaked local_err.

> +            }
> +            req->cqe.result = cpu_to_le32(nsid);
> +            break;
> +        case NVME_CSI_ZONED:
> +            /* fall through for now */
> +        default:
> +            return NVME_INVALID_FIELD | NVME_DNR;
> +         }
> +        break;
> +    default:
> +        return NVME_INVALID_FIELD | NVME_DNR;
> +    }
> +
> +    return NVME_SUCCESS;
> +}