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

[Philippe Mathieu-Daudé]

On 7/6/20 12:01 PM, Marc-André Lureau wrote:
> 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.

I totally understand, but at the same time we are excluding hobbyist
contributors with mainstream exigency.

This is unfortunate. I'd like to suggest ideas how to keep the QEMU
project open to non-corporate contributors, but I don't have any.

Well, maybe this one: better document API changes, if possible with
examples (example can be as simple as pointing to a particular commit
sha1). At least for each release, because forks usually try to rebase
after releases.

> 
>>> -
>>>  #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;
>>>
>>
> 
>