Re: [PATCH 02/11] hw/i386/pc: Use qdev_prop_set_array()
Peter Maydell writes:
> On Mon, 11 Sept 2023 at 17:42, Kevin Wolf wrote:
>>
>> Am 11.09.2023 um 17:42 hat Peter Maydell geschrieben:
>> > On Fri, 8 Sept 2023 at 15:37, Kevin Wolf 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
>> > > ---
>> > > 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("0xfee0:0xfeef:%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.
Re: [PATCH 02/11] hw/i386/pc: Use qdev_prop_set_array()
On Mon, 11 Sept 2023 at 17:42, Kevin Wolf wrote:
>
> Am 11.09.2023 um 17:42 hat Peter Maydell geschrieben:
> > On Fri, 8 Sept 2023 at 15:37, Kevin Wolf 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
> > > ---
> > > 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("0xfee0:0xfeef:%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
Re: [PATCH 02/11] hw/i386/pc: Use qdev_prop_set_array()
Am 11.09.2023 um 17:42 hat Peter Maydell geschrieben:
> On Fri, 8 Sept 2023 at 15:37, Kevin Wolf 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
> > ---
> > 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("0xfee0:0xfeef:%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
Re: [PATCH 02/11] hw/i386/pc: Use qdev_prop_set_array()
On Fri, 8 Sept 2023 at 15:37, Kevin Wolf 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
> ---
> 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("0xfee0:0xfeef:%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
thanks
-- PMM
[PATCH 02/11] hw/i386/pc: Use qdev_prop_set_array()
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
---
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("0xfee0:0xfeef:%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);
}
--
2.41.0
