From: Anthony Liguori <anthony@codemonkey.ws>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v2 6/8] qom: introduce get/set methods for Property
Date: Fri, 16 Dec 2011 11:02:39 -0600 [thread overview]
Message-ID: <4EEB79AF.2020309@codemonkey.ws> (raw)
In-Reply-To: <1324054053-20484-7-git-send-email-pbonzini@redhat.com>
On 12/16/2011 10:47 AM, Paolo Bonzini wrote:
> This patch adds a visitor interface to Property. This way, QOM will be
> able to expose Properties that access a fixed field in a struct without
> exposing also the everything-is-a-string "feature" of qdev properties.
>
> Whenever the printed representation in both QOM and qdev (which is
> typically the case for device backends), parse/print code can be reused
> via get_generic/set_generic. Dually, whenever multiple PropertyInfos
> have the same representation in both the struct and the visitors the
> code can be reused (for example among all of int32/uint32/hex32).
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Regards,
Anthony Liguori
> ---
> hw/qdev-addr.c | 41 ++++++
> hw/qdev-properties.c | 355 ++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/qdev.h | 4 +
> 3 files changed, 400 insertions(+), 0 deletions(-)
>
> diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
> index 305c2d3..5ddda2d 100644
> --- a/hw/qdev-addr.c
> +++ b/hw/qdev-addr.c
> @@ -18,12 +18,53 @@ static int print_taddr(DeviceState *dev, Property *prop, char *dest, size_t len)
> return snprintf(dest, len, "0x" TARGET_FMT_plx, *ptr);
> }
>
> +static void get_taddr(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + target_phys_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
> + int64_t value;
> +
> + value = *ptr;
> + visit_type_int(v,&value, name, errp);
> +}
> +
> +static void set_taddr(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + target_phys_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
> + Error *local_err = NULL;
> + int64_t value;
> +
> + if (dev->state != DEV_STATE_CREATED) {
> + error_set(errp, QERR_PERMISSION_DENIED);
> + return;
> + }
> +
> + visit_type_int(v,&value, name,&local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + if ((uint64_t)value<= (uint64_t) ~(target_phys_addr_t)0) {
> + *ptr = value;
> + } else {
> + error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> + dev->id?:"", name, value, (uint64_t) 0,
> + (uint64_t) ~(target_phys_addr_t)0);
> + }
> +}
> +
> +
> PropertyInfo qdev_prop_taddr = {
> .name = "taddr",
> .type = PROP_TYPE_TADDR,
> .size = sizeof(target_phys_addr_t),
> .parse = parse_taddr,
> .print = print_taddr,
> + .get = get_taddr,
> + .set = set_taddr,
> };
>
> void qdev_prop_set_taddr(DeviceState *dev, const char *name, target_phys_addr_t value)
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 76ecb38..f7aa3cb 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -55,12 +55,44 @@ static int print_bit(DeviceState *dev, Property *prop, char *dest, size_t len)
> return snprintf(dest, len, (*p& qdev_get_prop_mask(prop)) ? "on" : "off");
> }
>
> +static void get_bit(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + uint32_t *p = qdev_get_prop_ptr(dev, prop);
> + bool value = (*p& qdev_get_prop_mask(prop)) != 0;
> +
> + visit_type_bool(v,&value, name, errp);
> +}
> +
> +static void set_bit(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + Error *local_err = NULL;
> + bool value;
> +
> + if (dev->state != DEV_STATE_CREATED) {
> + error_set(errp, QERR_PERMISSION_DENIED);
> + return;
> + }
> +
> + visit_type_bool(v,&value, name,&local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + bit_prop_set(dev, prop, value);
> +}
> +
> PropertyInfo qdev_prop_bit = {
> .name = "on/off",
> .type = PROP_TYPE_BIT,
> .size = sizeof(uint32_t),
> .parse = parse_bit,
> .print = print_bit,
> + .get = get_bit,
> + .set = set_bit,
> };
>
> /* --- 8bit integer --- */
> @@ -85,12 +117,54 @@ static int print_uint8(DeviceState *dev, Property *prop, char *dest, size_t len)
> return snprintf(dest, len, "%" PRIu8, *ptr);
> }
>
> +static void get_int8(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + int8_t *ptr = qdev_get_prop_ptr(dev, prop);
> + int64_t value;
> +
> + value = *ptr;
> + visit_type_int(v,&value, name, errp);
> +}
> +
> +static void set_int8(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + int8_t *ptr = qdev_get_prop_ptr(dev, prop);
> + Error *local_err = NULL;
> + int64_t value;
> +
> + if (dev->state != DEV_STATE_CREATED) {
> + error_set(errp, QERR_PERMISSION_DENIED);
> + return;
> + }
> +
> + visit_type_int(v,&value, name,&local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + if (value> prop->info->min&& value<= prop->info->max) {
> + *ptr = value;
> + } else {
> + error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> + dev->id?:"", name, value, prop->info->min,
> + prop->info->max);
> + }
> +}
> +
> PropertyInfo qdev_prop_uint8 = {
> .name = "uint8",
> .type = PROP_TYPE_UINT8,
> .size = sizeof(uint8_t),
> .parse = parse_uint8,
> .print = print_uint8,
> + .get = get_int8,
> + .set = set_int8,
> + .min = 0,
> + .max = 255,
> };
>
> /* --- 8bit hex value --- */
> @@ -120,6 +194,10 @@ PropertyInfo qdev_prop_hex8 = {
> .size = sizeof(uint8_t),
> .parse = parse_hex8,
> .print = print_hex8,
> + .get = get_int8,
> + .set = set_int8,
> + .min = 0,
> + .max = 255,
> };
>
> /* --- 16bit integer --- */
> @@ -144,12 +222,54 @@ static int print_uint16(DeviceState *dev, Property *prop, char *dest, size_t len
> return snprintf(dest, len, "%" PRIu16, *ptr);
> }
>
> +static void get_int16(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + int16_t *ptr = qdev_get_prop_ptr(dev, prop);
> + int64_t value;
> +
> + value = *ptr;
> + visit_type_int(v,&value, name, errp);
> +}
> +
> +static void set_int16(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + int16_t *ptr = qdev_get_prop_ptr(dev, prop);
> + Error *local_err = NULL;
> + int64_t value;
> +
> + if (dev->state != DEV_STATE_CREATED) {
> + error_set(errp, QERR_PERMISSION_DENIED);
> + return;
> + }
> +
> + visit_type_int(v,&value, name,&local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + if (value> prop->info->min&& value<= prop->info->max) {
> + *ptr = value;
> + } else {
> + error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> + dev->id?:"", name, value, prop->info->min,
> + prop->info->max);
> + }
> +}
> +
> PropertyInfo qdev_prop_uint16 = {
> .name = "uint16",
> .type = PROP_TYPE_UINT16,
> .size = sizeof(uint16_t),
> .parse = parse_uint16,
> .print = print_uint16,
> + .get = get_int16,
> + .set = set_int16,
> + .min = 0,
> + .max = 65535,
> };
>
> /* --- 32bit integer --- */
> @@ -174,12 +294,54 @@ static int print_uint32(DeviceState *dev, Property *prop, char *dest, size_t len
> return snprintf(dest, len, "%" PRIu32, *ptr);
> }
>
> +static void get_int32(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + int32_t *ptr = qdev_get_prop_ptr(dev, prop);
> + int64_t value;
> +
> + value = *ptr;
> + visit_type_int(v,&value, name, errp);
> +}
> +
> +static void set_int32(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + int32_t *ptr = qdev_get_prop_ptr(dev, prop);
> + Error *local_err = NULL;
> + int64_t value;
> +
> + if (dev->state != DEV_STATE_CREATED) {
> + error_set(errp, QERR_PERMISSION_DENIED);
> + return;
> + }
> +
> + visit_type_int(v,&value, name,&local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + if (value> prop->info->min&& value<= prop->info->max) {
> + *ptr = value;
> + } else {
> + error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> + dev->id?:"", name, value, prop->info->min,
> + prop->info->max);
> + }
> +}
> +
> PropertyInfo qdev_prop_uint32 = {
> .name = "uint32",
> .type = PROP_TYPE_UINT32,
> .size = sizeof(uint32_t),
> .parse = parse_uint32,
> .print = print_uint32,
> + .get = get_int32,
> + .set = set_int32,
> + .min = 0,
> + .max = 0xFFFFFFFFULL,
> };
>
> static int parse_int32(DeviceState *dev, Property *prop, const char *str)
> @@ -207,6 +369,10 @@ PropertyInfo qdev_prop_int32 = {
> .size = sizeof(int32_t),
> .parse = parse_int32,
> .print = print_int32,
> + .get = get_int32,
> + .set = set_int32,
> + .min = -0x80000000LL,
> + .max = 0x7FFFFFFFLL,
> };
>
> /* --- 32bit hex value --- */
> @@ -236,6 +402,10 @@ PropertyInfo qdev_prop_hex32 = {
> .size = sizeof(uint32_t),
> .parse = parse_hex32,
> .print = print_hex32,
> + .get = get_int32,
> + .set = set_int32,
> + .min = 0,
> + .max = 0xFFFFFFFFULL,
> };
>
> /* --- 64bit integer --- */
> @@ -260,12 +430,37 @@ static int print_uint64(DeviceState *dev, Property *prop, char *dest, size_t len
> return snprintf(dest, len, "%" PRIu64, *ptr);
> }
>
> +static void get_int64(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + int64_t *ptr = qdev_get_prop_ptr(dev, prop);
> +
> + visit_type_int(v, ptr, name, errp);
> +}
> +
> +static void set_int64(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + int64_t *ptr = qdev_get_prop_ptr(dev, prop);
> +
> + if (dev->state != DEV_STATE_CREATED) {
> + error_set(errp, QERR_PERMISSION_DENIED);
> + return;
> + }
> +
> + visit_type_int(v, ptr, name, errp);
> +}
> +
> PropertyInfo qdev_prop_uint64 = {
> .name = "uint64",
> .type = PROP_TYPE_UINT64,
> .size = sizeof(uint64_t),
> .parse = parse_uint64,
> .print = print_uint64,
> + .get = get_int64,
> + .set = set_int64,
> };
>
> /* --- 64bit hex value --- */
> @@ -295,6 +490,8 @@ PropertyInfo qdev_prop_hex64 = {
> .size = sizeof(uint64_t),
> .parse = parse_hex64,
> .print = print_hex64,
> + .get = get_int64,
> + .set = set_int64,
> };
>
> /* --- string --- */
> @@ -322,6 +519,48 @@ static int print_string(DeviceState *dev, Property *prop, char *dest, size_t len
> return snprintf(dest, len, "\"%s\"", *ptr);
> }
>
> +static void get_string(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + char **ptr = qdev_get_prop_ptr(dev, prop);
> +
> + if (!*ptr) {
> + char *str = (char *)"";
> + visit_type_str(v,&str, name, errp);
> + } else {
> + visit_type_str(v, ptr, name, errp);
> + }
> +}
> +
> +static void set_string(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + char **ptr = qdev_get_prop_ptr(dev, prop);
> + Error *local_err = NULL;
> + char *str;
> +
> + if (dev->state != DEV_STATE_CREATED) {
> + error_set(errp, QERR_PERMISSION_DENIED);
> + return;
> + }
> +
> + visit_type_str(v,&str, name,&local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + if (!*str) {
> + g_free(str);
> + str = NULL;
> + }
> + if (*ptr) {
> + g_free(*ptr);
> + }
> + *ptr = str;
> +}
> +
> PropertyInfo qdev_prop_string = {
> .name = "string",
> .type = PROP_TYPE_STRING,
> @@ -329,6 +568,8 @@ PropertyInfo qdev_prop_string = {
> .parse = parse_string,
> .print = print_string,
> .free = free_string,
> + .get = get_string,
> + .set = set_string,
> };
>
> /* --- drive --- */
> @@ -364,12 +605,59 @@ static int print_drive(DeviceState *dev, Property *prop, char *dest, size_t len)
> *ptr ? bdrv_get_device_name(*ptr) : "<null>");
> }
>
> +static void get_generic(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + void **ptr = qdev_get_prop_ptr(dev, prop);
> + char buffer[1024];
> + char *p = buffer;
> +
> + buffer[0] = 0;
> + if (*ptr) {
> + prop->info->print(dev, prop, buffer, sizeof(buffer));
> + }
> + visit_type_str(v,&p, name, errp);
> +}
> +
> +static void set_generic(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + Error *local_err = NULL;
> + char *str;
> + int ret;
> +
> + if (dev->state != DEV_STATE_CREATED) {
> + error_set(errp, QERR_PERMISSION_DENIED);
> + return;
> + }
> +
> + visit_type_str(v,&str, name,&local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + if (!*str) {
> + g_free(str);
> + qdev_prop_error(errp, EINVAL, dev, prop, str);
> + return;
> + }
> + ret = prop->info->parse(dev, prop, str);
> + if (ret != 0) {
> + qdev_prop_error(errp, ret, dev, prop, str);
> + }
> + g_free(str);
> +}
> +
> PropertyInfo qdev_prop_drive = {
> .name = "drive",
> .type = PROP_TYPE_DRIVE,
> .size = sizeof(BlockDriverState *),
> .parse = parse_drive,
> .print = print_drive,
> + .get = get_generic,
> + .set = set_generic,
> .free = free_drive,
> };
>
> @@ -407,6 +695,8 @@ PropertyInfo qdev_prop_chr = {
> .size = sizeof(CharDriverState*),
> .parse = parse_chr,
> .print = print_chr,
> + .get = get_generic,
> + .set = set_generic,
> };
>
> /* --- netdev device --- */
> @@ -441,6 +731,8 @@ PropertyInfo qdev_prop_netdev = {
> .size = sizeof(VLANClientState*),
> .parse = parse_netdev,
> .print = print_netdev,
> + .get = get_generic,
> + .set = set_generic,
> };
>
> /* --- vlan --- */
> @@ -469,12 +761,57 @@ static int print_vlan(DeviceState *dev, Property *prop, char *dest, size_t len)
> }
> }
>
> +static void get_vlan(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + VLANState **ptr = qdev_get_prop_ptr(dev, prop);
> + int64_t id;
> +
> + id = *ptr ? (*ptr)->id : -1;
> + visit_type_int(v,&id, name, errp);
> +}
> +
> +static void set_vlan(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + VLANState **ptr = qdev_get_prop_ptr(dev, prop);
> + Error *local_err = NULL;
> + int64_t id;
> + VLANState *vlan;
> +
> + if (dev->state != DEV_STATE_CREATED) {
> + error_set(errp, QERR_PERMISSION_DENIED);
> + return;
> + }
> +
> + visit_type_int(v,&id, name,&local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + if (id == -1) {
> + *ptr = NULL;
> + return;
> + }
> + vlan = qemu_find_vlan(id, 1);
> + if (!vlan) {
> + error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> + name, prop->info->name);
> + return;
> + }
> + *ptr = vlan;
> +}
> +
> PropertyInfo qdev_prop_vlan = {
> .name = "vlan",
> .type = PROP_TYPE_VLAN,
> .size = sizeof(VLANClientState*),
> .parse = parse_vlan,
> .print = print_vlan,
> + .get = get_vlan,
> + .set = set_vlan,
> };
>
> /* --- pointer --- */
> @@ -531,6 +867,8 @@ PropertyInfo qdev_prop_macaddr = {
> .size = sizeof(MACAddr),
> .parse = parse_mac,
> .print = print_mac,
> + .get = get_generic,
> + .set = set_generic,
> };
>
> /* --- pci address --- */
> @@ -570,12 +908,29 @@ static int print_pci_devfn(DeviceState *dev, Property *prop, char *dest, size_t
> }
> }
>
> +static void get_pci_devfn(DeviceState *dev, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + Property *prop = opaque;
> + uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
> + char buffer[32];
> + char *p = buffer;
> +
> + buffer[0] = 0;
> + if (*ptr != -1) {
> + snprintf(buffer, sizeof(buffer), "%02x.%x", *ptr>> 3, *ptr& 7);
> + }
> + visit_type_str(v,&p, name, errp);
> +}
> +
> PropertyInfo qdev_prop_pci_devfn = {
> .name = "pci-devfn",
> .type = PROP_TYPE_UINT32,
> .size = sizeof(uint32_t),
> .parse = parse_pci_devfn,
> .print = print_pci_devfn,
> + .get = get_pci_devfn,
> + .set = set_generic,
> };
>
> /* --- public helpers --- */
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 828d811..a1cce37 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -158,9 +158,13 @@ struct PropertyInfo {
> const char *name;
> size_t size;
> enum PropertyType type;
> + int64_t min;
> + int64_t max;
> int (*parse)(DeviceState *dev, Property *prop, const char *str);
> int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len);
> void (*free)(DeviceState *dev, Property *prop);
> + DevicePropertyAccessor *get;
> + DevicePropertyAccessor *set;
> };
>
> typedef struct GlobalProperty {
next prev parent reply other threads:[~2011-12-16 17:02 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-12-16 16:47 [Qemu-devel] [PATCH v2 0/8] qom: introduce non-legacy static properties Paolo Bonzini
2011-12-16 16:47 ` [Qemu-devel] [PATCH v2 1/8] qapi: protect against NULL QObject in qmp_input_get_object Paolo Bonzini
2011-12-16 16:47 ` [Qemu-devel] [PATCH v2 2/8] qom: fix swapped parameters Paolo Bonzini
2011-12-16 16:47 ` [Qemu-devel] [PATCH v2 3/8] qom: push permission checks up into qdev_property_add_legacy Paolo Bonzini
2011-12-16 16:47 ` [Qemu-devel] [PATCH v2 4/8] qom: interpret the return value when setting legacy properties Paolo Bonzini
2011-12-16 17:00 ` Anthony Liguori
2011-12-16 17:19 ` Paolo Bonzini
2011-12-16 16:47 ` [Qemu-devel] [PATCH v2 5/8] qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE Paolo Bonzini
2011-12-16 17:00 ` Anthony Liguori
2011-12-16 16:47 ` [Qemu-devel] [PATCH v2 6/8] qom: introduce get/set methods for Property Paolo Bonzini
2011-12-16 17:02 ` Anthony Liguori [this message]
2011-12-16 16:47 ` [Qemu-devel] [PATCH v2 7/8] qom: distinguish "legacy" property type name from QOM type name Paolo Bonzini
2011-12-16 17:03 ` Anthony Liguori
2011-12-16 16:47 ` [Qemu-devel] [PATCH v2 8/8] qom: register qdev properties also as non-legacy properties Paolo Bonzini
2011-12-16 17:05 ` Anthony Liguori
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4EEB79AF.2020309@codemonkey.ws \
--to=anthony@codemonkey.ws \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.