Re: [Qemu-devel] [PATCH] qdev: Constify data pointed by few arguments and local variables

[Krzysztof Kozlowski]

On Mon, Mar 06, 2017 at 09:09:48AM -0300, Eduardo Habkost wrote:
> Hi,
> 
> 
> On Sun, Mar 05, 2017 at 11:46:33PM +0200, Krzysztof Kozlowski wrote:
> > In few places the function arguments and local variables are not
> > modifying data passed through pointers so this can be made const for
> > code safeness.
> > 
> > Signed-off-by: Krzysztof Kozlowski <address@hidden>
> 
> I believe most changes below are misleading to users of the API,
> and make the code less safe. Most of the pointers passed as
> argument to those functions will be stored at non-const pointer
> fields inside the objects.
> 
> > ---
> >  hw/core/qdev-properties-system.c |  6 +++---
> >  hw/core/qdev-properties.c        |  7 ++++---
> >  include/hw/qdev-properties.h     | 11 +++++++----
> >  3 files changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/core/qdev-properties-system.c 
> > b/hw/core/qdev-properties-system.c
> > index c34be1c1bace..abbf3ef754d8 100644
> > --- a/hw/core/qdev-properties-system.c
> > +++ b/hw/core/qdev-properties-system.c
> > @@ -405,7 +405,7 @@ void qdev_prop_set_drive(DeviceState *dev, const char 
> > *name,
> >      if (value) {
> >          ref = blk_name(value);
> >          if (!*ref) {
> > -            BlockDriverState *bs = blk_bs(value);
> > +            const BlockDriverState *bs = blk_bs(value);
> >              if (bs) {
> >                  ref = bdrv_get_node_name(bs);
> >              }
> 
> This part looks safe, but still misleading: the
> object_property_set_str() call will end up changing a non-const
> pointer field in the object. I'm not sure what's the benefit of
> this change.

I might be missing something... but I am touching only the 'bs' and it
is passed to bdrv_get_node_name() also as const. The
bdrv_get_node_name() just accepts pointer to const.

I am not sure why are you referring to object_property_set_str(). The
value returned by bdrv_get_node_name() is pointer to const.
object_property_set_str() also takes pointer to const.

So entire path, starting from 'bs' uses pointer to const... thus I
find misleading that 'bs' is not pointing to const data. It should.

> 
> > @@ -416,7 +416,7 @@ void qdev_prop_set_drive(DeviceState *dev, const char 
> > *name,
> >  }
> >  
> >  void qdev_prop_set_chr(DeviceState *dev, const char *name,
> > -                       Chardev *value)
> > +                       const Chardev *value)
> 
> This wrapper will end up storing 'value' in a non-const pointer
> in the object (e.g. PL011State::chr). Declaring 'value' as const
> is misleading.

The object_property_set_str() takes data as pointer to const. If data
ends up as being non-const, then this is the mistake -
object_property_set_str().

> 
> 
> >  {
> >      assert(!value || value->label);
> >      object_property_set_str(OBJECT(dev),
> > @@ -424,7 +424,7 @@ void qdev_prop_set_chr(DeviceState *dev, const char 
> > *name,
> >  }
> >  
> >  void qdev_prop_set_netdev(DeviceState *dev, const char *name,
> > -                          NetClientState *value)
> > +                          const NetClientState *value)
> 
> Same case here: the wrapper will store 'value' in a non-const
> NetClientState pointer in the object.
> 
> >  {
> >      assert(!value || value->name);
> >      object_property_set_str(OBJECT(dev),
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 6ab4265eb478..34ec10f0caac 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -1010,7 +1010,8 @@ void qdev_prop_set_string(DeviceState *dev, const 
> > char *name, const char *value)
> >      object_property_set_str(OBJECT(dev), value, name, &error_abort);
> >  }
> >  
> > -void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t 
> > *value)
> > +void qdev_prop_set_macaddr(DeviceState *dev, const char *name,
> > +                           const uint8_t *value)
> 
> This one makes sense to me.
> 
> >  {
> >      char str[2 * 6 + 5 + 1];
> >      snprintf(str, sizeof(str), "%02x:%02x:%02x:%02x:%02x:%02x",
> > @@ -1028,10 +1029,10 @@ 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)
> > +void qdev_prop_set_ptr(DeviceState *dev, const char *name, const void 
> > *value)
> >  {
> >      Property *prop;
> > -    void **ptr;
> > +    const void **ptr;
> 
> This one is also misleading: the field pointed by
> qdev_prop_find() is normally a non-const pointer.

Understood, I'll drop it.

Best regards,
Krzysztof