On Fri, 11 Oct 2019 19:04:51 +0300
Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> wrote:
> If we want to add some info to errp (by error_prepend() or
> error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
> Otherwise, this info will not be added when errp == &fatal_err
> (the program will exit prior to the error_append_hint() or
> error_prepend() call). Fix such cases.
>
> If we want to check error after errp-function call, we need to
> introduce local_err and than propagate it to errp. Instead, use
> ERRP_AUTO_PROPAGATE macro, benefits are:
> 1. No need of explicit error_propagate call
> 2. No need of explicit local_err variable: use errp directly
> 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
> &error_fatel, this means that we don't break error_abort
> (we'll abort on error_set, not on error_propagate)
>
> This commit (together with its neighbors) was generated by
>
> for f in $(git grep -l errp \*.[ch]); do \
> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
> --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
> done;
>
> then fix a bit of compilation problems: coccinelle for some reason
> leaves several
> f() {
> ...
> goto out;
> ...
> out:
> }
> patterns, with "out:" at function end.
>
> then
> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
>
> (auto-msg was a file with this commit message)
>
> Still, for backporting it may be more comfortable to use only the first
> command and then do one huge commit.
>
> Reported-by: Kevin Wolf <kw...@redhat.com>
> Reported-by: Greg Kurz <gr...@kaod.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> ---
> hw/intc/spapr_xive.c | 12 ++++-----
> hw/intc/spapr_xive_kvm.c | 55 ++++++++++++++++++----------------------
> hw/intc/xive.c | 27 ++++++++------------
> 3 files changed, 40 insertions(+), 54 deletions(-)
>
We did a huge cleanup recently in this area so this patch doesn't apply
anymore. Ideally it should be based on David Gibson's ppc-for-5.0 branch
available at https://github.com/dgibson/qemu .
Same goes for the PowerPC patch 035/126 actually.
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 04879abf2e..b25c9ef9ea 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -273,10 +273,10 @@ static void spapr_xive_instance_init(Object *obj)
>
> static void spapr_xive_realize(DeviceState *dev, Error **errp)
> {
> + ERRP_AUTO_PROPAGATE();
> SpaprXive *xive = SPAPR_XIVE(dev);
> XiveSource *xsrc = &xive->source;
> XiveENDSource *end_xsrc = &xive->end_source;
> - Error *local_err = NULL;
>
> if (!xive->nr_irqs) {
> error_setg(errp, "Number of interrupt needs to be greater 0");
> @@ -295,9 +295,8 @@ static void spapr_xive_realize(DeviceState *dev, Error
> **errp)
> &error_fatal);
> object_property_add_const_link(OBJECT(xsrc), "xive", OBJECT(xive),
> &error_fatal);
> - object_property_set_bool(OBJECT(xsrc), true, "realized", &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> + object_property_set_bool(OBJECT(xsrc), true, "realized", errp);
> + if (*errp) {
> return;
> }
> sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xsrc->esb_mmio);
> @@ -309,9 +308,8 @@ static void spapr_xive_realize(DeviceState *dev, Error
> **errp)
> &error_fatal);
> object_property_add_const_link(OBJECT(end_xsrc), "xive", OBJECT(xive),
> &error_fatal);
> - object_property_set_bool(OBJECT(end_xsrc), true, "realized", &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> + object_property_set_bool(OBJECT(end_xsrc), true, "realized", errp);
> + if (*errp) {
> return;
> }
> sysbus_init_mmio(SYS_BUS_DEVICE(xive), &end_xsrc->esb_mmio);
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 51b334b676..02243537e6 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -186,6 +186,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
> void kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS
> *eas,
> Error **errp)
> {
> + ERRP_AUTO_PROPAGATE();
> uint32_t end_idx;
> uint32_t end_blk;
> uint8_t priority;
> @@ -193,7 +194,6 @@ void kvmppc_xive_set_source_config(SpaprXive *xive,
> uint32_t lisn, XiveEAS *eas,
> bool masked;
> uint32_t eisn;
> uint64_t kvm_src;
> - Error *local_err = NULL;
>
> assert(xive_eas_is_valid(eas));
>
> @@ -214,9 +214,8 @@ void kvmppc_xive_set_source_config(SpaprXive *xive,
> uint32_t lisn, XiveEAS *eas,
> KVM_XIVE_SOURCE_EISN_MASK;
>
> kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_CONFIG, lisn,
> - &kvm_src, true, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> + &kvm_src, true, errp);
> + if (*errp) {
> return;
> }
> }
> @@ -255,19 +254,17 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int
> srcno, Error **errp)
>
> static void kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp)
> {
> + ERRP_AUTO_PROPAGATE();
> SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
> int i;
>
> for (i = 0; i < xsrc->nr_irqs; i++) {
> - Error *local_err = NULL;
> -
> if (!xive_eas_is_valid(&xive->eat[i])) {
> continue;
> }
>
> - kvmppc_xive_source_reset_one(xsrc, i, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> + kvmppc_xive_source_reset_one(xsrc, i, errp);
> + if (*errp) {
> return;
> }
> }
> @@ -389,11 +386,11 @@ void kvmppc_xive_get_queue_config(SpaprXive *xive,
> uint8_t end_blk,
> uint32_t end_idx, XiveEND *end,
> Error **errp)
> {
> + ERRP_AUTO_PROPAGATE();
> struct kvm_ppc_xive_eq kvm_eq = { 0 };
> uint64_t kvm_eq_idx;
> uint8_t priority;
> uint32_t server;
> - Error *local_err = NULL;
>
> assert(xive_end_is_valid(end));
>
> @@ -406,9 +403,8 @@ void kvmppc_xive_get_queue_config(SpaprXive *xive,
> uint8_t end_blk,
> KVM_XIVE_EQ_SERVER_MASK;
>
> kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_EQ_CONFIG, kvm_eq_idx,
> - &kvm_eq, false, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> + &kvm_eq, false, errp);
> + if (*errp) {
> return;
> }
>
> @@ -425,11 +421,11 @@ void kvmppc_xive_set_queue_config(SpaprXive *xive,
> uint8_t end_blk,
> uint32_t end_idx, XiveEND *end,
> Error **errp)
> {
> + ERRP_AUTO_PROPAGATE();
> struct kvm_ppc_xive_eq kvm_eq = { 0 };
> uint64_t kvm_eq_idx;
> uint8_t priority;
> uint32_t server;
> - Error *local_err = NULL;
>
> /*
> * Build the KVM state from the local END structure.
> @@ -468,9 +464,8 @@ void kvmppc_xive_set_queue_config(SpaprXive *xive,
> uint8_t end_blk,
> KVM_XIVE_EQ_SERVER_MASK;
>
> kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_EQ_CONFIG, kvm_eq_idx,
> - &kvm_eq, true, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> + &kvm_eq, true, errp);
> + if (*errp) {
> return;
> }
> }
> @@ -483,7 +478,7 @@ void kvmppc_xive_reset(SpaprXive *xive, Error **errp)
>
> static void kvmppc_xive_get_queues(SpaprXive *xive, Error **errp)
> {
> - Error *local_err = NULL;
> + ERRP_AUTO_PROPAGATE();
> int i;
>
> for (i = 0; i < xive->nr_ends; i++) {
> @@ -492,9 +487,8 @@ static void kvmppc_xive_get_queues(SpaprXive *xive, Error
> **errp)
> }
>
> kvmppc_xive_get_queue_config(xive, SPAPR_XIVE_BLOCK_ID, i,
> - &xive->endt[i], &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> + &xive->endt[i], errp);
> + if (*errp) {
> return;
> }
> }
> @@ -742,8 +736,8 @@ static void *kvmppc_xive_mmap(SpaprXive *xive, int pgoff,
> size_t len,
> */
> void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
> {
> + ERRP_AUTO_PROPAGATE();
> XiveSource *xsrc = &xive->source;
> - Error *local_err = NULL;
> size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
> size_t tima_len = 4ull << TM_SHIFT;
> CPUState *cs;
> @@ -772,8 +766,8 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
> * 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) {
> + errp);
> + if (*errp) {
> goto fail;
> }
>
> @@ -790,8 +784,8 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
> * 3. TIMA pages - KVM mapping
> */
> xive->tm_mmap = kvmppc_xive_mmap(xive, KVM_XIVE_TIMA_PAGE_OFFSET,
> tima_len,
> - &local_err);
> - if (local_err) {
> + errp);
> + if (*errp) {
> goto fail;
> }
> memory_region_init_ram_device_ptr(&xive->tm_mmio_kvm, OBJECT(xive),
> @@ -806,15 +800,15 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
> CPU_FOREACH(cs) {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
>
> - kvmppc_xive_cpu_connect(spapr_cpu_state(cpu)->tctx, &local_err);
> - if (local_err) {
> + kvmppc_xive_cpu_connect(spapr_cpu_state(cpu)->tctx, errp);
> + if (*errp) {
> goto fail;
> }
> }
>
> /* Update the KVM sources */
> - kvmppc_xive_source_reset(xsrc, &local_err);
> - if (local_err) {
> + kvmppc_xive_source_reset(xsrc, errp);
> + if (*errp) {
> goto fail;
> }
>
> @@ -824,7 +818,6 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
> return;
>
> fail:
> - error_propagate(errp, local_err);
> kvmppc_xive_disconnect(xive, NULL);
> }
>
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 29df06df11..368f94df71 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -570,15 +570,14 @@ static void xive_tctx_reset(void *dev)
>
> static void xive_tctx_realize(DeviceState *dev, Error **errp)
> {
> + ERRP_AUTO_PROPAGATE();
> XiveTCTX *tctx = XIVE_TCTX(dev);
> PowerPCCPU *cpu;
> CPUPPCState *env;
> Object *obj;
> - Error *local_err = NULL;
>
> - obj = object_property_get_link(OBJECT(dev), "cpu", &local_err);
> + obj = object_property_get_link(OBJECT(dev), "cpu", errp);
> if (!obj) {
> - error_propagate(errp, local_err);
> error_prepend(errp, "required link 'cpu' not found: ");
> return;
> }
> @@ -601,9 +600,8 @@ static void xive_tctx_realize(DeviceState *dev, Error
> **errp)
>
> /* Connect the presenter to the VCPU (required for CPU hotplug) */
> if (kvm_irqchip_in_kernel()) {
> - kvmppc_xive_cpu_connect(tctx, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> + kvmppc_xive_cpu_connect(tctx, errp);
> + if (*errp) {
> return;
> }
> }
> @@ -681,15 +679,15 @@ static const TypeInfo xive_tctx_info = {
>
> Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
> {
> - Error *local_err = NULL;
> + ERRP_AUTO_PROPAGATE();
> Object *obj;
>
> obj = object_new(TYPE_XIVE_TCTX);
> object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
> object_unref(obj);
> object_property_add_const_link(obj, "cpu", cpu, &error_abort);
> - object_property_set_bool(obj, true, "realized", &local_err);
> - if (local_err) {
> + object_property_set_bool(obj, true, "realized", errp);
> + if (*errp) {
> goto error;
> }
>
> @@ -697,7 +695,6 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr,
> Error **errp)
>
> error:
> object_unparent(obj);
> - error_propagate(errp, local_err);
> return NULL;
> }
>
> @@ -1050,13 +1047,12 @@ static void xive_source_reset(void *dev)
>
> static void xive_source_realize(DeviceState *dev, Error **errp)
> {
> + ERRP_AUTO_PROPAGATE();
> XiveSource *xsrc = XIVE_SOURCE(dev);
> Object *obj;
> - Error *local_err = NULL;
>
> - obj = object_property_get_link(OBJECT(dev), "xive", &local_err);
> + obj = object_property_get_link(OBJECT(dev), "xive", errp);
> if (!obj) {
> - error_propagate(errp, local_err);
> error_prepend(errp, "required link 'xive' not found: ");
> return;
> }
> @@ -1806,13 +1802,12 @@ static const MemoryRegionOps xive_end_source_ops = {
>
> static void xive_end_source_realize(DeviceState *dev, Error **errp)
> {
> + ERRP_AUTO_PROPAGATE();
> XiveENDSource *xsrc = XIVE_END_SOURCE(dev);
> Object *obj;
> - Error *local_err = NULL;
>
> - obj = object_property_get_link(OBJECT(dev), "xive", &local_err);
> + obj = object_property_get_link(OBJECT(dev), "xive", errp);
> if (!obj) {
> - error_propagate(errp, local_err);
> error_prepend(errp, "required link 'xive' not found: ");
> return;
> }
