qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] qdev: Constify data pointed by few arguments an


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH] qdev: Constify data pointed by few arguments and local variables
Date: Wed, 8 Mar 2017 16:53:23 -0300
User-agent: Mutt/1.7.1 (2016-10-04)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]