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 <k...@kernel.org>
>
> 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.
>
> > @@ -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().
>
>
> > {
> > assert(!value || value->label);
> > object_property_set_str(OBJECT(dev),
> > @@ -424,7 +424,7 @@ void qdev_prop_set_chr(DeviceState *dev, const char
> > *name,
> > }
> >
> > void qdev_prop_set_netdev(DeviceState *dev, const char *name,
> > - NetClientState *value)
> > + const NetClientState *value)
>
> Same case here: the wrapper will store 'value' in a non-const
> NetClientState pointer in the object.
>
> > {
> > assert(!value || value->name);
> > object_property_set_str(OBJECT(dev),
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 6ab4265eb478..34ec10f0caac 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -1010,7 +1010,8 @@ void qdev_prop_set_string(DeviceState *dev, const
> > char *name, const char *value)
> > object_property_set_str(OBJECT(dev), value, name, &error_abort);
> > }
> >
> > -void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t
> > *value)
> > +void qdev_prop_set_macaddr(DeviceState *dev, const char *name,
> > + const uint8_t *value)
>
> This one makes sense to me.
>
> > {
> > char str[2 * 6 + 5 + 1];
> > snprintf(str, sizeof(str), "%02x:%02x:%02x:%02x:%02x:%02x",
> > @@ -1028,10 +1029,10 @@ void qdev_prop_set_enum(DeviceState *dev, const
> > char *name, int value)
> > name, &error_abort);
> > }
> >
> > -void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
> > +void qdev_prop_set_ptr(DeviceState *dev, const char *name, const void
> > *value)
> > {
> > Property *prop;
> > - void **ptr;
> > + const void **ptr;
>
> This one is also misleading: the field pointed by
> qdev_prop_find() is normally a non-const pointer.
Understood, I'll drop it.
Best regards,
Krzysztof
