Re: [PATCH v4 23/32] qdev: Move dev->realized check to qdev_property_set()
On Mon, 14 Dec 2020 12:24:18 -0500
Eduardo Habkost wrote:
> On Mon, Dec 14, 2020 at 03:55:30PM +0100, Igor Mammedov wrote:
> > On Fri, 11 Dec 2020 17:05:20 -0500
> > Eduardo Habkost wrote:
> >
> > > Every single qdev property setter function manually checks
> > > dev->realized. We can just check dev->realized inside
> > > qdev_property_set() instead.
> > >
> > > The check is being added as a separate function
> > > (qdev_prop_allow_set()) because it will become a callback later.
> >
> > is callback added within this series?
> > and I'd add here what's the purpose of it.
>
> It will be added in part 2 of the series. See v3:
> https://lore.kernel.org/qemu-devel/20201112214350.872250-35-ehabk...@redhat.com/
>
> I don't know what else I could say about its purpose, in addition
> to what I wrote above, and the comment below[1].
>
> If you are just curious about the callback and confused because
> it is not anywhere in this series, I can just remove the
> paragraph above from the commit message. Would that be enough?
lets remove it for now to avoid confusion
Reviewed-by: Igor Mammedov
>
> >
> [...]
> > > +/* returns: true if property is allowed to be set, false otherwise */
>
> [1] ^^^
>
> > > +static bool qdev_prop_allow_set(Object *obj, const char *name,
> > > +Error **errp)
> > > +{
> > > +DeviceState *dev = DEVICE(obj);
> > > +
> > > +if (dev->realized) {
> > > +qdev_prop_set_after_realize(dev, name, errp);
> > > +return false;
> > > +}
> > > +return true;
> > > +}
> > > +
>
Re: [PATCH v4 23/32] qdev: Move dev->realized check to qdev_property_set()
On Mon, Dec 14, 2020 at 03:55:30PM +0100, Igor Mammedov wrote:
> On Fri, 11 Dec 2020 17:05:20 -0500
> Eduardo Habkost wrote:
>
> > Every single qdev property setter function manually checks
> > dev->realized. We can just check dev->realized inside
> > qdev_property_set() instead.
> >
> > The check is being added as a separate function
> > (qdev_prop_allow_set()) because it will become a callback later.
>
> is callback added within this series?
> and I'd add here what's the purpose of it.
It will be added in part 2 of the series. See v3:
https://lore.kernel.org/qemu-devel/20201112214350.872250-35-ehabk...@redhat.com/
I don't know what else I could say about its purpose, in addition
to what I wrote above, and the comment below[1].
If you are just curious about the callback and confused because
it is not anywhere in this series, I can just remove the
paragraph above from the commit message. Would that be enough?
>
[...]
> > +/* returns: true if property is allowed to be set, false otherwise */
[1] ^^^
> > +static bool qdev_prop_allow_set(Object *obj, const char *name,
> > +Error **errp)
> > +{
> > +DeviceState *dev = DEVICE(obj);
> > +
> > +if (dev->realized) {
> > +qdev_prop_set_after_realize(dev, name, errp);
> > +return false;
> > +}
> > +return true;
> > +}
> > +
--
Eduardo
Re: [PATCH v4 23/32] qdev: Move dev->realized check to qdev_property_set()
On Fri, 11 Dec 2020 17:05:20 -0500
Eduardo Habkost wrote:
> Every single qdev property setter function manually checks
> dev->realized. We can just check dev->realized inside
> qdev_property_set() instead.
>
> The check is being added as a separate function
> (qdev_prop_allow_set()) because it will become a callback later.
is callback added within this series?
and I'd add here what's the purpose of it.
>
> Reviewed-by: Stefan Berger
> Signed-off-by: Eduardo Habkost
> ---
> Changes v1 -> v2:
> * Removed unused variable at xen_block_set_vdev()
> * Redone patch after changes in the previous patches in the
> series
> ---
> Cc: Stefan Berger
> Cc: Stefano Stabellini
> Cc: Anthony Perard
> Cc: Paul Durrant
> Cc: Kevin Wolf
> Cc: Max Reitz
> Cc: Paolo Bonzini
> Cc: "Daniel P. Berrangé"
> Cc: Eduardo Habkost
> Cc: Cornelia Huck
> Cc: Halil Pasic
> Cc: Christian Borntraeger
> Cc: Richard Henderson
> Cc: David Hildenbrand
> Cc: Thomas Huth
> Cc: Matthew Rosato
> Cc: Alex Williamson
> Cc: Mark Cave-Ayland
> Cc: Artyom Tarasenko
> Cc: qemu-de...@nongnu.org
> Cc: xen-de...@lists.xenproject.org
> Cc: qemu-block@nongnu.org
> Cc: qemu-s3...@nongnu.org
> ---
> backends/tpm/tpm_util.c | 6 --
> hw/block/xen-block.c | 6 --
> hw/core/qdev-properties-system.c | 70 --
> hw/core/qdev-properties.c| 100 ++-
> hw/s390x/css.c | 6 --
> hw/s390x/s390-pci-bus.c | 6 --
> hw/vfio/pci-quirks.c | 6 --
> target/sparc/cpu.c | 6 --
> 8 files changed, 18 insertions(+), 188 deletions(-)
>
> diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
> index a5d997e7dc..39b45fa46d 100644
> --- a/backends/tpm/tpm_util.c
> +++ b/backends/tpm/tpm_util.c
> @@ -46,16 +46,10 @@ static void get_tpm(Object *obj, Visitor *v, const char
> *name, void *opaque,
> static void set_tpm(Object *obj, Visitor *v, const char *name, void *opaque,
> Error **errp)
> {
> -DeviceState *dev = DEVICE(obj);
> Property *prop = opaque;
> TPMBackend *s, **be = qdev_get_prop_ptr(obj, prop);
> char *str;
>
> -if (dev->realized) {
> -qdev_prop_set_after_realize(dev, name, errp);
> -return;
> -}
> -
> if (!visit_type_str(v, name, , errp)) {
> return;
> }
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 905e4acd97..bd1aef63a7 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -395,17 +395,11 @@ static int vbd_name_to_disk(const char *name, const
> char **endp,
> static void xen_block_set_vdev(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> -DeviceState *dev = DEVICE(obj);
> Property *prop = opaque;
> XenBlockVdev *vdev = qdev_get_prop_ptr(obj, prop);
> char *str, *p;
> const char *end;
>
> -if (dev->realized) {
> -qdev_prop_set_after_realize(dev, name, errp);
> -return;
> -}
> -
> if (!visit_type_str(v, name, , errp)) {
> return;
> }
> diff --git a/hw/core/qdev-properties-system.c
> b/hw/core/qdev-properties-system.c
> index 42529c3b65..f31aea3de1 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -94,11 +94,6 @@ static void set_drive_helper(Object *obj, Visitor *v,
> const char *name,
> bool blk_created = false;
> int ret;
>
> -if (dev->realized) {
> -qdev_prop_set_after_realize(dev, name, errp);
> -return;
> -}
> -
> if (!visit_type_str(v, name, , errp)) {
> return;
> }
> @@ -230,17 +225,11 @@ static void get_chr(Object *obj, Visitor *v, const char
> *name, void *opaque,
> static void set_chr(Object *obj, Visitor *v, const char *name, void *opaque,
> Error **errp)
> {
> -DeviceState *dev = DEVICE(obj);
> Property *prop = opaque;
> CharBackend *be = qdev_get_prop_ptr(obj, prop);
> Chardev *s;
> char *str;
>
> -if (dev->realized) {
> -qdev_prop_set_after_realize(dev, name, errp);
> -return;
> -}
> -
> if (!visit_type_str(v, name, , errp)) {
> return;
> }
> @@ -311,18 +300,12 @@ static void get_mac(Object *obj, Visitor *v, const char
> *name, void *opaque,
> static void set_mac(Object *obj, Visitor *v, const char *name, void *opaque,
> Error **errp)
> {
> -DeviceState *dev = DEVICE(obj);
> Property *prop = opaque;
> MACAddr *mac = qdev_get_prop_ptr(obj, prop);
> int i, pos;
> char *str;
> const char *p;
>
> -if (dev->realized) {
> -qdev_prop_set_after_realize(dev, name, errp);
> -return;
> -}
> -
> if (!visit_type_str(v, name, , errp)) {
> return;
> }
> @@ -390,7 +373,6 @@ static void get_netdev(Object *obj, Visitor *v, const
> char *name,
> static void
Re: [PATCH v4 23/32] qdev: Move dev->realized check to qdev_property_set()
On Fri, 11 Dec 2020 17:05:20 -0500 Eduardo Habkost wrote: > Every single qdev property setter function manually checks > dev->realized. We can just check dev->realized inside > qdev_property_set() instead. > > The check is being added as a separate function > (qdev_prop_allow_set()) because it will become a callback later. > > Reviewed-by: Stefan Berger > Signed-off-by: Eduardo Habkost > --- > Changes v1 -> v2: > * Removed unused variable at xen_block_set_vdev() > * Redone patch after changes in the previous patches in the > series > --- > Cc: Stefan Berger > Cc: Stefano Stabellini > Cc: Anthony Perard > Cc: Paul Durrant > Cc: Kevin Wolf > Cc: Max Reitz > Cc: Paolo Bonzini > Cc: "Daniel P. Berrangé" > Cc: Eduardo Habkost > Cc: Cornelia Huck > Cc: Halil Pasic > Cc: Christian Borntraeger > Cc: Richard Henderson > Cc: David Hildenbrand > Cc: Thomas Huth > Cc: Matthew Rosato > Cc: Alex Williamson > Cc: Mark Cave-Ayland > Cc: Artyom Tarasenko > Cc: qemu-de...@nongnu.org > Cc: xen-de...@lists.xenproject.org > Cc: qemu-block@nongnu.org > Cc: qemu-s3...@nongnu.org > --- > backends/tpm/tpm_util.c | 6 -- > hw/block/xen-block.c | 6 -- > hw/core/qdev-properties-system.c | 70 -- > hw/core/qdev-properties.c| 100 ++- > hw/s390x/css.c | 6 -- > hw/s390x/s390-pci-bus.c | 6 -- > hw/vfio/pci-quirks.c | 6 -- > target/sparc/cpu.c | 6 -- > 8 files changed, 18 insertions(+), 188 deletions(-) Reviewed-by: Cornelia Huck
RE: [PATCH v4 23/32] qdev: Move dev->realized check to qdev_property_set()
> -Original Message- > From: Eduardo Habkost > Sent: 11 December 2020 22:05 > To: qemu-de...@nongnu.org > Cc: Markus Armbruster ; Igor Mammedov > ; Stefan Berger > ; Marc-André Lureau ; > Daniel P. Berrange > ; Philippe Mathieu-Daudé ; John Snow > ; Kevin > Wolf ; Eric Blake ; Paolo Bonzini > ; Stefan > Berger ; Stefano Stabellini > ; Anthony Perard > ; Paul Durrant ; Max Reitz > ; Cornelia Huck > ; Halil Pasic ; Christian Borntraeger > ; Richard Henderson ; David > Hildenbrand ; > Thomas Huth ; Matthew Rosato ; Alex > Williamson > ; Mark Cave-Ayland > ; Artyom Tarasenko > ; xen-de...@lists.xenproject.org; qemu-block@nongnu.org; > qemu-s3...@nongnu.org > Subject: [PATCH v4 23/32] qdev: Move dev->realized check to > qdev_property_set() > > Every single qdev property setter function manually checks > dev->realized. We can just check dev->realized inside > qdev_property_set() instead. > > The check is being added as a separate function > (qdev_prop_allow_set()) because it will become a callback later. > > Reviewed-by: Stefan Berger > Signed-off-by: Eduardo Habkost Xen parts... Acked-by: Paul Durrant
