On Wed, Mar 08, 2017 at 09:34:19PM +0200, Krzysztof Kozlowski wrote:
> 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)
I assume you meant:
qdev_prop_set_chr(Chardev *value)
> const char *l = value->label
> object_property_set_str(const char *l)
This is correct, but:
> dev->some_field = copy_of(l);
This is not what object_property_set_str() does. It will end up
calling the set_chr() setter, which will end up getting 'value'
back as a non-const pointer, and doing 'dev->some_field = value'.
>
> 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.
I believe you got the point. Except that this is not a planned
implementation change, but the current and expected behavior of
qdev_prop_set_chr().
>
> 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.
The current implementation _will_ store 'value' in
dev->some_field (in a very convoluted way, through the set_chr()
setter), as this is its whole purpose.
>
> 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).
OK. Thanks!
>
> Best regards,
> Krzysztof
>
--
Eduardo
