Re: [Qemu-devel] [PATCH v2 6/6] qdev: switch property accessors to fixed-width visitor interfaces

[Andreas Färber]

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=ttyS0 seed=1498
>> -nographic -enable-kvm -device virtio-balloon-pci,id=balloon0
>> -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
> 
> Sorry, put way too much faith in the unit tests catching this.
> 
> 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 = 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 and
> let it be re-interpreted as -1 or 255 depending on the property type.
> 
> 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.
> 
> There's a few other places where we hit similar issues. The 2 possible
> solutions are:
> 
> 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
> 
> 2) Add set_uint*/get_uint* accessors for uint* qdev properties.
> 
> 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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg