[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 15:57:26 -0300 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
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.
>
> >
> > > @@ -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.
That means code must never call qdev_prop_set_chr(dev, field, v)
if 'v' is a const pointer.
--
Eduardo