On Tue, Apr 18, 2017 at 04:19:10PM -0500, Eric Blake wrote: > On 04/18/2017 04:01 PM, Michael Roth wrote: > >>> @@ -3706,8 +3707,17 @@ static void x86_cpu_get_bit_prop(Object *obj, > >>> Visitor *v, const char *name, > >>> X86CPU *cpu = X86_CPU(obj); > >>> BitProperty *fp = opaque; > >>> uint32_t f = cpu->env.features[fp->w]; > >>> + uint32_t ff = cpu->forced_features[fp->w]; > >>> bool value = (f & fp->mask) == fp->mask; > >>> - visit_type_bool(v, name, &value, errp); > >>> + bool forced = (ff & fp->mask) == fp->mask; > >>> + char str[] = "force"; > >>> + char *strval = str; > >>> + > >>> + if (forced) { > >>> + visit_type_str(v, name, &strval, errp); > >>> + } else { > >>> + visit_type_bool(v, name, &value, errp); > >> > >> Interesting approach. This means we keep returning a boolean > >> value on the property getter (which is important to not break > >> compatibility), but return a string in case the feature was set > >> to "force". > >> > >> We probably should update the 'type' field of the property, as it > >> won't be a real "bool" property anymore. > >> > >> I won't say I love that solution, but it seems to work. I'm CCing > >> the QAPI maintainers to see what they think about it. > > Use of a formal QAPI alternate type seems reasonable here. > > >> > >> (For reference: in the v1 thread I have suggested introducing a > >> new enum type in the QAPI schema, but this would break > >> compatibility with existing management code that expects the > >> property to return a boolean value [like the users of > >> query-cpu-model-expansion]). > > >>> @@ -3724,7 +3735,15 @@ static void x86_cpu_set_bit_prop(Object *obj, > >>> Visitor *v, const char *name, > >>> return; > >>> } > >>> > >>> - visit_type_bool(v, name, &value, &local_err); > >>> + visit_type_str(v, name, &strval, &local_err); > >>> + if (!local_err && !strcmp(strval, "force")) { > >>> + value = true; > >>> + cpu->forced_features[fp->w] |= fp->mask; > >>> + } else { > >>> + local_err = NULL; > >>> + visit_type_bool(v, name, &value, &local_err); > >>> + } > >> > >> I'm not sure this is valid usage of the visitor API. If the > >> visit_type_str() call succeeds, isn't the visitor allowed to > >> consume the original value, and return something completely > >> different when visit_type_bool() is called? > > > > Better would be defining the QAPI alternate type: > > { 'enum': 'Force', 'data': ['force']} > { 'alternate': 'BoolOrForce', > 'data': { 'b': 'bool', 's': 'Force' } } > > then given a BoolOrForce *state; declaration, you do either: > > state->type = QTYPE_QBOOL; > state->u.b = true; > > or > > state->type = QTYPE_QSTRING; > state->u.s = FORCE_FORCE; > > and then call visit_type_BoolOrForce(..., &state ...) > > Qcow2OverlapChecks is an example of an alternate in use, if that helps.
Wow, this is wonderful. I will take a look. Thanks! > [...] -- Eduardo