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

[Krzysztof Kozlowski]

On Wed, Mar 08, 2017 at 04:22:01PM -0300, Eduardo Habkost wrote:
> On Wed, Mar 08, 2017 at 09:06:06PM +0200, Krzysztof Kozlowski wrote:
> > > > 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.

In current implementation (v2.8.0-2132-gb64842dee42d) the
only thing qdev_prop_set_chr() does is to call
object_property_set_str() for value->label.

So the flow is:
    qdev_prop_set_chr(char *value)
        const char *l = value->label
        object_property_set_str(const char *l)
            dev->some_field = copy_of(l);

The only non-const part of the flow is the first call. All of others are
const.

Of course the implementation might change and maybe that is the
intention/plan - the qdev_prop_set_chr() should not commit to the caller
that it will not store the value itself.

Then I understand it.

However if there are no such plans, then in current implementation the
qdev_prop_set_chr() does not store any part of the 'value' itself but
only a copy of it through object_property_set_str(). Thus it can provide
this hint to the caller: I will not store the 'value' directly so I am
taking pointer to const.

Anyway, this is a trivial, boring and correctness-like change. :) Not worth
all the talks so I do not mind resending without this (and others which
were disapproved).

Best regards,
Krzysztof