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

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

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
>>
>>
>>
>>
>
>
> .
>