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

[Markus Armbruster <armbru@redhat.com>]

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

> Hi
>
> On Wed, May 17, 2017 at 12:50 PM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> 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?
>>
>
> It looks like prop->type is only used internally by qom/object.c and not
> exposed to QMP or other internal APIs, it would be nice if someone more
> familiar with it could confirm.

I see.  I can have a look once I'm done catching up with your replies.

>> How did you find the names that need fixing?
>
> Mostly by grepping and reviewing the code, I don't have a better approach..

Can't think of a better way offhand.  If something comes to me, I'll let
you know.  Thanks for going the extra mile!

>> > 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.
>>
>
> Ok, although this bug exist before the type name change, right? I'll make
> it a seperate patch

Yes, please.

[...]