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 <[email protected]> > > > > 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
