diff mbox series

[v2,09/16] nvme: factor out property/constraint checks

Message ID 20200415130159.611361-10-its@irrelevant.dk (mailing list archive)
State New, archived
Headers show
Series nvme: refactoring and cleanups | expand

Commit Message

Klaus Jensen April 15, 2020, 1:01 p.m. UTC 
From: Klaus Jensen <k.jensen@samsung.com>

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

Comments

Philippe Mathieu-Daudé April 15, 2020, 1:08 p.m. UTC | #1 
On 4/15/20 3:01 PM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/block/nvme.c | 43 ++++++++++++++++++++++++++++---------------
>   1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 44856e873fd1..5f9ebbd6a1d5 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1311,24 +1311,19 @@ static const MemoryRegionOps nvme_cmb_ops = {
>       },
>   };
>   
> -static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> +static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
>   {
> -    NvmeCtrl *n = NVME(pci_dev);
> -    NvmeIdCtrl *id = &n->id_ctrl;
> +    NvmeParams *params = &n->params;
>   
> -    int i;
> -    int64_t bs_size;
> -    uint8_t *pci_conf;
> -
> -    if (n->params.num_queues) {
> +    if (params->num_queues) {
>           warn_report("num_queues is deprecated; please use max_ioqpairs "
>                       "instead");
>   
> -        n->params.max_ioqpairs = n->params.num_queues - 1;
> +        params->max_ioqpairs = params->num_queues - 1;
>       }
>   
> -    if (n->params.max_ioqpairs < 1 ||
> -        n->params.max_ioqpairs > PCI_MSIX_FLAGS_QSIZE) {
> +    if (params->max_ioqpairs < 1 ||
> +        params->max_ioqpairs > PCI_MSIX_FLAGS_QSIZE) {
>           error_setg(errp, "max_ioqpairs must be between 1 and %d",
>                      PCI_MSIX_FLAGS_QSIZE);
>           return;
> @@ -1339,16 +1334,34 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>           return;
>       }
>   
> +    if (!params->serial) {
> +        error_setg(errp, "serial property not set");
> +        return;
> +    }
> +}
> +
> +static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> +{
> +    NvmeCtrl *n = NVME(pci_dev);
> +    NvmeIdCtrl *id = &n->id_ctrl;
> +    Error *err = NULL;

Nitpick if you ever have to respin, name this 'local_err'.

> +
> +    int i;
> +    int64_t bs_size;
> +    uint8_t *pci_conf;
> +
> +    nvme_check_constraints(n, &err);
> +    if (err) {
> +        error_propagate(errp, err);

Thanks :)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +        return;
> +    }
> +
>       bs_size = blk_getlength(n->conf.blk);
>       if (bs_size < 0) {
>           error_setg(errp, "could not get backing file size");
>           return;
>       }
>   
> -    if (!n->params.serial) {
> -        error_setg(errp, "serial property not set");
> -        return;
> -    }
>       blkconf_blocksizes(&n->conf);
>       if (!blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
>                                          false, errp)) {
>
Maxim Levitsky April 21, 2020, 2:53 p.m. UTC | #2 
On Wed, 2020-04-15 at 15:01 +0200, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c | 43 ++++++++++++++++++++++++++++---------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 44856e873fd1..5f9ebbd6a1d5 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1311,24 +1311,19 @@ static const MemoryRegionOps nvme_cmb_ops = {
>      },
>  };
>  
> -static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> +static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
>  {
> -    NvmeCtrl *n = NVME(pci_dev);
> -    NvmeIdCtrl *id = &n->id_ctrl;
> +    NvmeParams *params = &n->params;
>  
> -    int i;
> -    int64_t bs_size;
> -    uint8_t *pci_conf;
> -
> -    if (n->params.num_queues) {
> +    if (params->num_queues) {
>          warn_report("num_queues is deprecated; please use max_ioqpairs "
>                      "instead");
>  
> -        n->params.max_ioqpairs = n->params.num_queues - 1;
> +        params->max_ioqpairs = params->num_queues - 1;
>      }
>  
> -    if (n->params.max_ioqpairs < 1 ||
> -        n->params.max_ioqpairs > PCI_MSIX_FLAGS_QSIZE) {
> +    if (params->max_ioqpairs < 1 ||
> +        params->max_ioqpairs > PCI_MSIX_FLAGS_QSIZE) {
>          error_setg(errp, "max_ioqpairs must be between 1 and %d",
>                     PCI_MSIX_FLAGS_QSIZE);
>          return;
> @@ -1339,16 +1334,34 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>          return;
>      }
>  
> +    if (!params->serial) {
> +        error_setg(errp, "serial property not set");
> +        return;
> +    }
> +}
> +
> +static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> +{
> +    NvmeCtrl *n = NVME(pci_dev);
> +    NvmeIdCtrl *id = &n->id_ctrl;
> +    Error *err = NULL;
Yep, lets name it indeed local_err.
> +
> +    int i;
> +    int64_t bs_size;
> +    uint8_t *pci_conf;
> +
> +    nvme_check_constraints(n, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
>      bs_size = blk_getlength(n->conf.blk);
>      if (bs_size < 0) {
>          error_setg(errp, "could not get backing file size");
>          return;
>      }
>  
> -    if (!n->params.serial) {
> -        error_setg(errp, "serial property not set");
> -        return;
> -    }
>      blkconf_blocksizes(&n->conf);
>      if (!blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
>                                         false, errp)) {
 

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky
diff mbox series

Patch

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 44856e873fd1..5f9ebbd6a1d5 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1311,24 +1311,19 @@  static const MemoryRegionOps nvme_cmb_ops = {
     },
 };
 
-static void nvme_realize(PCIDevice *pci_dev, Error **errp)
+static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
 {
-    NvmeCtrl *n = NVME(pci_dev);
-    NvmeIdCtrl *id = &n->id_ctrl;
+    NvmeParams *params = &n->params;
 
-    int i;
-    int64_t bs_size;
-    uint8_t *pci_conf;
-
-    if (n->params.num_queues) {
+    if (params->num_queues) {
         warn_report("num_queues is deprecated; please use max_ioqpairs "
                     "instead");
 
-        n->params.max_ioqpairs = n->params.num_queues - 1;
+        params->max_ioqpairs = params->num_queues - 1;
     }
 
-    if (n->params.max_ioqpairs < 1 ||
-        n->params.max_ioqpairs > PCI_MSIX_FLAGS_QSIZE) {
+    if (params->max_ioqpairs < 1 ||
+        params->max_ioqpairs > PCI_MSIX_FLAGS_QSIZE) {
         error_setg(errp, "max_ioqpairs must be between 1 and %d",
                    PCI_MSIX_FLAGS_QSIZE);
         return;
@@ -1339,16 +1334,34 @@  static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         return;
     }
 
+    if (!params->serial) {
+        error_setg(errp, "serial property not set");
+        return;
+    }
+}
+
+static void nvme_realize(PCIDevice *pci_dev, Error **errp)
+{
+    NvmeCtrl *n = NVME(pci_dev);
+    NvmeIdCtrl *id = &n->id_ctrl;
+    Error *err = NULL;
+
+    int i;
+    int64_t bs_size;
+    uint8_t *pci_conf;
+
+    nvme_check_constraints(n, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
     bs_size = blk_getlength(n->conf.blk);
     if (bs_size < 0) {
         error_setg(errp, "could not get backing file size");
         return;
     }
 
-    if (!n->params.serial) {
-        error_setg(errp, "serial property not set");
-        return;
-    }
     blkconf_blocksizes(&n->conf);
     if (!blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
                                        false, errp)) {