On Thu, May 04, 2017 at 07:09:11PM +0200, Andrea Bolognani wrote:
> On Thu, 2017-04-27 at 17:28 +1000, David Gibson wrote:
> > @@ -2480,6 +2480,10 @@ static void spapr_machine_initfn(Object *obj)
> >                                      " place of standard EPOW events when possible"
> >                                      " (required for memory hot-unplug support)",
> >                                      NULL);
> > +
> > +    object_property_add(obj, "max-cpu-compat", "str",
> > +                        ppc_compat_prop_get, ppc_compat_prop_set,
> > +                        NULL, &spapr->max_compat_pvr, &error_fatal);
> 
> I'm not familiar with QEMU's object system, but shouldn't
> you be using object_property_add_str() instead? It looks
> like you're doing more than the straightforward wrapper
> would do, so maybe that's just not possible.

Right.  I can't use object_property_add_str() for two reasons.  First,
I need the opaque parameter which it lacks, second it assumes that
you're storing the property 
opaque parameter (which the _str() variant lacks) to pass in the
destination variable for the compat pvr.

> In any case, all other string properties look like
> 
>   pseries-2.10.kvm-type=string
> 
> whereas this one ends up looking like
> 
>   pseries-2.10.max-cpu-compat=str
> 
> which I think should be fixed - object_property_add_str()
> passes "string" instead of "str" to object_property_add().

Ah, yes, that's a bug, I'll fix.

> You should also add a sensible description for the property,
> preferably spelling out all the accepted values.

I'll add that.

> Speaking of properties...
> 
>   $ qemu-system-ppc64 -cpu host,compat=whatever
>   Segmentation fault
> 
> You might want to look into that ;)

Uh.. yeah.  I think I've fixed that.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson