qdev: Constify data pointed by few arguments and local variables

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]>

---
 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(-)

hw/core/qdev-properties-system.c +0 -0

index c34be1c1bace..abbf3ef754d8 100644

hw/core/qdev-properties-system.c +0 -0

 {
     assert(!value || value->name);
     object_property_set_str(OBJECT(dev),

hw/core/qdev-properties.c +0 -0

index 6ab4265eb478..34ec10f0caac 100644

hw/core/qdev-properties.c +0 -0

     prop = qdev_prop_find(dev, name);
     assert(prop && prop->info == &qdev_prop_ptr);

include/hw/qdev-properties.h +7 -4

index 7ac315331aa0..659561daad0d 100644
@@ -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);

Signature · 1 line

2.9.3

hw/core/qdev-properties.c void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value) · line 1014

Quoted 4 lines

hw/core/qdev-properties.c +2 -1

-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.

Hi,

Quoted 5 lines · Krzysztof Kozlowski

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.

Quoted 16 lines

---
 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(-)

hw/core/qdev-properties-system.c +1 -1

index c34be1c1bace..abbf3ef754d8 100644
@@ -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);

hw/core/qdev-properties-system.c void qdev_prop_set_drive(DeviceState *dev, const char *name, · line 411

Quoted 3 lines

hw/core/qdev-properties-system.c +0 -0

             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.

hw/core/qdev-properties-system.c void qdev_prop_set_drive(DeviceState *dev, const char *name, · line 419

Quoted 7 lines

hw/core/qdev-properties-system.c +1 -1

@@ -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.

hw/core/qdev-properties-system.c void qdev_prop_set_chr(DeviceState *dev, const char *name, · line 427

Quoted 11 lines

hw/core/qdev-properties-system.c +1 -1

 {
     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.

Quoted 8 lines

hw/core/qdev-properties-system.c +0 -0

 {
     assert(!value || value->name);
     object_property_set_str(OBJECT(dev),

hw/core/qdev-properties.c +0 -0

index 6ab4265eb478..34ec10f0caac 100644

hw/core/qdev-properties.c void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value) · line 1014

Quoted 6 lines

hw/core/qdev-properties.c +2 -1

@@ -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.

Quoted 7 lines

hw/core/qdev-properties.c +0 -0

 {
     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);
 }
 

hw/core/qdev-properties.c void qdev_prop_set_enum(DeviceState *dev, const char *name, int value) · line 1035

Quoted 6 lines

hw/core/qdev-properties.c +2 -2

-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.

Quoted 31 lines · Eduardo Habkost

Hi,

Quoted 5 lines · Krzysztof Kozlowski

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.

Quoted 16 lines

---
 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(-)

hw/core/qdev-properties-system.c +1 -1

index c34be1c1bace..abbf3ef754d8 100644
@@ -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);

Quoted 8 lines

hw/core/qdev-properties-system.c void qdev_prop_set_drive(DeviceState *dev, const char *name, · line 411

Quoted 3 lines

hw/core/qdev-properties-system.c +0 -0

             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.

Quoted 5 lines

Quoted 5 lines

hw/core/qdev-properties-system.c +0 -0

@@ -416,7 +416,7 @@ void qdev_prop_set_drive(DeviceState *dev, const char *name,
 }
 

Quoted 7 lines

hw/core/qdev-properties-system.c void qdev_prop_set_drive(DeviceState *dev, const char *name, · line 419

Quoted 3 lines

hw/core/qdev-properties-system.c +1 -1

 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().

Quoted 49 lines · Krzysztof Kozlowski

Quoted 31 lines · Eduardo Habkost

Hi,

Quoted 5 lines · Krzysztof Kozlowski

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.

Quoted 16 lines

---
 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(-)

hw/core/qdev-properties-system.c +1 -1

index c34be1c1bace..abbf3ef754d8 100644
@@ -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);

Quoted 8 lines

hw/core/qdev-properties-system.c void qdev_prop_set_drive(DeviceState *dev, const char *name, · line 411

Quoted 3 lines

hw/core/qdev-properties-system.c +0 -0

             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.

Quoted 2 lines

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.

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

hw/core/qdev-properties-system.c +0 -0

@@ -416,7 +416,7 @@ void qdev_prop_set_drive(DeviceState *dev, const char *name,
 }
 

Quoted 7 lines

hw/core/qdev-properties-system.c void qdev_prop_set_drive(DeviceState *dev, const char *name, · line 419

Quoted 3 lines

hw/core/qdev-properties-system.c +1 -1

 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.

Quoted 3 lines

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.

Quoted 53 lines · Eduardo Habkost

Quoted 51 lines · Krzysztof Kozlowski

Quoted 31 lines · Eduardo Habkost

Hi,

Quoted 5 lines · Krzysztof Kozlowski

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.

Quoted 16 lines

---
 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(-)

hw/core/qdev-properties-system.c +1 -1

index c34be1c1bace..abbf3ef754d8 100644
@@ -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);

Quoted 8 lines

hw/core/qdev-properties-system.c void qdev_prop_set_drive(DeviceState *dev, const char *name, · line 411

Quoted 3 lines

hw/core/qdev-properties-system.c +0 -0

             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.

Quoted 4 lines

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.

Quoted 19 lines

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

hw/core/qdev-properties-system.c +0 -0

@@ -416,7 +416,7 @@ void qdev_prop_set_drive(DeviceState *dev, const char *name,
 }
 

Quoted 7 lines

hw/core/qdev-properties-system.c void qdev_prop_set_drive(DeviceState *dev, const char *name, · line 419

Quoted 3 lines

hw/core/qdev-properties-system.c +1 -1

 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.

Quoted 3 lines

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().

Quoted 7 lines

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;
               ^

Quoted 19 lines · Krzysztof Kozlowski

Quoted 11 lines

Quoted 3 lines

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;

Quoted 3 lines

	/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.

Quoted 20 lines · Eduardo Habkost

Quoted 19 lines · Krzysztof Kozlowski

Quoted 11 lines

Quoted 3 lines

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;

Quoted 7 lines

Quoted 3 lines

	/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