| Message ID | 20230908143703.172758-3-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qdev: Make array properties user accessible again | expand |
On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote: > > Instead of manually setting "foo-len" and "foo[i]" properties, build a > QList and use the new qdev_prop_set_array() helper to set the whole > array property with a single call. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > hw/i386/pc.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 54838c0c41..0e84e454cb 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -81,6 +81,7 @@ > #include "qapi/error.h" > #include "qapi/qapi-visit-common.h" > #include "qapi/qapi-visit-machine.h" > +#include "qapi/qmp/qlist.h" > #include "qapi/visitor.h" > #include "hw/core/cpu.h" > #include "hw/usb.h" > @@ -1508,9 +1509,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d", > VIRTIO_IOMMU_RESV_MEM_T_MSI); > > - object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp); > - object_property_set_str(OBJECT(dev), "reserved-regions[0]", > - resv_prop_str, errp); > + QList *reserved_regions = qlist_new(); > + qlist_append_str(reserved_regions, resv_prop_str); > + qdev_prop_set_array(dev, "reserved-regions", reserved_regions); > + The variable declaration should be at the top of the block; otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
Am 11.09.2023 um 17:42 hat Peter Maydell geschrieben: > On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote: > > > > Instead of manually setting "foo-len" and "foo[i]" properties, build a > > QList and use the new qdev_prop_set_array() helper to set the whole > > array property with a single call. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > hw/i386/pc.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 54838c0c41..0e84e454cb 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -81,6 +81,7 @@ > > #include "qapi/error.h" > > #include "qapi/qapi-visit-common.h" > > #include "qapi/qapi-visit-machine.h" > > +#include "qapi/qmp/qlist.h" > > #include "qapi/visitor.h" > > #include "hw/core/cpu.h" > > #include "hw/usb.h" > > @@ -1508,9 +1509,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > > char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d", > > VIRTIO_IOMMU_RESV_MEM_T_MSI); > > > > - object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp); > > - object_property_set_str(OBJECT(dev), "reserved-regions[0]", > > - resv_prop_str, errp); > > + QList *reserved_regions = qlist_new(); > > + qlist_append_str(reserved_regions, resv_prop_str); > > + qdev_prop_set_array(dev, "reserved-regions", reserved_regions); > > + > > The variable declaration should be at the top of the block; It is at the top of the block, the only thing before it is another variable declaration. Would you prefer to have the empty line removed or after the declaration to make this visually clearer? Kevin
On Mon, 11 Sept 2023 at 17:42, Kevin Wolf <kwolf@redhat.com> wrote: > > Am 11.09.2023 um 17:42 hat Peter Maydell geschrieben: > > On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > Instead of manually setting "foo-len" and "foo[i]" properties, build a > > > QList and use the new qdev_prop_set_array() helper to set the whole > > > array property with a single call. > > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > --- > > > hw/i386/pc.c | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > index 54838c0c41..0e84e454cb 100644 > > > --- a/hw/i386/pc.c > > > +++ b/hw/i386/pc.c > > > @@ -81,6 +81,7 @@ > > > #include "qapi/error.h" > > > #include "qapi/qapi-visit-common.h" > > > #include "qapi/qapi-visit-machine.h" > > > +#include "qapi/qmp/qlist.h" > > > #include "qapi/visitor.h" > > > #include "hw/core/cpu.h" > > > #include "hw/usb.h" > > > @@ -1508,9 +1509,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > > > char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d", > > > VIRTIO_IOMMU_RESV_MEM_T_MSI); > > > > > > - object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp); > > > - object_property_set_str(OBJECT(dev), "reserved-regions[0]", > > > - resv_prop_str, errp); > > > + QList *reserved_regions = qlist_new(); > > > + qlist_append_str(reserved_regions, resv_prop_str); > > > + qdev_prop_set_array(dev, "reserved-regions", reserved_regions); > > > + > > > > The variable declaration should be at the top of the block; > > It is at the top of the block, the only thing before it is another > variable declaration. Would you prefer to have the empty line removed or > after the declaration to make this visually clearer? Sorry, I think I just misread the diff somehow. I guess that having the blank line after the variable declarations would be more usual, but it doesn't really matter. -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 11 Sept 2023 at 17:42, Kevin Wolf <kwolf@redhat.com> wrote: >> >> Am 11.09.2023 um 17:42 hat Peter Maydell geschrieben: >> > On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote: >> > > >> > > Instead of manually setting "foo-len" and "foo[i]" properties, build a >> > > QList and use the new qdev_prop_set_array() helper to set the whole >> > > array property with a single call. >> > > >> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> > > --- >> > > hw/i386/pc.c | 8 +++++--- >> > > 1 file changed, 5 insertions(+), 3 deletions(-) >> > > >> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> > > index 54838c0c41..0e84e454cb 100644 >> > > --- a/hw/i386/pc.c >> > > +++ b/hw/i386/pc.c >> > > @@ -81,6 +81,7 @@ >> > > #include "qapi/error.h" >> > > #include "qapi/qapi-visit-common.h" >> > > #include "qapi/qapi-visit-machine.h" >> > > +#include "qapi/qmp/qlist.h" >> > > #include "qapi/visitor.h" >> > > #include "hw/core/cpu.h" >> > > #include "hw/usb.h" >> > > @@ -1508,9 +1509,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, >> > > char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d", >> > > VIRTIO_IOMMU_RESV_MEM_T_MSI); >> > > >> > > - object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp); >> > > - object_property_set_str(OBJECT(dev), "reserved-regions[0]", >> > > - resv_prop_str, errp); >> > > + QList *reserved_regions = qlist_new(); >> > > + qlist_append_str(reserved_regions, resv_prop_str); >> > > + qdev_prop_set_array(dev, "reserved-regions", reserved_regions); >> > > + >> > >> > The variable declaration should be at the top of the block; >> >> It is at the top of the block, the only thing before it is another >> variable declaration. Would you prefer to have the empty line removed or >> after the declaration to make this visually clearer? > > Sorry, I think I just misread the diff somehow. I guess that having > the blank line after the variable declarations would be more usual, > but it doesn't really matter. I really like to have a blank line between declarations and statements, because declaration vs. statement can be difficult to decide at a glance, and the blank line convention helps.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 54838c0c41..0e84e454cb 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -81,6 +81,7 @@ #include "qapi/error.h" #include "qapi/qapi-visit-common.h" #include "qapi/qapi-visit-machine.h" +#include "qapi/qmp/qlist.h" #include "qapi/visitor.h" #include "hw/core/cpu.h" #include "hw/usb.h" @@ -1508,9 +1509,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d", VIRTIO_IOMMU_RESV_MEM_T_MSI); - object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp); - object_property_set_str(OBJECT(dev), "reserved-regions[0]", - resv_prop_str, errp); + QList *reserved_regions = qlist_new(); + qlist_append_str(reserved_regions, resv_prop_str); + qdev_prop_set_array(dev, "reserved-regions", reserved_regions); + g_free(resv_prop_str); }
Instead of manually setting "foo-len" and "foo[i]" properties, build a QList and use the new qdev_prop_set_array() helper to set the whole array property with a single call. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- hw/i386/pc.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)