diff mbox

[6/8] qom: introduce get/set methods for Property

Message ID 1324036918-2405-7-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Dec. 16, 2011, 12:01 p.m. UTC 
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>
---
 hw/qdev-addr.c       |   41 ++++++
 hw/qdev-properties.c |  348 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h            |    4 +
 3 files changed, 393 insertions(+), 0 deletions(-)

Comments

Gerd Hoffmann Dec. 16, 2011, 1:11 p.m. UTC | #1 
Hi,

>  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 +687,8 @@ PropertyInfo qdev_prop_chr = {
>      .size  = sizeof(CharDriverState*),
>      .parse = parse_chr,
>      .print = print_chr,
> +    .get   = get_generic,
> +    .set   = set_generic,
>  };
>  
>  /* --- netdev device --- */
> @@ -441,6 +723,8 @@ PropertyInfo qdev_prop_netdev = {
>      .size  = sizeof(VLANClientState*),
>      .parse = parse_netdev,
>      .print = print_netdev,
> +    .get   = get_generic,
> +    .set   = set_generic,
>  };

>  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 +860,8 @@ PropertyInfo qdev_prop_macaddr = {
>      .size  = sizeof(MACAddr),
>      .parse = parse_mac,
>      .print = print_mac,
> +    .get   = get_generic,
> +    .set   = set_generic,
>  };
>  
>  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,
>  };

I think we should not touch these.  First it doesn't buy us much as we
are using the old parse/print functions for the visitor-based access,
which doesn't look like a good idea to me.  Also they will not stay but
will be converted to child<> and link<>, so I think it is better to keep
the old stuff in the legacy<> namespace.

Agree on the bit/bool/int types.  Although we maybe should apply some
care to integer bus properties, some of them are used for addressing and
will most likely replaced by child<> and link<> too.

cheers,
  Gerd
Paolo Bonzini Dec. 16, 2011, 1:51 p.m. UTC | #2 
On 12/16/2011 02:11 PM, Gerd Hoffmann wrote:
> I think we should not touch these.  First it doesn't buy us much as we
> are using the old parse/print functions for the visitor-based access,
> which doesn't look like a good idea to me.  Also they will not stay but
> will be converted to child<>  and link<>, so I think it is better to keep
> the old stuff in the legacy<>  namespace.

I thought the same initially.  However, I noticed that the visitor 
interfaces for links is also a string.  So, even if a block/char/netdev 
property later becomes a link<>, the interface would not change.

Using the old parse/print functions and get_set/generic is only to avoid 
code duplication, I can surely inline everything but it would be uglier. 
  And again, I found an example in the code of using a similar adapter 
pattern (the string properties).

There is one case where I had doubts, namely the PCI address properties. 
  They will be replaced by links that you set in the parent.  However, 
in the end I decided to start this way because:

1) QOM properties can still come and go at this stage;

2) The PCI address property can still stay forever as a synthetic 
property declared by PCIDevice, so the "qom-get" ABI won't change.  The 
"qom-set" ABI will, so it might be better to do:

  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   = NULL,
  };

Advantages: it shows that setting the PCI address is (going to be) a 
legacy feature;

Disadvantages: looks a little ad-hoc.  See below for an alternative.

> Agree on the bit/bool/int types.  Although we maybe should apply some
> care to integer bus properties, some of them are used for addressing and
> will most likely replaced by child<>  and link<>  too.

Yes, these will also become synthetic and read-only.  So an alternative 
could be:

    for (prop = dev->info->props; prop && prop->name; prop++) {
        qdev_property_add_legacy(dev, prop, NULL);

        /* Let the generic initializer register alternative definitions
         * for qdev properties.
         */
        if (!qdev_property_find(dev, prop->name) {
            qdev_property_add_static(dev, prop, NULL);
        }
    }

    for (prop = dev->info->bus_info->props; prop && prop->name; prop++) {
        qdev_property_add_legacy(dev, prop, NULL);
        if (!qdev_property_find(dev, prop->name) {
            qdev_property_add_static(dev, prop, NULL);
        }
    }

For now the pci_devfn property remains read-write, but as soon as the 
PCIDevice will be able to define it as synthetic, it will become read-only.

Paolo
Anthony Liguori Dec. 16, 2011, 2:05 p.m. UTC | #3 
On 12/16/2011 07:51 AM, Paolo Bonzini wrote:
> On 12/16/2011 02:11 PM, Gerd Hoffmann wrote:
>> I think we should not touch these. First it doesn't buy us much as we
>> are using the old parse/print functions for the visitor-based access,
>> which doesn't look like a good idea to me. Also they will not stay but
>> will be converted to child<> and link<>, so I think it is better to keep
>> the old stuff in the legacy<> namespace.
>
> I thought the same initially. However, I noticed that the visitor interfaces for
> links is also a string. So, even if a block/char/netdev property later becomes a
> link<>, the interface would not change.

The semantics change though.  A "drive" link takes a flat block device name. 
When it's converted to a link, it will take a QOM path.  Since block devices 
will exist in their own directory, it will certainly still be possible to use 
the flat block device name but since a paths will also be supported, I think 
it's best to clearly distinguish the link based property from the flat block 
device name property.

Regards,

Anthony Liguori

>
> Paolo
>
Paolo Bonzini Dec. 16, 2011, 2:18 p.m. UTC | #4 
On 12/16/2011 03:05 PM, Anthony Liguori wrote:
>>
>> I thought the same initially. However, I noticed that the visitor
>> interfaces for
>> links is also a string. So, even if a block/char/netdev property later
>> becomes a
>> link<>, the interface would not change.
>
> The semantics change though.  A "drive" link takes a flat block device
> name. When it's converted to a link, it will take a QOM path.  Since
> block devices will exist in their own directory, it will certainly still
> be possible to use the flat block device name but since a paths will
> also be supported, I think it's best to clearly distinguish the link
> based property from the flat block device name property.

But it's a superset, no?

Paolo
Anthony Liguori Dec. 16, 2011, 2:44 p.m. UTC | #5 
On 12/16/2011 08:18 AM, Paolo Bonzini wrote:
> On 12/16/2011 03:05 PM, Anthony Liguori wrote:
>>>
>>> I thought the same initially. However, I noticed that the visitor
>>> interfaces for
>>> links is also a string. So, even if a block/char/netdev property later
>>> becomes a
>>> link<>, the interface would not change.
>>
>> The semantics change though. A "drive" link takes a flat block device
>> name. When it's converted to a link, it will take a QOM path. Since
>> block devices will exist in their own directory, it will certainly still
>> be possible to use the flat block device name but since a paths will
>> also be supported, I think it's best to clearly distinguish the link
>> based property from the flat block device name property.
>
> But it's a superset, no?

My concern is whether you'll get a graceful failure going new->old if you start 
making use of absolute paths.

The type name would change, so I guess that's good enough.

Regards,

Anthony Liguori

> Paolo
>
diff mbox

Patch

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 f0b811c..5e8dd9a 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,41 @@  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) {
+        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 (*ptr) {
+        g_free(*ptr);
+    }
+    *ptr = str;
+}
+
 PropertyInfo qdev_prop_string = {
     .name  = "string",
     .type  = PROP_TYPE_STRING,
@@ -329,6 +561,8 @@  PropertyInfo qdev_prop_string = {
     .parse = parse_string,
     .print = print_string,
     .free  = free_string,
+    .get   = get_string,
+    .set   = set_string,
 };
 
 /* --- drive --- */
@@ -364,12 +598,58 @@  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);
+
+    /* Same as qdev_get_legacy_property, but do not pass anything if
+     * the property is empty (NULL).
+     */
+    if (*ptr) {
+        char buffer[1024];
+        char *p = buffer;
+
+        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;
+    }
+    ret = prop->info->parse(dev, prop, str);
+    if (ret != 0) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  name, prop->info->name);
+    }
+    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 +687,8 @@  PropertyInfo qdev_prop_chr = {
     .size  = sizeof(CharDriverState*),
     .parse = parse_chr,
     .print = print_chr,
+    .get   = get_generic,
+    .set   = set_generic,
 };
 
 /* --- netdev device --- */
@@ -441,6 +723,8 @@  PropertyInfo qdev_prop_netdev = {
     .size  = sizeof(VLANClientState*),
     .parse = parse_netdev,
     .print = print_netdev,
+    .get   = get_generic,
+    .set   = set_generic,
 };
 
 /* --- vlan --- */
@@ -469,12 +753,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);
+
+    if (*ptr) {
+        int64_t id = (*ptr)->id;
+        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;
+    }
+}
+
 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 +860,8 @@  PropertyInfo qdev_prop_macaddr = {
     .size  = sizeof(MACAddr),
     .parse = parse_mac,
     .print = print_mac,
+    .get   = get_generic,
+    .set   = set_generic,
 };
 
 /* --- pci address --- */
@@ -570,12 +901,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);
+
+    if (*ptr != -1) {
+        char buffer[32];
+        char *p = buffer;
+
+        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 6e18427..9778123 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 {