From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1MPLoJ-0002Hx-AG
	for qemu-devel@nongnu.org; Fri, 10 Jul 2009 15:31:07 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1MPLoE-0002GS-Ng
	for qemu-devel@nongnu.org; Fri, 10 Jul 2009 15:31:06 -0400
Received: from [199.232.76.173] (port=51725 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1MPLoE-0002GP-Ia
	for qemu-devel@nongnu.org; Fri, 10 Jul 2009 15:31:02 -0400
Received: from mx2.redhat.com ([66.187.237.31]:44464)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <kraxel@redhat.com>) id 1MPLoE-0002Xa-5W
	for qemu-devel@nongnu.org; Fri, 10 Jul 2009 15:31:02 -0400
Message-ID: <4A57967A.50008@redhat.com>
Date: Fri, 10 Jul 2009 21:28:58 +0200
From: Gerd Hoffmann <kraxel@redhat.com>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH 01/13] qdev: rework device properties.
References: <1247225179-5495-1-git-send-email-kraxel@redhat.com>
	<1247225179-5495-2-git-send-email-kraxel@redhat.com>
	<200907101813.37430.paul@codesourcery.com>
In-Reply-To: <200907101813.37430.paul@codesourcery.com>
Content-Type: text/plain; charset=ISO-8859-15; format=flowed
Content-Transfer-Encoding: 7bit
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paul Brook <paul@codesourcery.com>
Cc: qemu-devel@nongnu.org

On 07/10/09 19:13, Paul Brook wrote:
>> +extern PropertyInfo qdev_prop_uint32;
>> +extern PropertyInfo qdev_prop_hex32;
>
> Having both of these seems wrong.

Why?

There are properties which tend to be specified as decimal numbers 
(counts, sizes, ...) and some which tend to be specified in hex 
(adresses, ioports, ...).  I think it is useful to have separate 
parse/print functions for them, although they both end up being an uint32_t.

>> +            .name   = "fifo-size",
>> +            .info   =&qdev_prop_uint32,
>> +            .offset = offsetof(SyborgPointerState, fifo_size),
>> +            .defval = (uint32_t[]) { 16 },
>
> This feels kinda crufty. Very easy to get the wrong types.

I think I can make defval a string instead, then go through 
PropertyInfo->parse().  OK?

> There's also no
> typechecking in qdev_set_prop*.

It checks the size.  Which will miss some type mismatches.  I'll fix it.

>> +int qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t
>> value);
>
> Why does this return a value?

It can fail if the size check (soon to be type check) mentioned above 
failed.  Such a failure would be a clear qemu bug though, so maybe 
abort() instead?

cheers,
   Gerd