Re: [Qemu-devel] [PATCH 11/17] object: use more specific property type names

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Use the actual unsigned integer type name (this should't break since
> property type aren't directly accessed from outside it seems).

I'm not sure I understand the parenthesis.  Do you mean changing the
type names is safe because they're used only in a certain way?  Which
way?

How did you find the names that need fixing?

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  backends/cryptodev.c |  2 +-
>  hw/pci-host/piix.c   |  8 ++++----
>  hw/pci-host/q35.c    | 10 +++++-----
>  hw/ppc/pnv.c         |  2 +-
>  net/dump.c           |  2 +-
>  net/filter-buffer.c  |  2 +-
>  6 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/backends/cryptodev.c b/backends/cryptodev.c
> index 832f056266..1764c179fe 100644
> --- a/backends/cryptodev.c
> +++ b/backends/cryptodev.c
> @@ -222,7 +222,7 @@ cryptodev_backend_can_be_deleted(UserCreatable *uc, Error **errp)
>  
>  static void cryptodev_backend_instance_init(Object *obj)
>  {
> -    object_property_add(obj, "queues", "int",
> +    object_property_add(obj, "queues", "uint32",
>                            cryptodev_backend_get_queues,
>                            cryptodev_backend_set_queues,
>                            NULL, NULL, NULL);

Correct, because the callbacks access struct CryptoDevBackendPeers
member queues, which is uint32_t.

The callbacks pass a local variable instead of the struct member to the
visitor.  Problematic, because it weakens the type checking we get from
the compiler.  Let's tighten it up:

   static void
   cryptodev_backend_get_queues(Object *obj, Visitor *v, const char *name,
                                void *opaque, Error **errp)
   {
       CryptoDevBackend *backend = CRYPTODEV_BACKEND(obj);
  -    uint32_t value = backend->conf.peers.queues;

  -    visit_type_uint32(v, name, &value, errp);
  +    visit_type_uint32(v, name, &backend->conf.peers.queues, errp);
   }

   static void
   cryptodev_backend_set_queues(Object *obj, Visitor *v, const char *name,
                                void *opaque, Error **errp)
   {
       CryptoDevBackend *backend = CRYPTODEV_BACKEND(obj);
       Error *local_err = NULL;
  -    uint32_t value;

  -    visit_type_uint32(v, name, &value, &local_err);
  +    visit_type_uint32(v, name, &backend->conf.peers.queues, &local_err);
       if (local_err) {
           goto out;
       }
  -    if (!value) {
  +    if (!backend->conf.peers.queues) {
           error_setg(&local_err, "Property '%s.%s' doesn't take value '%"
  -                   PRIu32 "'", object_get_typename(obj), name, value);
  +                   PRIu32 "'", object_get_typename(obj), name,
  +                   backend->conf.peers.queues);
  -        goto out;
       }
  -    backend->conf.peers.queues = value;
  -out:
       error_propagate(errp, local_err);
   }

If we need to preserve backend->conf.peers.queue on an attempt to set it
to zero, things get a bit more complicated.  But having the compiler
enforce the visitor matches the member type exactly is worth it, in my
opinion.

Separate patch, not necessarily in this series.

> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index f9218aa952..9aed6225bf 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -279,19 +279,19 @@ static void i440fx_pcihost_initfn(Object *obj)
>      memory_region_init_io(&s->data_mem, obj, &pci_host_data_le_ops, s,
>                            "pci-conf-data", 4);
>  
> -    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "uint32",
>                          i440fx_pcihost_get_pci_hole_start,
>                          NULL, NULL, NULL, NULL);
>  
> -    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_END, "int",
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_END, "uint32",
>                          i440fx_pcihost_get_pci_hole_end,
>                          NULL, NULL, NULL, NULL);
>  
> -    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_START, "int",
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_START, "uint64",
>                          i440fx_pcihost_get_pci_hole64_start,
>                          NULL, NULL, NULL, NULL);
>  
> -    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "int",
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "uint64",
>                          i440fx_pcihost_get_pci_hole64_end,
>                          NULL, NULL, NULL, NULL);
>  }

