From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48835) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S1KB1-0004CE-IX for qemu-devel@nongnu.org; Sat, 25 Feb 2012 11:08:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S1KAq-0001LR-9K for qemu-devel@nongnu.org; Sat, 25 Feb 2012 11:08:51 -0500 Received: from cantor2.suse.de ([195.135.220.15]:37007 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S1KAq-0001L9-0B for qemu-devel@nongnu.org; Sat, 25 Feb 2012 11:08:40 -0500 Message-ID: <4F490785.1070603@suse.de> Date: Sat, 25 Feb 2012 17:08:37 +0100 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1330028546-13183-1-git-send-email-mdroth@linux.vnet.ibm.com> <1330028546-13183-7-git-send-email-mdroth@linux.vnet.ibm.com> <4F47C73E.6030105@codemonkey.ws> <20120225154107.GA2725@illuin> In-Reply-To: <20120225154107.GA2725@illuin> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 6/6] qdev: switch property accessors to fixed-width visitor interfaces List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, Anthony Liguori Am 25.02.2012 16:41, schrieb Michael Roth: > On Fri, Feb 24, 2012 at 11:22:06AM -0600, Anthony Liguori wrote: >> According to git bisect and qemu-test, this breaks: >> >> qemu-system-x86_64 -kernel bin/vmlinuz-3.0 -initrd >> .tmp-26227/initramfs-26227.img.gz -append console=3DttyS0 seed=3D1498 >> -nographic -enable-kvm -device virtio-balloon-pci,id=3Dballoon0 >> -pidfile .tmp-26227/pidfile-26227.pid -qmp >> unix:.tmp-26227/qmpsock-26227.sock,server,nowait >> qemu-system-x86_64: Parameter 'id' expects int8_t >> Aborted >=20 > Sorry, put way too much faith in the unit tests catching this. >=20 > The issue is we currently use set_int* for both uint* and int* > properties. In this case the default uint8_t property value was > (uint8_t)-1 =3D 255, which we'd stick in a qobject and feed to the > visitors. Before, we'd just read that back into an int64_t container an= d > let it be re-interpreted as -1 or 255 depending on the property type. >=20 > Now, we still fall back to visit_type_int() for QmpInputVisitor, but in > the case of visit_type_int8() we check that the value falls within the > signed range, which isn't the case for 255. >=20 > There's a few other places where we hit similar issues. The 2 possible > solutions are: >=20 > 1) Loosen the range checks in qapi-visit-core.c so that we ignore > signedness and only check that (uintX_t)value is small enough to fit > in X bytes, or >=20 > 2) Add set_uint*/get_uint* accessors for uint* qdev properties. >=20 > 1 is less code, and more forgiving of cases were we might use int*/uint= * > interchangeably, but 2 I think is more correct and tightens up the > bounds checking for qdev and whatever else we use QmpInputVisitor for. I'm not too deep into visitors yet but 2) sounds better to me. I've seen a couple of places where command line args are not properly checked before passing them on (will send patches for those I remember) so having the checks close to where the values came from sounds good. Paolo did provide separate object_property_set_[u]int* accessors so we should be good in QOM land when not fiddling with these things at such a "deep" level. Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg