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

[Eduardo Habkost]

On Wed, Mar 08, 2017 at 09:06:06PM +0200, Krzysztof Kozlowski wrote:
> On Wed, Mar 08, 2017 at 03:57:26PM -0300, Eduardo Habkost wrote:
> > On Wed, Mar 08, 2017 at 06:22:13PM +0200, Krzysztof Kozlowski wrote:
> > > 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.
> > 
> > This code path is correct, yes. That's why I don't mind either
> > way. What I find misleading is that the object_property_set_str()
> > call will end up setting a pointer inside the object to 'bs', and
> > that pointer is not const.
> 
> No, I believe it won't. The qstring_from_str() makes a copy of passed
> value. That is also the whole idea of pointer to const for input
> argument - a commitment that this data will not be stored for later.

It will set a non-const pointer to 'value' (take a look at
set_drive()). But I was wrong because I was talking about 'bs',
not 'value'. I see no problems in making 'bs' const.

> 
> > 
> > > 
> > > > 
> > > > > @@ -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().
> > 
> > I don't see the mistake. The whole purpose of:
> >   qdev_prop_set_chr(dev, "some-field", v)
> > is to end up doing this assignment internally:
> >   dev->some_field = v;
> > and on most (or all?) cases dev->some_field is not a const
> > pointer. The details are hidden behind the
> > object_property_set_str() call.
> 
> If that would be the case, the object_property_set_str() cannot take a
> pinter to const. Not only because of the safety and logic but also C
> will prohibit it without a case.
> 
>       const char *c = "foo bar";
>       char *v = c;
> 
>       /home/dev/qemu/qemu/qobject/qstring.c:67:15: error: initialization 
> discards ‘const’ qualifier from pointer target type 
> [-Werror=discarded-qualifiers]
>            char *v = c;
>                ^

The 'value' parameter to object_property_set_str() is const, and
that's correct. But the set_chr() setter will take care of the
'dev->some_field = value' part.

-- 
Eduardo