Re: [Qemu-devel] [PATCH 03/22] target-i386: split out CPU creation and features parsing into cpu_x86_create()

[Andreas Färber]

Am 09.04.2013 12:33, schrieb Igor Mammedov:
> On Tue, 09 Apr 2013 12:30:21 +0200
> Paolo Bonzini <address@hidden> wrote:
> 
>> Il 08/04/2013 20:30, Eduardo Habkost ha scritto:
>>>>> -    cpu_x86_register(cpu, name, &error);
>>>>> -    if (error) {
>>>>> +    cpu_x86_register(cpu, name, errp);
>>>>> +    if (error_is_set(errp)) {
>>> So the function now does error checking properly if and only if errp is
>>> not NULL. Do we really want to do that?
>>
>> No, using error_propagate is the correct idiom indeed.
> Ok, I'll use  error_propagate() in next version.

I remember accepting some CPU refactoring patch but asking you to fix up
the same issue as follow-up - I don't remember having received a single
fix-up patch, could you please check on whether that is hidden in some
series or still missing? Thanks!

Andreas

> 
>>
>> Paolo
>>
>>>>>          goto out;
>>>>>      }
>>>>>  
>>>>> -    cpu_x86_parse_featurestr(cpu, features, &error);
>>>>> -    if (error) {
>>>>> +    cpu_x86_parse_featurestr(cpu, features, errp);
>>>>> +    if (error_is_set(errp)) {
>>>>>          goto out;
>>>>>      }
>>>>>  
>>>>> -    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
>>>>> +out:
>>>>> +    g_strfreev(model_pieces);
>>> Any specific reason you didn't choose to keep 'Error *error = NULL'
>>> inside cpu_x86_create() as well, and use error_propagate() here? I
>>> believe it would make the patch simpler and easier to review, and at the
>>> same time make cpu_x86_init() check for errors properly even if errp is
>>> NULL. This is the opposite of what you did on x86_cpu_realizefn() at
>>> patch 01/22.
>>
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg