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

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

[zhanghailiang]

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 <zhang.zhanghailiang@huawei.com>
---
 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);
 }
 
-- 
1.7.12.4

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

[Luiz Capitulino]

On Wed, 27 Aug 2014 19:18:53 +0800
zhanghailiang <zhang.zhanghailiang@huawei.com> 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 <zhang.zhanghailiang@huawei.com>
> ---
>  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().

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

[zhanghailiang]

On 2014/8/27 21:18, Luiz Capitulino wrote:
> On Wed, 27 Aug 2014 19:18:53 +0800
> zhanghailiang<zhang.zhanghailiang@huawei.com>  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<zhang.zhanghailiang@huawei.com>
>> ---
>>   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?

What's your suggestion? Thanks.:)

Best Regards,
zhanghailiang

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

[Luiz Capitulino]

On Fri, 29 Aug 2014 16:06:18 +0800
zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:

> On 2014/8/27 21:18, Luiz Capitulino wrote:
> > On Wed, 27 Aug 2014 19:18:53 +0800
> > zhanghailiang<zhang.zhanghailiang@huawei.com>  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<zhang.zhanghailiang@huawei.com>
> >> ---
> >>   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
> 
> 
> 
> 

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

[zhanghailiang]

On 2014/8/29 20:55, Luiz Capitulino wrote:
> On Fri, 29 Aug 2014 16:06:18 +0800
> zhanghailiang<zhang.zhanghailiang@huawei.com>  wrote:
>
>> On 2014/8/27 21:18, Luiz Capitulino wrote:
>>> On Wed, 27 Aug 2014 19:18:53 +0800
>>> zhanghailiang<zhang.zhanghailiang@huawei.com>   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<zhang.zhanghailiang@huawei.com>
>>>> ---
>>>>    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.
>

OK, I will fix this and submit the second version. Thanks:)

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