Re: [Qemu-devel] [PATCH] dump: let dump_error printf the error reason

[Luiz Capitulino]

On Fri, 29 Aug 2014 16:06:18 +0800
zhanghailiang <address@hidden> wrote:

> On 2014/8/27 21:18, Luiz Capitulino wrote:
> > On Wed, 27 Aug 2014 19:18:53 +0800
> > zhanghailiang<address@hidden>  wrote:
> >
> >> The second parameter of dump_error is unused, but one purpose of
> >> using this function is to report the error info.
> >>
> >> Signed-off-by: zhanghailiang<address@hidden>
> >> ---
> >>   dump.c | 3 +++
> >>   1 file changed, 3 insertions(+)
> >>
> >> diff --git a/dump.c b/dump.c
> >> index 71d3e94..0f44e9d 100644
> >> --- a/dump.c
> >> +++ b/dump.c
> >> @@ -83,6 +83,9 @@ static int dump_cleanup(DumpState *s)
> >>
> >>   static void dump_error(DumpState *s, const char *reason)
> >>   {
> >> +    if (reason) {
> >> +        error_report("%s", reason);
> >> +    }
> >>       dump_cleanup(s);
> >>   }
> >>
> >
> > Good catch, but error_report() will report the error only to the user. This
> > is QMP code, so we have to use the Error API.
> >
> > I think that the best way to solve this is to make dump_error() fill an
> > Error object (eg. by calling error_setg()) and then returning it after
> > the call to dump_cleanup(). Of course that you will have to change _all_
> > code paths calling dump_error() to propagate the error up.
> >
> > For more information on this, please read docs/writing-qmp-commands.txt.
> > You can also take a look at simple commands doing error propagation, like
> > qmp_cont() or qmp_block_passwd().
> >
> > .
> >
> Hi,
> 
> Thanks for your review.
> 
> Actually, for all paths that call dump_error, at last they will come into a
> common path which will call error_setg().
> 
> The call process as below:
> qmp_dump_guest_memory
> (1)   -->create_kdump_vmcore
>               -->write_start_flat_header
>                       -->dump_error
>               -->write_end_flat_header
>                       -->dump_error
>               ...
>          -->error_set(errp, QERR_IO_ERROR)(if create_kdump_vmcore failed)
> (2)   -->create_vmcore
>               -->dump_begin
>                       -->write_elf64_header
>                               -->dump_error
>                       -->write_elf64_note
>                               -->dump_error
>                       ...
>               -->dump_iterate
>                       -->write_data
>                               -->dump_error
>                       ...
>       -->error_set(errp, QERR_IO_ERROR)(if create_kdump_vmcore failed)
> 
> And a short *IO ERROR* info will be returned to the caller of 
> qmp_dump_guest_memory.
> 
> So, is it OK in dump_error just report the detailed error info to users
> (actually, it will be stored in qemu log)? Or should these error info
> also returned to the caller?

The errors should be propagated up to the caller. This way they can be
consumed by QMP and HMP.

> 
> What's your suggestion? Thanks.:)
> 
> Best Regards,
> zhanghailiang
> 
> 
> 
>