Re: [Qemu-devel] [PATCH 1/2] add support for KVM_CAP_SPLIT_IRQCHIP

[Eric Blake]

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 <address@hidden>
> ---

> -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