These look good.  The underlying values are 64 bits, but the 32 bit
callbacks assert they fit into 32.

> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 344f77b10c..5438be8253 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -176,23 +176,23 @@ static void q35_host_initfn(Object *obj)
>      qdev_prop_set_uint32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
>      qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
>  
> -    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "uint32",
>                          q35_host_get_pci_hole_start,
>                          NULL, NULL, NULL, NULL);
>  
> -    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_END, "int",
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_END, "uint32",
>                          q35_host_get_pci_hole_end,
>                          NULL, NULL, NULL, NULL);
>  
> -    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_START, "int",
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_START, "uint64",
>                          q35_host_get_pci_hole64_start,
>                          NULL, NULL, NULL, NULL);
>  
> -    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "int",
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "uint64",
>                          q35_host_get_pci_hole64_end,
>                          NULL, NULL, NULL, NULL);

Likewise.

>  
> -    object_property_add(obj, PCIE_HOST_MCFG_SIZE, "int",
> +    object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint32",
>                          q35_host_get_mmcfg_size,
>                          NULL, NULL, NULL, NULL);

This one leads to a bug, I think:

   static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name,
                                       void *opaque, Error **errp)
   {
       PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
       uint32_t value = e->size;

       visit_type_uint32(v, name, &value, errp);
   }

e->size is hwaddr, i.e. uint64_t.  We silently truncate.

Demonstrates that the detour through a separate variable breeds bugs and
should be avoided.

Suggested fix:

   static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name,
                                       void *opaque, Error **errp)
   {
       PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
  -    uint32_t value = e->size;

  -    visit_type_uint64(v, name, &value, errp);
  +    visit_type_uint64(v, name, &e->size, errp);
   }

You then have to change the type name to "uint64" instead of "uint32",
of course.

>  
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index d4bcdb027f..a964354081 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1110,7 +1110,7 @@ static void powernv_machine_initfn(Object *obj)
>  
>  static void powernv_machine_class_props_init(ObjectClass *oc)
>  {
> -    object_class_property_add(oc, "num-chips", "uint32_t",
> +    object_class_property_add(oc, "num-chips", "uint32",
>                                pnv_get_num_chips, pnv_set_num_chips,
>                                NULL, NULL, NULL);
>      object_class_property_set_description(oc, "num-chips",

Looks good.

> diff --git a/net/dump.c b/net/dump.c
> index 89a149b5dd..7f4d3fda52 100644
> --- a/net/dump.c
> +++ b/net/dump.c
> @@ -325,7 +325,7 @@ static void filter_dump_instance_init(Object *obj)
>  
>      nfds->maxlen = 65536;
>  
> -    object_property_add(obj, "maxlen", "int", filter_dump_get_maxlen,
> +    object_property_add(obj, "maxlen", "uint32", filter_dump_get_maxlen,
>                          filter_dump_set_maxlen, NULL, NULL, NULL);
>      object_property_add_str(obj, "file", file_dump_get_filename,
>                              file_dump_set_filename, NULL);

Same type checking weakness as in cryptodev.c.

> diff --git a/net/filter-buffer.c b/net/filter-buffer.c
> index cc6bd94445..9ce96aaa35 100644
> --- a/net/filter-buffer.c
> +++ b/net/filter-buffer.c
> @@ -191,7 +191,7 @@ out:
>  
>  static void filter_buffer_init(Object *obj)
>  {
> -    object_property_add(obj, "interval", "int",
> +    object_property_add(obj, "interval", "uint32",
>                          filter_buffer_get_interval,
>                          filter_buffer_set_interval, NULL, NULL, NULL);
>  }

Likeise.

Aside: I dislike having to define setters and getters for everything in
QOM.  It's flexible, but verbose and error-prone when the flexibility
isn't actually needed.