From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39787) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZxOQy-0003QT-Oi for qemu-devel@nongnu.org; Fri, 13 Nov 2015 19:11:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZxOQt-000848-On for qemu-devel@nongnu.org; Fri, 13 Nov 2015 19:11:12 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45894) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZxOQt-00083y-Hw for qemu-devel@nongnu.org; Fri, 13 Nov 2015 19:11:07 -0500 References: <0172715B-B1BD-40E8-96A2-3C320928447D@google.com> From: Eric Blake Message-ID: <56467C15.9010800@redhat.com> Date: Fri, 13 Nov 2015 17:11:01 -0700 MIME-Version: 1.0 In-Reply-To: <0172715B-B1BD-40E8-96A2-3C320928447D@google.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="hnObbPWRnI6gIRTMEBXNmqfHGR5eaJ4fH" Subject: Re: [Qemu-devel] [PATCH 1/2] add support for KVM_CAP_SPLIT_IRQCHIP List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Matt Gingell , qemu-devel@nongnu.org, Markus Armbruster This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --hnObbPWRnI6gIRTMEBXNmqfHGR5eaJ4fH Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > Signed-off-by: Matt Gingell > --- > -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 =3D MACHINE(obj); > - > - ms->kernel_irqchip_allowed =3D value; > - ms->kernel_irqchip_required =3D value; > + OnOffSplit mode; > + MachineState *ms =3D 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=3D=3DNULL). Or in code, either: OnOffSplit mode =3D 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 =3D 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 =3D true; > + ms->kernel_irqchip_required =3D true; > + ms->kernel_irqchip_split =3D false; > + break; > + case ON_OFF_SPLIT_OFF: > + ms->kernel_irqchip_allowed =3D false; > + ms->kernel_irqchip_required =3D false; > + ms->kernel_irqchip_split =3D false; > + break; > + case ON_OFF_SPLIT_SPLIT: > + ms->kernel_irqchip_allowed =3D true; > + ms->kernel_irqchip_required =3D true; > + ms->kernel_irqchip_split =3D true; > + break; > + default: > + error_report("Option 'kernel-irqchip' must be one of on|off|sp= lit"); > + 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --hnObbPWRnI6gIRTMEBXNmqfHGR5eaJ4fH Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWRnwWAAoJEKeha0olJ0NqVdgIAJjqzYAOOmna83FC+9YqOz2B 5LwHFOvr7evfF5W1hhQVRo9/QycrSFgCU7f4YfJSgmaGpeeZLPL1w134ns/PzlJl LS5HIf1HSZDSmplv4Eq2tskLHklIKv2CjeFcIahV+xCmuSn78SiaA1P91vYh+Ui/ JtgqTiRZqRG4ReFq4VEiuPbY2tUtuYapzFDScFx3VlX60FYv/uMx5vpEdHbnZ04q 8O+foOTZkV9R/BMJ1HnTrEnz/YyGP+wFS8RHk5j/1SJkvmfHcWu0V3a38Cz/mxA6 Pe0h9gD5dq0CXB+ZMRUdazuzFfhzl5n48QL8cgv8rVzQ08aUPs3+f17lbvCKw9Q= =NQQ+ -----END PGP SIGNATURE----- --hnObbPWRnI6gIRTMEBXNmqfHGR5eaJ4fH--