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; > +}