Re: [PULL 36/37] qdev: remove QDEV_PROP_PTR

[Marc-André Lureau]

Hi

On Mon, Jul 6, 2020 at 12:44 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 1/7/20 4:04 PM, Marc-André Lureau wrote:
> > No longer used in the tree. The comment about user_creatable is still
> > quite relevant, but there is already a similar comment in qdev-core.h.
> >
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/hw/qdev-properties.h | 22 ----------------------
> >  hw/core/qdev-properties.c    | 18 ------------------
> >  2 files changed, 40 deletions(-)
> >
> > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > index 16837ab5dd..a90a9cec80 100644
> > --- a/include/hw/qdev-properties.h
> > +++ b/include/hw/qdev-properties.h
> > @@ -18,7 +18,6 @@ extern const PropertyInfo qdev_prop_size;
> >  extern const PropertyInfo qdev_prop_string;
> >  extern const PropertyInfo qdev_prop_chr;
> >  extern const PropertyInfo qdev_prop_tpm;
> > -extern const PropertyInfo qdev_prop_ptr;
> >  extern const PropertyInfo qdev_prop_macaddr;
> >  extern const PropertyInfo qdev_prop_on_off_auto;
> >  extern const PropertyInfo qdev_prop_losttickpolicy;
> > @@ -171,25 +170,6 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
> >  #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
> >      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
> >
> > -/*
> > - * Please avoid pointer properties.  If you must use them, you must
> > - * cover them in their device's class init function as follows:
> > - *
> > - * - If the property must be set, the device cannot be used with
> > - *   device_add, so add code like this:
> > - *   |* Reason: pointer property "NAME-OF-YOUR-PROP" *|
> > - *   DeviceClass *dc = DEVICE_CLASS(class);
> > - *   dc->user_creatable = false;
> > - *
> > - * - If the property may safely remain null, document it like this:
> > - *   |*
> > - *    * Note: pointer property "interrupt_vector" may remain null, thus
> > - *    * no need for dc->user_creatable = false;
> > - *    *|
> > - */
> > -#define DEFINE_PROP_PTR(_n, _s, _f)             \
> > -    DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, void*)
>
> So the replacement is DEFINE_PROP_LINK()? It is not documented, but
> it takes a TYPENAME argument, so I assume we can only LINK QOM types.
> Can the documentation be improved?

Certainly, although we already have some documentation in
object_property_add_link(). It's annoying that we have several places
documenting similar/close API, or API docs in docs/ that quickly
becomes outdated, or more difficult to find. Root of the issue is that
there are at least 3 ways to add props: object_{,class}_property &
QDEV_PROPS ...

> Yet another sneaky way to force forks to either update to QOM or die...

You can't blame upstream from doing cleanups and modernization, or
stagnating. Forks are forks, with all the pain they carry. If they
want to avoid the maintenance cost, they have to do the extra effort
to get it upstream. This is also a "sneaky way" to remind them that
effort is better spent in this direction.

> > -
> >  #define DEFINE_PROP_CHR(_n, _s, _f)             \
> >      DEFINE_PROP(_n, _s, _f, qdev_prop_chr, CharBackend)
> >  #define DEFINE_PROP_STRING(_n, _s, _f)             \
> > @@ -262,8 +242,6 @@ void qdev_prop_set_drive(DeviceState *dev, const char 
> > *name,
> >  void qdev_prop_set_macaddr(DeviceState *dev, const char *name,
> >                             const uint8_t *value);
> >  void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
> > -/* FIXME: Remove opaque pointer properties.  */
> > -void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
> >
> >  void qdev_prop_register_global(GlobalProperty *prop);
> >  int qdev_prop_check_globals(void);
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index ac28890e5a..6ca7697599 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -501,13 +501,6 @@ const PropertyInfo qdev_prop_string = {
> >      .set   = set_string,
> >  };
> >
> > -/* --- pointer --- */
> > -
> > -/* Not a proper property, just for dirty hacks.  TODO Remove it!  */
> > -const PropertyInfo qdev_prop_ptr = {
> > -    .name  = "ptr",
> > -};
> > -
> >  /* --- mac address --- */
> >
> >  /*
> > @@ -1165,17 +1158,6 @@ void qdev_prop_set_enum(DeviceState *dev, const char 
> > *name, int value)
> >                              name, &error_abort);
> >  }
> >
> > -void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
> > -{
> > -    Property *prop;
> > -    void **ptr;
> > -
> > -    prop = qdev_prop_find(dev, name);
> > -    assert(prop && prop->info == &qdev_prop_ptr);
> > -    ptr = qdev_get_prop_ptr(dev, prop);
> > -    *ptr = value;
> > -}
> > -
> >  static GPtrArray *global_props(void)
> >  {
> >      static GPtrArray *gp;
> >
>