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

[Paolo Bonzini]

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.

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.