Re: [Qemu-devel] [PATCH 4/8] target-i386: Keep track of apic-id setting

[Eduardo Habkost]

On Fri, Dec 19, 2014 at 12:23:07PM +0100, Paolo Bonzini wrote:
> On 19/12/2014 03:41, Eduardo Habkost wrote:
> > Set a flag indicating that the apic-id property was set, so we can later
> > ensure we won't try to realize a X86CPU object before apic-id was set.
> > 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > Cc: Gu Zheng <address@hidden>
> > ---
> >  target-i386/cpu-qom.h | 1 +
> >  target-i386/cpu.c     | 3 ++-
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> > index 4a6f48a..ba0ee15 100644
> > --- a/target-i386/cpu-qom.h
> > +++ b/target-i386/cpu-qom.h
> > @@ -94,6 +94,7 @@ typedef struct X86CPU {
> >      bool migratable;
> >      bool host_features;
> >      uint32_t apic_id;
> > +    bool apic_id_set;
> >  
> >      /* if true the CPUID code directly forward host cache leaves to the 
> > guest */
> >      bool cache_info_passthrough;
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index cbed717..bb9525d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1691,7 +1691,7 @@ static void x86_cpuid_get_apic_id(Object *obj, 
> > Visitor *v, void *opaque,
> >                                    const char *name, Error **errp)
> >  {
> >      X86CPU *cpu = X86_CPU(obj);
> > -    int64_t value = cpu->apic_id;
> > +    int64_t value = cpu->apic_id_set ? cpu->apic_id : -1;
> 
> Should this return an error if the apic_id was not set?

Both approaches look valid to me. But I wouldn't expect a property to
return errors by default when read, I expect it to simply have a default
value (in this case, -1).

If we return errors, a tool/command that reads all properties from
objects would need to treat it as a special case, instead of simply
printing "-1".

Do we have any other cases where we return errors from a property getter
(especially in the default case)? I didn't find any.

-- 
Eduardo