[libvirt] [PATCH] virStringListLength: Ensure const correctness
The virStringListLength function does not ever modify the passed string list. It merely counts the items in it. Make sure that we reflect this bit in the function header. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- This is hugely driven by a compilation error observed after the latest Martin revert (ea913d185df9). I'm seeing this compilation error: util/virpolkit.c: In function 'virPolkitCheckAuth': util/virpolkit.c:93:47: error: passing argument 1 of 'virStringListLength' from incompatible pointer type [-Werror] virStringListLength(details) / 2, ^ In file included from util/virpolkit.c:33:0: util/virstring.h:204:8: note: expected 'char **' but argument is of type 'const char **' size_t virStringListLength(char **strings); But for some reason, implicit typecast from char ** to const char ** nor const char * const * is allowed by gcc. I don't really understand why, so if anybody has some explanation, please do explain. src/lxc/lxc_native.c | 2 +- src/storage/storage_driver.c | 2 +- src/util/virprocess.c | 2 +- src/util/virstoragefile.c | 2 +- src/util/virstring.c | 2 +- src/util/virstring.h | 2 +- tests/virstringtest.c | 4 ++-- 7 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index 78f9c67..ef92c7d 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -611,7 +611,7 @@ lxcNetworkWalkCallback(const char *name, virConfValuePtr value, void *data) family = AF_INET6; ipparts = virStringSplit(value->str, "/", 2); - if (virStringListLength(ipparts) != 2 || + if (virStringListLength((const char * const *)ipparts) != 2 || virSocketAddrParse(&ip->address, ipparts[0], family) < 0 || virStrToLong_ui(ipparts[1], NULL, 10, &ip->prefix) < 0) { diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ed5395b..8ee2840 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3209,7 +3209,7 @@ virStorageAddISCSIPoolSourceHost(virDomainDiskDefPtr def, if (!(tokens = virStringSplit(def->src->srcpool->volume, ":", 0))) goto cleanup; - if (virStringListLength(tokens) != 4) { + if (virStringListLength((const char * const *)tokens) != 4) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected iscsi volume name '%s'"), def->src->srcpool->volume); diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 277b3bc..c7ffa42 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -965,7 +965,7 @@ int virProcessGetStartTime(pid_t pid, tokens = virStringSplit(tmp, " ", 0); - if (virStringListLength(tokens) < 20) { + if (virStringListLength((const char * const *)tokens) < 20) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot find start time in %s"), filename); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 101070f..5a4e101 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1283,7 +1283,7 @@ virStorageFileParseChainIndex(const char *diskTarget, if (name && diskTarget) strings = virStringSplit(name, "[", 2); - if (virStringListLength(strings) != 2) + if (virStringListLength((const char * const *) strings) != 2) goto cleanup; if (virStrToLong_uip(strings[1], &suffix, 10, &idx) < 0 || diff --git a/src/util/virstring.c b/src/util/virstring.c index fc4f5ba..7ec42aa 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -765,7 +765,7 @@ virStrndup(char **dest, } -size_t virStringListLength(char **strings) +size_t virStringListLength(const char * const *strings) { size_t i = 0; diff --git a/src/util/virstring.h b/src/util/virstring.h index cdf1058..16ed3b2 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -201,7 +201,7 @@ int virVasprintfInternal(bool report, int domcode, const char *filename, # define VIR_STRNDUP_QUIET(dst, src, n) virStrndup(&(dst), src, n, false, \ 0, NULL, NULL, 0) -size_t virStringListLength(char **strings); +size_t virStringListLength(const char * const *strings); /** * virVasprintf diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 38d0126..7a1dcfd 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -336,10 +336,10 @@ testStringSearch(const void *opaque) goto cleanup; } - if (virStringListLength(matches) != nmatches) { + if (virStringListLength((const char * const *)matches) != nmatches) { fprintf(stderr, "expected %zu matches on %s but got %zd matches\n", data->expectNMatches, data->str, - virStringListLength(matches)); + virStringListLength((const char * const *)matches)); goto cleanup; } -- 2.4.10
On Tue, 2016-02-09 at 18:18 +0100, Michal Privoznik wrote:
The virStringListLength function does not ever modify the passed string list. It merely counts the items in it. Make sure that we reflect this bit in the function header. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- This is hugely driven by a compilation error observed after the latest Martin revert (ea913d185df9). I'm seeing this compilation error: util/virpolkit.c: In function 'virPolkitCheckAuth': util/virpolkit.c:93:47: error: passing argument 1 of 'virStringListLength' from incompatible pointer type [-Werror] virStringListLength(details) / 2, ^ In file included from util/virpolkit.c:33:0: util/virstring.h:204:8: note: expected 'char **' but argument is of type 'const char **' size_t virStringListLength(char **strings); But for some reason, implicit typecast from char ** to const char ** nor const char * const * is allowed by gcc. I don't really understand why, so if anybody has some explanation, please do explain.
Nitpick: you're using both (const char * const *) var and (const char * const *)var in your patch: both are used across libvirt's codebase, so please pick one and stick to that. I've looked for answers on the Internet, as you do, and I've stumbled upon this entry from the C FAQ: http://c-faq.com/ansi/constmismatch.html Now, having to explicitly cast when calling the function definitely sucks, but I don't really see a way around it at the moment; plus doing the opposite and casting a const pointer to a non-const pointer looks like a step in the wrong direction. That said, I'd like to hear more opinions so weak ACK with the whitespace inconsistency fixed and the following squashed in. Cheers. diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index 1200813..264ccc8 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -167,7 +167,8 @@ virStorageBackendSheepdogRefreshAllVol(virConnectPtr conn ATTRIBUTE_UNUSED, cells = virStringSplit(line, " ", 0); - if (cells != NULL && virStringListLength(cells) > 2) { + if (cells != NULL && + virStringListLength((const char * const *) cells) > 2) { if (virStorageBackendSheepdogAddVolume(conn, pool, cells[1]) < 0) goto cleanup; } -- Andrea Bolognani Software Engineer - Virtualization Team
On 02/09/2016 01:19 PM, Andrea Bolognani wrote:
On Tue, 2016-02-09 at 18:18 +0100, Michal Privoznik wrote:
The virStringListLength function does not ever modify the passed string list. It merely counts the items in it. Make sure that we reflect this bit in the function header.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
This is hugely driven by a compilation error observed after the latest Martin revert (ea913d185df9). I'm seeing this compilation error:
util/virpolkit.c: In function 'virPolkitCheckAuth': util/virpolkit.c:93:47: error: passing argument 1 of 'virStringListLength' from incompatible pointer type [-Werror] virStringListLength(details) / 2, ^ In file included from util/virpolkit.c:33:0: util/virstring.h:204:8: note: expected 'char **' but argument is of type 'const char **' size_t virStringListLength(char **strings);
But for some reason, implicit typecast from char ** to const char ** nor const char * const * is allowed by gcc. I don't really understand why, so if anybody has some explanation, please do explain.
Nitpick: you're using both
(const char * const *) var
and
(const char * const *)var
in your patch: both are used across libvirt's codebase, so please pick one and stick to that.
I've looked for answers on the Internet, as you do, and I've stumbled upon this entry from the C FAQ:
http://c-faq.com/ansi/constmismatch.html
Now, having to explicitly cast when calling the function definitely sucks, but I don't really see a way around it at the moment; plus doing the opposite and casting a const pointer to a non-const pointer looks like a step in the wrong direction.
That said, I'd like to hear more opinions so weak ACK with the whitespace inconsistency fixed and the following squashed in.
Cheers.
diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index 1200813..264ccc8 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -167,7 +167,8 @@ virStorageBackendSheepdogRefreshAllVol(virConnectPtr conn ATTRIBUTE_UNUSED,
cells = virStringSplit(line, " ", 0);
- if (cells != NULL && virStringListLength(cells) > 2) { + if (cells != NULL && + virStringListLength((const char * const *) cells) > 2) { if (virStorageBackendSheepdogAddVolume(conn, pool, cells[1]) < 0) goto cleanup; }
Since this is breaking the default build for me, I squashed in that hunk and settled on '(cast)variable' spacing and pushed this patch. Thanks, Cole
participants (3)
-
Andrea Bolognani -
Cole Robinson -
Michal Privoznik