Re: [Qemu-devel] [PATCH RFC v5 3/7] qemu_init_vcpu: add a new Error parameter to propagate

[Fei Li]

On 10/12/2018 04:24 PM, Markus Armbruster wrote:
> Fei Li <address@hidden> writes:
>
> > On 10/11/2018 09:19 PM, Markus Armbruster wrote:
> > > Fei Li <address@hidden> writes:
> > >
> > > > The caller of qemu_init_vcpu() already passed the **errp to handle
> > > Which caller?  There are many.  Or do you mean "The callers"?
> > Oh, sorry, I mean "The callers" :)
> > > > errors. In view of this, add a new Error parameter to the following
> > > > call trace to propagate the error and let the further caller check it.
> > > Which "call trace"?
> > Actually, I want to say all functions called by qemu_init_vcpu()..
> > > > Besides, make qemu_init_vcpu() return a Boolean value to let its
> > > > callers know whether it succeeds.
> > > >
> > > > Signed-off-by: Fei Li <address@hidden>
> > > > Reviewed-by: Fam Zheng <address@hidden>
> > > > ---
> > > >    accel/tcg/user-exec-stub.c      |  2 +-
> > > >    cpus.c                          | 34 +++++++++++++++++++++-------------
> > > >    include/qom/cpu.h               |  2 +-
> > > >    target/alpha/cpu.c              |  4 +++-
> > > >    target/arm/cpu.c                |  4 +++-
> > > >    target/cris/cpu.c               |  4 +++-
> > > >    target/hppa/cpu.c               |  4 +++-
> > > >    target/i386/cpu.c               |  4 +++-
> > > >    target/lm32/cpu.c               |  4 +++-
> > > >    target/m68k/cpu.c               |  4 +++-
> > > >    target/microblaze/cpu.c         |  4 +++-
> > > >    target/mips/cpu.c               |  4 +++-
> > > >    target/moxie/cpu.c              |  4 +++-
> > > >    target/nios2/cpu.c              |  4 +++-
> > > >    target/openrisc/cpu.c           |  4 +++-
> > > >    target/ppc/translate_init.inc.c |  4 +++-
> > > >    target/riscv/cpu.c              |  4 +++-
> > > >    target/s390x/cpu.c              |  4 +++-
> > > >    target/sh4/cpu.c                |  4 +++-
> > > >    target/sparc/cpu.c              |  4 +++-
> > > >    target/tilegx/cpu.c             |  4 +++-
> > > >    target/tricore/cpu.c            |  4 +++-
> > > >    target/unicore32/cpu.c          |  4 +++-
> > > >    target/xtensa/cpu.c             |  4 +++-
> > > >    24 files changed, 86 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
> > > > index a32b4496af..38f6b928d4 100644
> > > > --- a/accel/tcg/user-exec-stub.c
> > > > +++ b/accel/tcg/user-exec-stub.c
> > > > @@ -10,7 +10,7 @@ void cpu_resume(CPUState *cpu)
> > > >    {
> > > >    }
> > > >    -void qemu_init_vcpu(CPUState *cpu)
> > > > +bool qemu_init_vcpu(CPUState *cpu, Error **errp)
> > > >    {
> > > You need to return a value here.  Sure you compile-tested this?
> > Oops! I forget the TCG case.. The `return true` should be added.
> > Thanks for pointing this out!
> You're welcome.
>
> > > >    }
> > > >    diff --git a/cpus.c b/cpus.c
> [...]
> > > I see how you changed the code to pass an Error from the
> > > qemu_FOO_start_vcpu() through qemu_init_vcpu() to its callers.  I can't
> > > see how such errors can be created.  Without a way to create any, the
> > > patch is pointless.  What am I missing?
> > This patch is also the pre-patch for the updated qemu_thread_create()
> > in patch 7/7
> > just as I explained in patch 2/7 [Issue1].
> Patches that make little sense on their own, but pave the way for a
> later patch are okay.  But they should explain that purpose in their
> commit message.
Right. Thanks for the advice, will amend the commit message in next version.

Have a nice day
Fei