| Message ID | 20170305214633.19844-1-krzk@kernel.org |
|---|---|
| State | New |
| Headers | show |
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 <krzk@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. > @@ -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. > { > 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. > > prop = qdev_prop_find(dev, name); > assert(prop && prop->info == &qdev_prop_ptr); > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h > index 7ac315331aa0..659561daad0d 100644 > --- a/include/hw/qdev-properties.h > +++ b/include/hw/qdev-properties.h > @@ -184,14 +184,17 @@ void qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value); > void qdev_prop_set_int32(DeviceState *dev, const char *name, int32_t value); > void qdev_prop_set_uint64(DeviceState *dev, const char *name, uint64_t value); > void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value); > -void qdev_prop_set_chr(DeviceState *dev, const char *name, Chardev *value); > -void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState *value); > +void qdev_prop_set_chr(DeviceState *dev, const char *name, > + const Chardev *value); > +void qdev_prop_set_netdev(DeviceState *dev, const char *name, > + const NetClientState *value); > void qdev_prop_set_drive(DeviceState *dev, const char *name, > BlockBackend *value, Error **errp); > -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); > void qdev_prop_set_enum(DeviceState *dev, const char *name, int value); > /* FIXME: Remove opaque pointer properties. */ > -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); > > void qdev_prop_register_global(GlobalProperty *prop); > void qdev_prop_register_global_list(GlobalProperty *props); > -- > 2.9.3 >
On 06/03/2017 13:09, Eduardo Habkost wrote: >> >> -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. Indeed. Krzysztof, you could extract this one and send it to qemu-trivial@nongnu.org. Thanks, Paolo
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 <krzk@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
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 <krzk@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. 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.
On Wed, Mar 08, 2017 at 03:57:26PM -0300, Eduardo Habkost wrote: > 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 <krzk@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. > > 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. No, I believe it won't. The qstring_from_str() makes a copy of passed value. That is also the whole idea of pointer to const for input argument - a commitment that this data will not be stored for later. > > > > > > > > > > @@ -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. 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; ^ Best regards, Krzysztof
On Wed, Mar 08, 2017 at 09:06:06PM +0200, Krzysztof Kozlowski wrote: > On Wed, Mar 08, 2017 at 03:57:26PM -0300, Eduardo Habkost wrote: > > 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 <krzk@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. > > > > 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. > > No, I believe it won't. The qstring_from_str() makes a copy of passed > value. That is also the whole idea of pointer to const for input > argument - a commitment that this data will not be stored for later. It will set a non-const pointer to 'value' (take a look at set_drive()). But I was wrong because I was talking about 'bs', not 'value'. I see no problems in making 'bs' 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. > > 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.
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) const char *l = value->label object_property_set_str(const char *l) dev->some_field = copy_of(l); 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. 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. 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). Best regards, Krzysztof
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 >
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); } @@ -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) { 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) { 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) { 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; prop = qdev_prop_find(dev, name); assert(prop && prop->info == &qdev_prop_ptr); diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index 7ac315331aa0..659561daad0d 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -184,14 +184,17 @@ void qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value); void qdev_prop_set_int32(DeviceState *dev, const char *name, int32_t value); void qdev_prop_set_uint64(DeviceState *dev, const char *name, uint64_t value); void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value); -void qdev_prop_set_chr(DeviceState *dev, const char *name, Chardev *value); -void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState *value); +void qdev_prop_set_chr(DeviceState *dev, const char *name, + const Chardev *value); +void qdev_prop_set_netdev(DeviceState *dev, const char *name, + const NetClientState *value); void qdev_prop_set_drive(DeviceState *dev, const char *name, BlockBackend *value, Error **errp); -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); void qdev_prop_set_enum(DeviceState *dev, const char *name, int value); /* FIXME: Remove opaque pointer properties. */ -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); void qdev_prop_register_global(GlobalProperty *prop); void qdev_prop_register_global_list(GlobalProperty *props);
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 <krzk@kernel.org> --- 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(-)