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 <gr...@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,
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
