[PATCH] virCgroupSetValueRaw: Use 'const' temporary variable
From: Peter Krempa <pkrempa@redhat.com> 'char *tmp' is assigned from calling 'strrchr' on a 'const char *'. New clang in fedora doesn't like it. Make 'tmp' const. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/12208300313 src/util/vircgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 532a7e5690..3d66d3acb2 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -517,7 +517,7 @@ int virCgroupSetValueRaw(const char *path, const char *value) { - char *tmp; + const char *tmp; VIR_DEBUG("Set path '%s' to value '%s'", path, value); if (virFileWriteStr(path, value, 0) < 0) { -- 2.52.0
On a Tuesday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
'char *tmp' is assigned from calling 'strrchr' on a 'const char *'. New clang in fedora doesn't like it. Make 'tmp' const.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/12208300313
I was hoping the link would show a fixed pipeline :)
src/util/vircgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 532a7e5690..3d66d3acb2 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -517,7 +517,7 @@ int virCgroupSetValueRaw(const char *path, const char *value) { - char *tmp; + const char *tmp;
VIR_DEBUG("Set path '%s' to value '%s'", path, value); if (virFileWriteStr(path, value, 0) < 0) {
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Tue, Nov 25, 2025 at 02:54:20PM +0100, Ján Tomko via Devel wrote:
On a Tuesday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
'char *tmp' is assigned from calling 'strrchr' on a 'const char *'. New clang in fedora doesn't like it. Make 'tmp' const.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/12208300313
I was hoping the link would show a fixed pipeline :)
I'm rather curious how clang decides to trigger that warning given the libc header file declares the return value non-const extern char *strchr (const char *__s, int __c) __THROW __attribute_pure__ __nonnull ((1)); It seems like clang has special-cased strchr/strrchr to enforce the const return for const input.
src/util/vircgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 532a7e5690..3d66d3acb2 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -517,7 +517,7 @@ int virCgroupSetValueRaw(const char *path, const char *value) { - char *tmp; + const char *tmp;
VIR_DEBUG("Set path '%s' to value '%s'", path, value); if (virFileWriteStr(path, value, 0) < 0) {
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 11/25/25 15:10, Daniel P. Berrangé via Devel wrote:
On Tue, Nov 25, 2025 at 02:54:20PM +0100, Ján Tomko via Devel wrote:
On a Tuesday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
'char *tmp' is assigned from calling 'strrchr' on a 'const char *'. New clang in fedora doesn't like it. Make 'tmp' const.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/12208300313
I was hoping the link would show a fixed pipeline :)
I'm rather curious how clang decides to trigger that warning given the libc header file declares the return value non-const
extern char *strchr (const char *__s, int __c) __THROW __attribute_pure__ __nonnull ((1));
It seems like clang has special-cased strchr/strrchr to enforce the const return for const input.
Well, it also triggers in places like: ../src/rpc/virnetsshsession.c:223:18: error: assigning to 'char *' from 'const char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] 223 | if ((tmp = strrchr(askcred[i].prompt, ':'))) | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ And just to give you context around the line: if ((tmp = strrchr(askcred[i].prompt, ':'))) *tmp = '\0'; So I'd rather this patch is NOT merged and CLang is fixed instead. Michal
On Tue, Nov 25, 2025 at 03:51:00PM +0100, Michal Prívozník wrote:
On 11/25/25 15:10, Daniel P. Berrangé via Devel wrote:
On Tue, Nov 25, 2025 at 02:54:20PM +0100, Ján Tomko via Devel wrote:
On a Tuesday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
'char *tmp' is assigned from calling 'strrchr' on a 'const char *'. New clang in fedora doesn't like it. Make 'tmp' const.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/12208300313
I was hoping the link would show a fixed pipeline :)
I'm rather curious how clang decides to trigger that warning given the libc header file declares the return value non-const
extern char *strchr (const char *__s, int __c) __THROW __attribute_pure__ __nonnull ((1));
It seems like clang has special-cased strchr/strrchr to enforce the const return for const input.
Well, it also triggers in places like:
../src/rpc/virnetsshsession.c:223:18: error: assigning to 'char *' from 'const char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] 223 | if ((tmp = strrchr(askcred[i].prompt, ':'))) | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
And just to give you context around the line:
if ((tmp = strrchr(askcred[i].prompt, ':'))) *tmp = '\0';
So I'd rather this patch is NOT merged and CLang is fixed instead.
I'm on the fence about whether clang is "broken" or not. It is in response to -Wincompatible-pointer-types-discards-qualifiers. Since warnings aren't standardized, the compiler author has free choice over semantics. In C++ the string.h header actually provides 2 overloaded definitions /* Find the first occurrence of C in S. */ #ifdef __CORRECT_ISO_CPP_STRING_H_PROTO extern "C++" { extern char *strchr (char *__s, int __c) __THROW __asm ("strchr") __attribute_pure__ __nonnull ((1)); extern const char *strchr (const char *__s, int __c) __THROW __asm ("strchr") __attribute_pure__ __nonnull ((1)); ...snip... IMHO it is not unreasonable for CLang to want to bring an equivalent level of const-correctness to C through warning flags. I figure we're probably got a choice of fixing all the non-const correct usage across the codebase, or turning off this warning with clang builds. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 11/25/25 16:03, Daniel P. Berrangé wrote:
On Tue, Nov 25, 2025 at 03:51:00PM +0100, Michal Prívozník wrote:
On 11/25/25 15:10, Daniel P. Berrangé via Devel wrote:
On Tue, Nov 25, 2025 at 02:54:20PM +0100, Ján Tomko via Devel wrote:
On a Tuesday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
'char *tmp' is assigned from calling 'strrchr' on a 'const char *'. New clang in fedora doesn't like it. Make 'tmp' const.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/12208300313
I was hoping the link would show a fixed pipeline :)
I'm rather curious how clang decides to trigger that warning given the libc header file declares the return value non-const
extern char *strchr (const char *__s, int __c) __THROW __attribute_pure__ __nonnull ((1));
It seems like clang has special-cased strchr/strrchr to enforce the const return for const input.
Well, it also triggers in places like:
../src/rpc/virnetsshsession.c:223:18: error: assigning to 'char *' from 'const char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] 223 | if ((tmp = strrchr(askcred[i].prompt, ':'))) | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
And just to give you context around the line:
if ((tmp = strrchr(askcred[i].prompt, ':'))) *tmp = '\0';
So I'd rather this patch is NOT merged and CLang is fixed instead.
I'm on the fence about whether clang is "broken" or not.
It is in response to -Wincompatible-pointer-types-discards-qualifiers. Since warnings aren't standardized, the compiler author has free choice over semantics.
In C++ the string.h header actually provides 2 overloaded definitions
/* Find the first occurrence of C in S. */ #ifdef __CORRECT_ISO_CPP_STRING_H_PROTO extern "C++" { extern char *strchr (char *__s, int __c) __THROW __asm ("strchr") __attribute_pure__ __nonnull ((1)); extern const char *strchr (const char *__s, int __c) __THROW __asm ("strchr") __attribute_pure__ __nonnull ((1)); ...snip...
IMHO it is not unreasonable for CLang to want to bring an equivalent level of const-correctness to C through warning flags.
I figure we're probably got a choice of fixing all the non-const correct usage across the codebase, or turning off this warning with clang builds.
Alright, after some investigation, I think I know what's happening. In fact, it's glibc that changed [1]. The strrchr() function is now declared in string.h as the following: extern char *strrchr (const char *__s, int __c) __THROW __attribute_pure__ __nonnull ((1)); # if __GLIBC_USE (ISOC23) && defined __glibc_const_generic && !defined _LIBC # define strrchr(S, C) \ __glibc_const_generic (S, const char *, strrchr (S, C)) # endif where __glibc_const_generic macro is then declared as: # define __glibc_const_generic(PTR, CTYPE, CALL) \ _Generic (0 ? (PTR) : (void *) 1, \ const void *: (CTYPE) (CALL), \ default: CALL) Long story short, the call to strrchr() from virCgroupSetValueRaw() expands to: tmp = _Generic (0 ? (path) : (void *) 1, const void *: (const char *) (strrchr (path, '/')), default: strrchr (path, '/')) Which indeed means that whenever strrchr() is given a (const void *) arg its retval is typecasted to (const char *). And it's the same story with strchr() and others. So I agree, this is something we ought to fix. What I don't quite understand is why we're not hitting the same warning with GCC since its preprocessor expands the call to the very same line. Michal 1: https://sourceware.org/git/?p=glibc.git;a=commit;h=cd748a63ab1a7ae846175c532...
On Wed, Nov 26, 2025 at 09:15:58AM +0100, Michal Prívozník wrote:
On 11/25/25 16:03, Daniel P. Berrangé wrote:
On Tue, Nov 25, 2025 at 03:51:00PM +0100, Michal Prívozník wrote:
On 11/25/25 15:10, Daniel P. Berrangé via Devel wrote:
On Tue, Nov 25, 2025 at 02:54:20PM +0100, Ján Tomko via Devel wrote:
On a Tuesday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
'char *tmp' is assigned from calling 'strrchr' on a 'const char *'. New clang in fedora doesn't like it. Make 'tmp' const.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/12208300313
I was hoping the link would show a fixed pipeline :)
I'm rather curious how clang decides to trigger that warning given the libc header file declares the return value non-const
extern char *strchr (const char *__s, int __c) __THROW __attribute_pure__ __nonnull ((1));
It seems like clang has special-cased strchr/strrchr to enforce the const return for const input.
Well, it also triggers in places like:
../src/rpc/virnetsshsession.c:223:18: error: assigning to 'char *' from 'const char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] 223 | if ((tmp = strrchr(askcred[i].prompt, ':'))) | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
And just to give you context around the line:
if ((tmp = strrchr(askcred[i].prompt, ':'))) *tmp = '\0';
So I'd rather this patch is NOT merged and CLang is fixed instead.
I'm on the fence about whether clang is "broken" or not.
It is in response to -Wincompatible-pointer-types-discards-qualifiers. Since warnings aren't standardized, the compiler author has free choice over semantics.
In C++ the string.h header actually provides 2 overloaded definitions
/* Find the first occurrence of C in S. */ #ifdef __CORRECT_ISO_CPP_STRING_H_PROTO extern "C++" { extern char *strchr (char *__s, int __c) __THROW __asm ("strchr") __attribute_pure__ __nonnull ((1)); extern const char *strchr (const char *__s, int __c) __THROW __asm ("strchr") __attribute_pure__ __nonnull ((1)); ...snip...
IMHO it is not unreasonable for CLang to want to bring an equivalent level of const-correctness to C through warning flags.
I figure we're probably got a choice of fixing all the non-const correct usage across the codebase, or turning off this warning with clang builds.
Alright, after some investigation, I think I know what's happening. In fact, it's glibc that changed [1]. The strrchr() function is now declared in string.h as the following:
extern char *strrchr (const char *__s, int __c) __THROW __attribute_pure__ __nonnull ((1)); # if __GLIBC_USE (ISOC23) && defined __glibc_const_generic && !defined _LIBC # define strrchr(S, C) \ __glibc_const_generic (S, const char *, strrchr (S, C)) # endif
where __glibc_const_generic macro is then declared as:
# define __glibc_const_generic(PTR, CTYPE, CALL) \ _Generic (0 ? (PTR) : (void *) 1, \ const void *: (CTYPE) (CALL), \ default: CALL)
Long story short, the call to strrchr() from virCgroupSetValueRaw() expands to:
tmp = _Generic (0 ? (path) : (void *) 1, const void *: (const char *) (strrchr (path, '/')), default: strrchr (path, '/'))
Which indeed means that whenever strrchr() is given a (const void *) arg its retval is typecasted to (const char *). And it's the same story with strchr() and others. So I agree, this is something we ought to fix. What I don't quite understand is why we're not hitting the same warning with GCC since its preprocessor expands the call to the very same line.
I'm actually seeing the opposite with a simple test program GCC will warn without needing any args: $ cat const.c #include <string.h> #include <stdbool.h> bool foo(const char *wizz); bool foo(const char *wizz) { char *bang = strchr(wizz, '/'); return bang != NULL; } $ gcc -c -o const const.c const.c: In function 'foo': const.c:8:16: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] 8 | char *bang = strchr(wizz, '/'); | ^~~~~~ clang meanwhile is completely quiet $ clang -c -o const const.c $ clang -Wincompatible-pointer-types-discards-qualifiers -Wall -c -o const const.c $ clang -Wincompatible-pointer-types-discards-qualifiers -Wall -std=gnu99 -c -o const const.c $ clang -Wincompatible-pointer-types-discards-qualifiers -Wall -std=gnu23 -c -o const const.c const.c:8:9: warning: initializing 'char *' with an expression of type 'const char *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers] 8 | char *bang = strchr(wizz, '/'); | ^ ~~~~~~~~~~~~~~~~~ 1 warning generated. which makes no sense, as libvirt builds with -std=gnu99 # rpm -q gcc glibc-devel clang gcc-15.2.1-4.fc44.x86_64 glibc-devel-2.42.9000-13.fc44.x86_64 clang-21.1.6-1.fc44.x86_64 With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On a Tuesday in 2025, Michal Prívozník wrote:
On 11/25/25 15:10, Daniel P. Berrangé via Devel wrote:
On Tue, Nov 25, 2025 at 02:54:20PM +0100, Ján Tomko via Devel wrote:
On a Tuesday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
'char *tmp' is assigned from calling 'strrchr' on a 'const char *'. New clang in fedora doesn't like it. Make 'tmp' const.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/12208300313
I was hoping the link would show a fixed pipeline :)
I'm rather curious how clang decides to trigger that warning given the libc header file declares the return value non-const
extern char *strchr (const char *__s, int __c) __THROW __attribute_pure__ __nonnull ((1));
It seems like clang has special-cased strchr/strrchr to enforce the const return for const input.
Well, it also triggers in places like:
../src/rpc/virnetsshsession.c:223:18: error: assigning to 'char *' from 'const char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] 223 | if ((tmp = strrchr(askcred[i].prompt, ':'))) | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
And just to give you context around the line:
if ((tmp = strrchr(askcred[i].prompt, ':'))) *tmp = '\0';
Well, the prompt is defined as const char*: struct _virConnectCredential { int type; /* One of virConnectCredentialType constants */ const char *prompt; /* Prompt to show to user */ const char *challenge; /* Additional challenge to show */ const char *defresult; /* Optional default result */ char *result; /* Result to be filled with user response (or defresult) */ unsigned int resultlen; /* Length of the result */ }; Jano
So I'd rather this patch is NOT merged and CLang is fixed instead.
Michal
On Tue, Nov 25, 2025 at 04:05:06PM +0100, Ján Tomko wrote:
On a Tuesday in 2025, Michal Prívozník wrote:
On 11/25/25 15:10, Daniel P. Berrangé via Devel wrote:
On Tue, Nov 25, 2025 at 02:54:20PM +0100, Ján Tomko via Devel wrote:
On a Tuesday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
'char *tmp' is assigned from calling 'strrchr' on a 'const char *'. New clang in fedora doesn't like it. Make 'tmp' const.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/12208300313
I was hoping the link would show a fixed pipeline :)
I'm rather curious how clang decides to trigger that warning given the libc header file declares the return value non-const
extern char *strchr (const char *__s, int __c) __THROW __attribute_pure__ __nonnull ((1));
It seems like clang has special-cased strchr/strrchr to enforce the const return for const input.
Well, it also triggers in places like:
../src/rpc/virnetsshsession.c:223:18: error: assigning to 'char *' from 'const char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] 223 | if ((tmp = strrchr(askcred[i].prompt, ':'))) | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
And just to give you context around the line:
if ((tmp = strrchr(askcred[i].prompt, ':'))) *tmp = '\0';
Well, the prompt is defined as const char*:
We just created askcred[i].prompt from g_strdup a line earlier so it is safe. None the less this is bad practice - we should use an intermediate variable to strip the ':', and only assign to the struct once done.
struct _virConnectCredential { int type; /* One of virConnectCredentialType constants */ const char *prompt; /* Prompt to show to user */ const char *challenge; /* Additional challenge to show */ const char *defresult; /* Optional default result */ char *result; /* Result to be filled with user response (or defresult) */ unsigned int resultlen; /* Length of the result */ };
Jano
So I'd rather this patch is NOT merged and CLang is fixed instead.
Michal
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 11/25/25 14:33, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
'char *tmp' is assigned from calling 'strrchr' on a 'const char *'. New clang in fedora doesn't like it. Make 'tmp' const.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/12208300313
src/util/vircgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 532a7e5690..3d66d3acb2 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -517,7 +517,7 @@ int virCgroupSetValueRaw(const char *path, const char *value) { - char *tmp; + const char *tmp;
VIR_DEBUG("Set path '%s' to value '%s'", path, value); if (virFileWriteStr(path, value, 0) < 0) {
NO NO NO. There are way too many places that need fixing and this will not help anything. I have a rawhide VM where I ran the build and this "fixes" just one place of many. In fact, I think it's CLang that's broken. Michal
On Tue, Nov 25, 2025 at 15:45:33 +0100, Michal Prívozník wrote:
On 11/25/25 14:33, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
'char *tmp' is assigned from calling 'strrchr' on a 'const char *'. New clang in fedora doesn't like it. Make 'tmp' const.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/12208300313
src/util/vircgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 532a7e5690..3d66d3acb2 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -517,7 +517,7 @@ int virCgroupSetValueRaw(const char *path, const char *value) { - char *tmp; + const char *tmp;
VIR_DEBUG("Set path '%s' to value '%s'", path, value); if (virFileWriteStr(path, value, 0) < 0) {
NO NO NO.
There are way too many places that need fixing and this will not help anything. I have a rawhide VM where I ran the build and this "fixes" just one place of many. In fact, I think it's CLang that's broken.
Ooh! this is actually 'rawhide'. Thinking this is Fedora 43, I did the above change and it built fine on my box, but I didn't observe the failure locally before as I've just updated. Anyways, I do agree that if this is widespread CLang needs to be fixed or we need a more complete solution.
participants (4)
-
Daniel P. Berrangé -
Ján Tomko -
Michal Prívozník -
Peter Krempa