[libvirt] [PATCH] util: Make virStringArrayHasString() const-correct
The first argument should be const char ** instead of char **, because this is a search function and as such it doesn't, and shouldn't, alter the haystack in any way. This change means we no longer have to cast arrays of immutable strings to arrays of mutable strings; we still have to do the opposite, though, but that's reasonable. --- src/lxc/lxc_native.c | 5 +++-- src/qemu/qemu_capabilities.c | 4 ++-- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 4 +++- src/util/virstring.c | 3 ++- src/util/virstring.h | 2 +- tests/qemumonitorjsontest.c | 10 +++++----- 7 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index 4b22e2a..94fb659 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -299,7 +299,7 @@ lxcAddFstabLine(virDomainDefPtr def, lxcFstabPtr fstab) type = VIR_DOMAIN_FS_TYPE_BLOCK; /* Do we have ro in options? */ - readonly = virStringArrayHasString(options, "ro"); + readonly = virStringArrayHasString((const char **) options, "ro"); if (lxcAddFSDef(def, type, src, dst, readonly, usage) < 0) goto cleanup; @@ -981,7 +981,8 @@ lxcSetCapDrop(virDomainDefPtr def, virConfPtr properties) for (i = 0; i < VIR_DOMAIN_CAPS_FEATURE_LAST; i++) { capString = virDomainCapsFeatureTypeToString(i); - if (toDrop != NULL && virStringArrayHasString(toDrop, capString)) + if (toDrop != NULL && + virStringArrayHasString((const char **) toDrop, capString)) def->caps_features[i] = VIR_TRISTATE_SWITCH_OFF; } diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 43e3ea7..1483217 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2580,7 +2580,7 @@ virQEMUCapsProbeQMPTPM(virQEMUCapsPtr qemuCaps, for (i = 0; i < ARRAY_CARDINALITY(virQEMUCapsTPMModelsToCaps); i++) { const char *needle = virDomainTPMModelTypeToString( virQEMUCapsTPMModelsToCaps[i].type); - if (virStringArrayHasString(entries, needle)) + if (virStringArrayHasString((const char **) entries, needle)) virQEMUCapsSet(qemuCaps, virQEMUCapsTPMModelsToCaps[i].caps); } @@ -2594,7 +2594,7 @@ virQEMUCapsProbeQMPTPM(virQEMUCapsPtr qemuCaps, for (i = 0; i < ARRAY_CARDINALITY(virQEMUCapsTPMTypesToCaps); i++) { const char *needle = virDomainTPMBackendTypeToString( virQEMUCapsTPMTypesToCaps[i].type); - if (virStringArrayHasString(entries, needle)) + if (virStringArrayHasString((const char **) entries, needle)) virQEMUCapsSet(qemuCaps, virQEMUCapsTPMTypesToCaps[i].caps); } } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d455adf..1df1e4a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5728,7 +5728,7 @@ qemuMonitorJSONGetMigrationCapability(qemuMonitorPtr mon, if (qemuMonitorJSONGetMigrationCapabilities(mon, &capsList) < 0) return -1; - ret = virStringArrayHasString(capsList, cap); + ret = virStringArrayHasString((const char **) capsList, cap); virStringFreeList(capsList); return ret; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7481626..3ade190 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3151,6 +3151,7 @@ qemuProcessUpdateDevices(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDeviceDef dev; + const char **qemuDevices; char **old; char **tmp; int ret = -1; @@ -3163,9 +3164,10 @@ qemuProcessUpdateDevices(virQEMUDriverPtr driver, if (qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) goto cleanup; + qemuDevices = (const char **) priv->qemuDevices; if ((tmp = old)) { while (*tmp) { - if (!virStringArrayHasString(priv->qemuDevices, *tmp) && + if (!virStringArrayHasString(qemuDevices, *tmp) && virDomainDefFindDevice(vm->def, *tmp, &dev, false) == 0 && qemuDomainRemoveDevice(driver, vm, &dev) < 0) { goto cleanup; diff --git a/src/util/virstring.c b/src/util/virstring.c index 0177a95..4a70620 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -210,7 +210,8 @@ virStringFreeListCount(char **strings, bool -virStringArrayHasString(char **strings, const char *needle) +virStringArrayHasString(const char **strings, + const char *needle) { size_t i = 0; diff --git a/src/util/virstring.h b/src/util/virstring.h index 040771e..8854d5f 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -44,7 +44,7 @@ char *virStringJoin(const char **strings, void virStringFreeList(char **strings); void virStringFreeListCount(char **strings, size_t count); -bool virStringArrayHasString(char **strings, const char *needle); +bool virStringArrayHasString(const char **strings, const char *needle); char *virStringGetFirstWithPrefix(char **strings, const char *prefix) ATTRIBUTE_NONNULL(2); diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 544b569..9988754 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -999,7 +999,7 @@ testQemuMonitorJSONGetDeviceAliases(const void *data) qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); int ret = -1; char **aliases = NULL; - char **alias; + const char **alias; const char *expected[] = { "virtio-disk25", "video0", "serial0", "ide0-0-0", "usb", NULL }; @@ -1033,14 +1033,14 @@ testQemuMonitorJSONGetDeviceAliases(const void *data) } ret = 0; - for (alias = aliases; *alias; alias++) { - if (!virStringArrayHasString((char **) expected, *alias)) { + for (alias = (const char **) aliases; *alias; alias++) { + if (!virStringArrayHasString(expected, *alias)) { fprintf(stderr, "got unexpected device alias '%s'\n", *alias); ret = -1; } } - for (alias = (char **) expected; *alias; alias++) { - if (!virStringArrayHasString(aliases, *alias)) { + for (alias = expected; *alias; alias++) { + if (!virStringArrayHasString((const char **) aliases, *alias)) { fprintf(stderr, "missing expected alias '%s'\n", *alias); ret = -1; } -- 2.7.4
On 16.08.2016 13:40, Andrea Bolognani wrote:
The first argument should be const char ** instead of char **, because this is a search function and as such it doesn't, and shouldn't, alter the haystack in any way.
This change means we no longer have to cast arrays of immutable strings to arrays of mutable strings; we still have to do the opposite, though, but that's reasonable.
Is it? I mean, we are restricting ourselves and compiler fails to see that. To me 'const char **' is more restrictive than 'char **' therefore there should be no typecast required. But this is the discussion I should have with gcc devels. For some reason, gcc does automatic typecasting to const just for the fist level pointers and not the second one. That's why compilers errors out.
--- src/lxc/lxc_native.c | 5 +++-- src/qemu/qemu_capabilities.c | 4 ++-- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 4 +++- src/util/virstring.c | 3 ++- src/util/virstring.h | 2 +- tests/qemumonitorjsontest.c | 10 +++++----- 7 files changed, 17 insertions(+), 13 deletions(-)
ACK Michal
On Tue, 2016-08-16 at 18:49 +0200, Michal Privoznik wrote:
The first argument should be const char ** instead of char **, because this is a search function and as such it doesn't, and shouldn't, alter the haystack in any way. This change means we no longer have to cast arrays of immutable strings to arrays of mutable strings; we still have to do the opposite, though, but that's reasonable. Is it? I mean, we are restricting ourselves and compiler fails to see
On 16.08.2016 13:40, Andrea Bolognani wrote: that. To me 'const char **' is more restrictive than 'char **' therefore there should be no typecast required. But this is the discussion I should have with gcc devels. For some reason, gcc does automatic typecasting to const just for the fist level pointers and not the second one. That's why compilers errors out.
The reason for this behavior is explained in the C FAQ: http://c-faq.com/ansi/constmismatch.html It's unfortunate, and very annoying. But I'd rather have to perform arguably redundant casts than being bitten by that kind of bug down the line :)
ACK
Pushed, thanks! -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Aug 16, 2016 at 07:55:11PM +0200, Andrea Bolognani wrote:
On Tue, 2016-08-16 at 18:49 +0200, Michal Privoznik wrote:
The first argument should be const char ** instead of char **, because this is a search function and as such it doesn't, and shouldn't, alter the haystack in any way. This change means we no longer have to cast arrays of immutable strings to arrays of mutable strings; we still have to do the opposite, though, but that's reasonable. Is it? I mean, we are restricting ourselves and compiler fails to see
On 16.08.2016 13:40, Andrea Bolognani wrote: that. To me 'const char **' is more restrictive than 'char **' therefore there should be no typecast required. But this is the discussion I should have with gcc devels. For some reason, gcc does automatic typecasting to const just for the fist level pointers and not the second one. That's why compilers errors out.
The reason for this behavior is explained in the C FAQ:
Just FYI, so that you know why adding more consts (even to sensible places) doesn't help in C, I found the answer to my question on stack overflow [1] very satisfactory and explanatory. Martin [1] https://stackoverflow.com/questions/35319842/why-c-doesnt-allow-implicit-con...
It's unfortunate, and very annoying. But I'd rather have to perform arguably redundant casts than being bitten by that kind of bug down the line :)
ACK
Pushed, thanks!
-- Andrea Bolognani / Red Hat / Virtualization
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2016-08-16 at 22:31 +0200, Martin Kletzander wrote:
This change means we no longer have to cast arrays of immutable strings to arrays of mutable strings; we still have to do the opposite, though, but that's reasonable. Is it? I mean, we are restricting ourselves and compiler fails to see that. To me 'const char **' is more restrictive than 'char **' therefore there should be no typecast required. But this is the discussion I should have with gcc devels. For some reason, gcc does automatic typecasting to const just for the fist level pointers and not the second one. That's why compilers errors out. The reason for this behavior is explained in the C FAQ: http://c-faq.com/ansi/constmismatch.html Just FYI, so that you know why adding more consts (even to sensible places) doesn't help in C, I found the answer to my question on stack overflow [1] very satisfactory and explanatory.
So the solution is simple: rewrite all of libvirt in C++! ;) -- Andrea Bolognani / Red Hat / Virtualization
On 17/08/16 09:56, Andrea Bolognani wrote:
On Tue, 2016-08-16 at 22:31 +0200, Martin Kletzander wrote:
This change means we no longer have to cast arrays of immutable strings to arrays of mutable strings; we still have to do the opposite, though, but that's reasonable.
Is it? I mean, we are restricting ourselves and compiler fails to see that. To me 'const char **' is more restrictive than 'char **' therefore there should be no typecast required. But this is the discussion I should have with gcc devels. For some reason, gcc does automatic typecasting to const just for the fist level pointers and not the second one. That's why compilers errors out.
The reason for this behavior is explained in the C FAQ:
Just FYI, so that you know why adding more consts (even to sensible places) doesn't help in C, I found the answer to my question on stack overflow [1] very satisfactory and explanatory.
So the solution is simple: rewrite all of libvirt in C++! ;)
-- Andrea Bolognani / Red Hat / Virtualization
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Patches are welcome ;)...oh, wait... Or we could make a "libvirt rationale" and state that some char ** arrays are meant to be constant in a textual manner :). Erik PS: hopefully nobody took that one seriously.
On Wed, Aug 17, 2016 at 10:22:46AM +0200, Erik Skultety wrote:
On 17/08/16 09:56, Andrea Bolognani wrote:
On Tue, 2016-08-16 at 22:31 +0200, Martin Kletzander wrote:
This change means we no longer have to cast arrays of immutable strings to arrays of mutable strings; we still have to do the opposite, though, but that's reasonable.
Is it? I mean, we are restricting ourselves and compiler fails to see that. To me 'const char **' is more restrictive than 'char **' therefore there should be no typecast required. But this is the discussion I should have with gcc devels. For some reason, gcc does automatic typecasting to const just for the fist level pointers and not the second one. That's why compilers errors out.
The reason for this behavior is explained in the C FAQ:
Just FYI, so that you know why adding more consts (even to sensible places) doesn't help in C, I found the answer to my question on stack overflow [1] very satisfactory and explanatory.
So the solution is simple: rewrite all of libvirt in C++! ;)
Because one thing makes more sense? How about ocaml then?
Patches are welcome ;)...oh, wait... Or we could make a "libvirt rationale" and state that some char ** arrays are meant to be constant in a textual manner :).
Erik
PS: hopefully nobody took that one seriously.
Which one, though :D
On 16.08.2016 19:55, Andrea Bolognani wrote:
On Tue, 2016-08-16 at 18:49 +0200, Michal Privoznik wrote:
On 16.08.2016 13:40, Andrea Bolognani wrote:
The first argument should be const char ** instead of char **, because this is a search function and as such it doesn't, and shouldn't, alter the haystack in any way.
This change means we no longer have to cast arrays of immutable strings to arrays of mutable strings; we still have to do the opposite, though, but that's reasonable.
Is it? I mean, we are restricting ourselves and compiler fails to see that. To me 'const char **' is more restrictive than 'char **' therefore there should be no typecast required. But this is the discussion I should have with gcc devels. For some reason, gcc does automatic typecasting to const just for the fist level pointers and not the second one. That's why compilers errors out.
The reason for this behavior is explained in the C FAQ:
This is very controversial to me. They argue that compiler cannot hold up to your promise with the following code: const char c = 'x'; /* 1 */ char *p1; /* 2 */ const char **p2 = &p1; /* 3 */ *p2 = &c; /* 4 */ *p1 = 'X'; /* 5 */ What they are arguing there is that step 3 shouldn't be allowed, because the compiler cannot hold up to our promise about constness of p2. Well I disagree. p2 is const char ** which means that only the first level pointer is const, the second level is pure char *. Therefore in my eyes it's step 4 where the problem is and where compiler should warn me. I mean, after I've dereferenced p2, I'm left with 'char *' which I'm trying to assign address of a const char. IOW: char * var1 = (const char *) var2; Obviously, this is not allowed. And compiler has to know what 'const char **' means, just like what 'char *' means. So I don't really understand their reasoning there. Michal
participants (4)
-
Andrea Bolognani -
Erik Skultety -
Martin Kletzander -
Michal Privoznik