[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 1/2] hw/nvme: Support for Namespaces Management from guest
From: |
Keith Busch |
Subject: |
Re: [PATCH v4 1/2] hw/nvme: Support for Namespaces Management from guest OS - create-ns |
Date: |
Wed, 28 Dec 2022 13:10:05 -0700 |
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;
> +}