| Message ID | 159707845972.1489912.719896767746375765.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc/spapr: Error handling fixes and cleanups | expand |
On Mon, Aug 10, 2020 at 06:54:19PM +0200, Greg Kurz wrote: > Callers currently check failures of kvmppc_xive_mmap() through the > @errp argument, which isn't a recommanded practice. It is preferred > to use a return value when possible. > > Since NULL isn't an invalid address in theory, it seems better to > return MAP_FAILED and to teach callers to handle it. Heh... there's a world of C and Unix subtleties buried in that assertion. But, given that it's a function named ..._mmap() using MAP_FAILED isn't a bad option. Applied to ppc-for-5.2. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/intc/spapr_xive_kvm.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c > index d801bf5cd11c..b2a36fd59dae 100644 > --- a/hw/intc/spapr_xive_kvm.c > +++ b/hw/intc/spapr_xive_kvm.c > @@ -698,6 +698,7 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id) > return 0; > } > > +/* Returns MAP_FAILED on error and sets errno */ > static void *kvmppc_xive_mmap(SpaprXive *xive, int pgoff, size_t len, > Error **errp) > { > @@ -708,7 +709,6 @@ static void *kvmppc_xive_mmap(SpaprXive *xive, int pgoff, size_t len, > pgoff << page_shift); > if (addr == MAP_FAILED) { > error_setg_errno(errp, errno, "XIVE: unable to set memory mapping"); > - return NULL; > } > > return addr; > @@ -728,6 +728,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers, > size_t tima_len = 4ull << TM_SHIFT; > CPUState *cs; > int fd; > + void *addr; > > /* > * The KVM XIVE device already in use. This is the case when > @@ -763,11 +764,12 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers, > /* > * 1. Source ESB pages - KVM mapping > */ > - xsrc->esb_mmap = kvmppc_xive_mmap(xive, KVM_XIVE_ESB_PAGE_OFFSET, esb_len, > - &local_err); > - if (local_err) { > + addr = kvmppc_xive_mmap(xive, KVM_XIVE_ESB_PAGE_OFFSET, esb_len, > + &local_err); > + if (addr == MAP_FAILED) { > goto fail; > } > + xsrc->esb_mmap = addr; > > memory_region_init_ram_device_ptr(&xsrc->esb_mmio_kvm, OBJECT(xsrc), > "xive.esb-kvm", esb_len, xsrc->esb_mmap); > @@ -781,11 +783,13 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers, > /* > * 3. TIMA pages - KVM mapping > */ > - xive->tm_mmap = kvmppc_xive_mmap(xive, KVM_XIVE_TIMA_PAGE_OFFSET, tima_len, > - &local_err); > - if (local_err) { > + addr = kvmppc_xive_mmap(xive, KVM_XIVE_TIMA_PAGE_OFFSET, tima_len, > + &local_err); > + if (addr == MAP_FAILED) { > goto fail; > } > + xive->tm_mmap = addr; > + > memory_region_init_ram_device_ptr(&xive->tm_mmio_kvm, OBJECT(xive), > "xive.tima", tima_len, xive->tm_mmap); > memory_region_add_subregion_overlap(&xive->tm_mmio, 0, > >
diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c index d801bf5cd11c..b2a36fd59dae 100644 --- a/hw/intc/spapr_xive_kvm.c +++ b/hw/intc/spapr_xive_kvm.c @@ -698,6 +698,7 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id) return 0; } +/* Returns MAP_FAILED on error and sets errno */ static void *kvmppc_xive_mmap(SpaprXive *xive, int pgoff, size_t len, Error **errp) { @@ -708,7 +709,6 @@ static void *kvmppc_xive_mmap(SpaprXive *xive, int pgoff, size_t len, pgoff << page_shift); if (addr == MAP_FAILED) { error_setg_errno(errp, errno, "XIVE: unable to set memory mapping"); - return NULL; } return addr; @@ -728,6 +728,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers, size_t tima_len = 4ull << TM_SHIFT; CPUState *cs; int fd; + void *addr; /* * The KVM XIVE device already in use. This is the case when @@ -763,11 +764,12 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers, /* * 1. Source ESB pages - KVM mapping */ - xsrc->esb_mmap = kvmppc_xive_mmap(xive, KVM_XIVE_ESB_PAGE_OFFSET, esb_len, - &local_err); - if (local_err) { + addr = kvmppc_xive_mmap(xive, KVM_XIVE_ESB_PAGE_OFFSET, esb_len, + &local_err); + if (addr == MAP_FAILED) { goto fail; } + xsrc->esb_mmap = addr; memory_region_init_ram_device_ptr(&xsrc->esb_mmio_kvm, OBJECT(xsrc), "xive.esb-kvm", esb_len, xsrc->esb_mmap); @@ -781,11 +783,13 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers, /* * 3. TIMA pages - KVM mapping */ - xive->tm_mmap = kvmppc_xive_mmap(xive, KVM_XIVE_TIMA_PAGE_OFFSET, tima_len, - &local_err); - if (local_err) { + addr = kvmppc_xive_mmap(xive, KVM_XIVE_TIMA_PAGE_OFFSET, tima_len, + &local_err); + if (addr == MAP_FAILED) { goto fail; } + xive->tm_mmap = addr; + memory_region_init_ram_device_ptr(&xive->tm_mmio_kvm, OBJECT(xive), "xive.tima", tima_len, xive->tm_mmap); memory_region_add_subregion_overlap(&xive->tm_mmio, 0,
Callers currently check failures of kvmppc_xive_mmap() through the @errp argument, which isn't a recommanded practice. It is preferred to use a return value when possible. Since NULL isn't an invalid address in theory, it seems better to return MAP_FAILED and to teach callers to handle it. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/intc/spapr_xive_kvm.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)