| Message ID | 20201027135547.374946-13-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/nvme: Fix Aarch64 host | expand |
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
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 --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;
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(-)