Hi Eric,
Thanks for your comments. I’m submitting a v2 based on your feedback.
Matt
> On Nov 13, 2015, at 4:11 PM, Eric Blake <ebl...@redhat.com> wrote:
>
> 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
>
