On 11/13/2015 04:25 PM, Matt Gingell wrote: [meta-comment:] This patch was sent without an In-Reply-To header tying it back to the 0/2 cover letter, which means mailers render it as its own thread. Git 'send-email' can do proper threading with the right settings (although I'm not sure exactly what to suggest tweaking, if the defaults aren't already right).
> This patch adds the initial plumbing for split IRQ chip mode via > KVM_CAP_SPLIT_IRQCHIP. In addition to option processing, a number of > kvm_*_in_kernel macros are defined to help clarify which component is > where. > > Signed-off-by: Matt Gingell <ging...@google.com> > --- > -static void machine_set_kernel_irqchip(Object *obj, bool value, Error **errp) > +static void machine_set_kernel_irqchip(Object *obj, Visitor *v, > + void *opaque, const char *name, > + Error **errp) > { > - MachineState *ms = MACHINE(obj); > - > - ms->kernel_irqchip_allowed = value; > - ms->kernel_irqchip_required = value; > + OnOffSplit mode; > + MachineState *ms = MACHINE(obj); > + > + visit_type_OnOffSplit(v, &mode, name, errp); > + switch (mode) { 'mode' starts life uninitialized, and the current contract of visit_type_OnOffSplit (or any visit_type_Enum, for that matter) is that it is left unchanged except on success (see qapi/qapi-visit-core.c:input_type_enum() for the implementation). However, you are blindly calling switch without checking whether an error was reported, which could lead to spurious code behavior depending on what was previously on the stack. Arguably, we could fix the qapi generator for visit_type_Enum() to set the parameter to the sentinel _MAX value on error, to make buggy code like this less likely to do the wrong things on uninitialized input. But I don't know if that is a bug worth fixing during 2.5 hardfreeze (it's more likely to expose other bugs, but why not let sleeping dogs lie rather than risk regressions where things happened to work by chance). So, your choices are to pre-initialize mode to a sane sentinel value, or else check for error before switching on mode (and remember that you can't check errp directly, but would need a local err, since the caller may have passed in errp==NULL). Or in code, either: OnOffSplit mode = ON_OFF_SPLIT_MAX; visit_type_OnOffSplit(v, &mode, name, errp); switch (mode) { case ON_OFF_SPLIT_MAX: // flag invalid input; errp already contains potentially nice message or: OnOffSplit mode; Error *err = NULL; visit_type_OnOffSplit(v, &mode, name, &err); if (err) { // handle err, perhaps with error_propagate() } else { switch (mode) { case ON_OFF_SPLIT_ON: case ON_OFF_SPLIT_OFF: case ON_OFF_SPLIT_SPLIT: // handle break; default: abort(); // unreachable } > + case ON_OFF_SPLIT_ON: > + ms->kernel_irqchip_allowed = true; > + ms->kernel_irqchip_required = true; > + ms->kernel_irqchip_split = false; > + break; > + case ON_OFF_SPLIT_OFF: > + ms->kernel_irqchip_allowed = false; > + ms->kernel_irqchip_required = false; > + ms->kernel_irqchip_split = false; > + break; > + case ON_OFF_SPLIT_SPLIT: > + ms->kernel_irqchip_allowed = true; > + ms->kernel_irqchip_required = true; > + ms->kernel_irqchip_split = true; > + break; > + default: > + error_report("Option 'kernel-irqchip' must be one of on|off|split"); > + exit(1); Eww. Why are you calling exit() in a function that already returns errp? Shouldn't you instead just bubble the error up the stack? > +++ b/qapi/common.json > @@ -114,3 +114,19 @@ > ## > { 'enum': 'OnOffAuto', > 'data': [ 'auto', 'on', 'off' ] } > + > +## > +# @OnOffSplit > +# > +# An enumeration of three values: on, off, and split > +# > +# @on: Enabled > +# > +# @off: Disabled > +# > +# @split: Mixed > +# > +# Since: 2.6 > +## > +{ 'enum': 'OnOffSplit', > + 'data': [ 'on', 'off', 'split' ] } Use of qapi for the enum mapping looks sane. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature