Re: [Qemu-devel] [PATCH 3/7] qdev_prop_parse(): report errors via Error

[Laszlo Ersek]

On 02/05/13 13:20, Luiz Capitulino wrote:
> On Tue, 05 Feb 2013 01:31:14 +0100
> Laszlo Ersek <address@hidden> wrote:
> 
>> On 02/04/13 18:27, Luiz Capitulino wrote:
>>>
>>> I usually split this kind of patch the following way:
>>>
>>>  1. add an Error ** argument to the function reporting the error. Callers
>>>     are changed to pass NULL for the new argument
>>
>> If the called function already switches to error_set() /
>> error_propagate() at this point, then standing at this patch in a
>> possible bisection, the error message is lost. (The caller doesn't print
>> it *yet*, the callee doesn't print it *any longer*.)
> 
> If I got what you meant, the called function shouldn't be using error_set()
> at this point, as it doesn't even take an Error ** argument.
> 
>> If, OTOH, the called function still prints the error message here, and
>> doesn't pass it up (only its signature has changed), then:
>>
>>>  2. Handling of the new error is added for each caller in a different
>>>     patch (it's OK to group callers when the error handling is the same)
>>
>> at some point there will be callers that need the callee to pass up the
>> error, and other callers (the ones not yet converted) that want the
>> callee not to.
> 
> Yes, but it's case-by-case. For example, if the called function prints to
> the monitor or uses fprintf(), then it's OK to keep them until all callers
> are converted.
> 
> Now, if the called function reports error with qerror_report() then
> we have different options. You could call qerror_report_err() at some
> point in the call stack, for example.
> 
> But, honestly speaking, I think I went too general on my feedback and
> lost sight of the details concerning this patch and I don't my feedback
> is good enough at this point, sorry for that. I'll try to be more
> specific when you respin.

OK, let me post v2 soon. I don't expect it to be final, but hopefully
it'll enable us to discuss it in more targeted details.

Thanks much!
Laszlo