diff mbox series

[12/25] block/nvme: Make nvme_init_queue() return boolean indicating error

Message ID 20201027135547.374946-13-philmd@redhat.com
State Superseded
Headers show
Series block/nvme: Fix Aarch64 host | expand

Commit Message

Philippe Mathieu-Daudé Oct. 27, 2020, 1:55 p.m. UTC 
Just for consistency, following the example documented since
commit e3fe3988d7 ("error: Document Error API usage rules"),
return a boolean value indicating an error is set or not.
This simplifies a bit nvme_create_queue_pair().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

Eric Auger Oct. 28, 2020, 11:10 a.m. UTC | #1 
Hi,
On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> Just for consistency, following the example documented since
> commit e3fe3988d7 ("error: Document Error API usage rules"),
> return a boolean value indicating an error is set or not.
> This simplifies a bit nvme_create_queue_pair().
also directly pass errp as the local_err is not requested in our case.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 74994c442e5..9324f0bfdc4 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -160,7 +160,8 @@ static QemuOptsList runtime_opts = {
>      },
>  };
>  
> -static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
> +/* Returns true on success, false on failure. */
> +static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
>                              unsigned nentries, size_t entry_bytes, Error **errp)
>  {
>      size_t bytes;
> @@ -171,13 +172,14 @@ static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
I personally prefer returning a conventional int instead of bool.
>      q->queue = qemu_try_memalign(s->page_size, bytes);
>      if (!q->queue) {
>          error_setg(errp, "Cannot allocate queue");
> -        return;
> +        return false;
>      }
>      memset(q->queue, 0, bytes);
>      r = qemu_vfio_dma_map(s->vfio, q->queue, bytes, false, &q->iova);
>      if (r) {
>          error_setg(errp, "Cannot map queue");
>      }
> +    return r == 0;
also avoids that kind of conversion and use of !() in the called
>  }
>  
>  static void nvme_free_queue_pair(NVMeQueuePair *q)
> @@ -210,7 +212,6 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
>                                               Error **errp)
>  {
>      int i, r;
> -    Error *local_err = NULL;
>      NVMeQueuePair *q;
>      uint64_t prp_list_iova;
>  
> @@ -247,16 +248,12 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
>          req->prp_list_iova = prp_list_iova + i * s->page_size;
>      }
>  
> -    nvme_init_queue(s, &q->sq, size, NVME_SQ_ENTRY_BYTES, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (!nvme_init_queue(s, &q->sq, size, NVME_SQ_ENTRY_BYTES, errp)) {
>          goto fail;
>      }
>      q->sq.doorbell = &s->doorbells[idx * s->doorbell_scale].sq_tail;
>  
> -    nvme_init_queue(s, &q->cq, size, NVME_CQ_ENTRY_BYTES, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (!nvme_init_queue(s, &q->cq, size, NVME_CQ_ENTRY_BYTES, errp)) {
>          goto fail;
>      }
>      q->cq.doorbell = &s->doorbells[idx * s->doorbell_scale].cq_head;
> 
Thanks

Eric
Stefan Hajnoczi Oct. 28, 2020, 3:11 p.m. UTC | #2 
On Tue, Oct 27, 2020 at 02:55:34PM +0100, Philippe Mathieu-Daudé wrote:
> Just for consistency, following the example documented since
> commit e3fe3988d7 ("error: Document Error API usage rules"),
> return a boolean value indicating an error is set or not.
> This simplifies a bit nvme_create_queue_pair().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff mbox series

Patch

diff --git a/block/nvme.c b/block/nvme.c
index 74994c442e5..9324f0bfdc4 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -160,7 +160,8 @@  static QemuOptsList runtime_opts = {
     },
 };
 
-static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
+/* Returns true on success, false on failure. */
+static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
                             unsigned nentries, size_t entry_bytes, Error **errp)
 {
     size_t bytes;
@@ -171,13 +172,14 @@  static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
     q->queue = qemu_try_memalign(s->page_size, bytes);
     if (!q->queue) {
         error_setg(errp, "Cannot allocate queue");
-        return;
+        return false;
     }
     memset(q->queue, 0, bytes);
     r = qemu_vfio_dma_map(s->vfio, q->queue, bytes, false, &q->iova);
     if (r) {
         error_setg(errp, "Cannot map queue");
     }
+    return r == 0;
 }
 
 static void nvme_free_queue_pair(NVMeQueuePair *q)
@@ -210,7 +212,6 @@  static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
                                              Error **errp)
 {
     int i, r;
-    Error *local_err = NULL;
     NVMeQueuePair *q;
     uint64_t prp_list_iova;
 
@@ -247,16 +248,12 @@  static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
         req->prp_list_iova = prp_list_iova + i * s->page_size;
     }
 
-    nvme_init_queue(s, &q->sq, size, NVME_SQ_ENTRY_BYTES, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!nvme_init_queue(s, &q->sq, size, NVME_SQ_ENTRY_BYTES, errp)) {
         goto fail;
     }
     q->sq.doorbell = &s->doorbells[idx * s->doorbell_scale].sq_tail;
 
-    nvme_init_queue(s, &q->cq, size, NVME_CQ_ENTRY_BYTES, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!nvme_init_queue(s, &q->cq, size, NVME_CQ_ENTRY_BYTES, errp)) {
         goto fail;
     }
     q->cq.doorbell = &s->doorbells[idx * s->doorbell_scale].cq_head;