From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:60298) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RbbBN-0002B1-2g for qemu-devel@nongnu.org; Fri, 16 Dec 2011 12:02:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RbbBC-0007DA-Jh for qemu-devel@nongnu.org; Fri, 16 Dec 2011 12:02:53 -0500 Received: from mail-yx0-f173.google.com ([209.85.213.173]:61446) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RbbBC-0007D5-Bf for qemu-devel@nongnu.org; Fri, 16 Dec 2011 12:02:42 -0500 Received: by yenm6 with SMTP id m6so2752525yen.4 for ; Fri, 16 Dec 2011 09:02:42 -0800 (PST) Message-ID: <4EEB79AF.2020309@codemonkey.ws> Date: Fri, 16 Dec 2011 11:02:39 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <1324054053-20484-1-git-send-email-pbonzini@redhat.com> <1324054053-20484-7-git-send-email-pbonzini@redhat.com> In-Reply-To: <1324054053-20484-7-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 6/8] qom: introduce get/set methods for Property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org 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 Reviewed-by: Anthony Liguori 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) : ""); > } > > +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 {