[libvirt] [PATCH] build: Fix prohibit_int_ijk (and iijjkk) on RHEL 5
On RHEL 5, make syntax-check was failing because even strings like 'int isTempChain' matched the 'int i' rule. To be honest, I haven't found the root cause, but the change added makes it work as expected and keeps the proper behavior on newer systems as well. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Notes: I'm not pushing this one as a build breaker since I haven't found the root cause, so feel free to object and fix it differently. cfg.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cfg.mk b/cfg.mk index 56821e2..e9da282 100644 --- a/cfg.mk +++ b/cfg.mk @@ -555,12 +555,12 @@ sc_avoid_attribute_unused_in_header: $(_sc_search_regexp) sc_prohibit_int_ijk: - @prohibit='\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' \ + @prohibit='\<(int|unsigned) ([^(]* )*(i|j|k)\>(\s|,|;)' \ halt='use size_t, not int/unsigned int for loop vars i, j, k' \ $(_sc_search_regexp) sc_prohibit_loop_iijjkk: - @prohibit='\<(int|unsigned) ([^=]+ )*(ii|jj|kk)(\s|,|;)' \ + @prohibit='\<(int|unsigned) ([^=]+ )*(ii|jj|kk)\>(\s|,|;)' \ halt='use i, j, k for loop iterators, not ii, jj, kk' \ $(_sc_search_regexp) -- 1.8.4
On 10/22/2013 05:19 PM, Martin Kletzander wrote:
On RHEL 5, make syntax-check was failing because even strings like 'int isTempChain' matched the 'int i' rule. To be honest, I haven't found the root cause, but the change added makes it work as expected and keeps the proper behavior on newer systems as well.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: I'm not pushing this one as a build breaker since I haven't found the root cause, so feel free to object and fix it differently.
cfg.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 56821e2..e9da282 100644 --- a/cfg.mk +++ b/cfg.mk @@ -555,12 +555,12 @@ sc_avoid_attribute_unused_in_header: $(_sc_search_regexp)
sc_prohibit_int_ijk: - @prohibit='\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' \ + @prohibit='\<(int|unsigned) ([^(]* )*(i|j|k)\>(\s|,|;)' \
What version of grep on RHEL 5? (I'm without access to my normal RHEL 5 VM at the moment.) I'm not seeing an obvious entry in grep's NEWS file, but suspect it may be a bug in that old of a grep rather than in our regex. At any rate, I agree with the fix: ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Oct 22, 2013 at 09:52:30PM +0100, Eric Blake wrote:
On 10/22/2013 05:19 PM, Martin Kletzander wrote:
On RHEL 5, make syntax-check was failing because even strings like 'int isTempChain' matched the 'int i' rule. To be honest, I haven't found the root cause, but the change added makes it work as expected and keeps the proper behavior on newer systems as well.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: I'm not pushing this one as a build breaker since I haven't found the root cause, so feel free to object and fix it differently.
cfg.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 56821e2..e9da282 100644 --- a/cfg.mk +++ b/cfg.mk @@ -555,12 +555,12 @@ sc_avoid_attribute_unused_in_header: $(_sc_search_regexp)
sc_prohibit_int_ijk: - @prohibit='\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' \ + @prohibit='\<(int|unsigned) ([^(]* )*(i|j|k)\>(\s|,|;)' \
What version of grep on RHEL 5? (I'm without access to my normal RHEL 5 VM at the moment.) I'm not seeing an obvious entry in grep's NEWS file, but suspect it may be a bug in that old of a grep rather than in our regex. At any rate, I agree with the fix:
The thing is that when I tried reproducing it using only grep, the regexp and the file, there was no match. Here are the version-related things: # grep -V grep (GNU grep) 2.5.1 # rpm -qf $(which grep) grep-2.5.1-55.el5 // Feel free to stop by tomorrow since we're both on KVM Forum, I // can't express how much interested I am in finding the root cause // of the regexp failure.
ACK.
Anyway, thanks for the late review ;-) I pushed it now. Martin
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
At Tue, 22 Oct 2013 23:09:20 +0100, Martin Kletzander wrote:
On Tue, Oct 22, 2013 at 09:52:30PM +0100, Eric Blake wrote:
On 10/22/2013 05:19 PM, Martin Kletzander wrote:
On RHEL 5, make syntax-check was failing because even strings like 'int isTempChain' matched the 'int i' rule. To be honest, I haven't found the root cause, but the change added makes it work as expected and keeps the proper behavior on newer systems as well.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: I'm not pushing this one as a build breaker since I haven't found the root cause, so feel free to object and fix it differently.
cfg.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 56821e2..e9da282 100644 --- a/cfg.mk +++ b/cfg.mk @@ -555,12 +555,12 @@ sc_avoid_attribute_unused_in_header: $(_sc_search_regexp)
sc_prohibit_int_ijk: - @prohibit='\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' \ + @prohibit='\<(int|unsigned) ([^(]* )*(i|j|k)\>(\s|,|;)' \
What version of grep on RHEL 5? (I'm without access to my normal RHEL 5 VM at the moment.) I'm not seeing an obvious entry in grep's NEWS file, but suspect it may be a bug in that old of a grep rather than in our regex. At any rate, I agree with the fix:
The thing is that when I tried reproducing it using only grep, the regexp and the file, there was no match.
Here are the version-related things:
# grep -V grep (GNU grep) 2.5.1 # rpm -qf $(which grep) grep-2.5.1-55.el5
// Feel free to stop by tomorrow since we're both on KVM Forum, I // can't express how much interested I am in finding the root cause // of the regexp failure.
Seems \s is buggy in this grep version with a non UTF-8 locale setting. Observe: $ LANG=en_US.UTF-8 grep -nE '\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' src/conf/interface_conf.h $ LANG=C grep -nE '\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' src/conf/interface_conf.h 135: int autoconf; /* only useful if family is ipv6 */ 167: unsigned int active:1; /* 1 if interface is active (up) */ Alas, grep does not colorize anything in the output line in that case (even with --color=always), just as if it does not match anything. According to grep's info pages \s should be equivalent to [[:space:]], but it is not, as the latter works alright: $ LANG=C grep -nE '\<(int|unsigned) ([^(]*)*(i|j|k)([[:space:]]|,|;)' src/conf/interface_conf.h So, I think the right fix would be to avoid \s altogether and use [[:space:]] instead. Claudio -- AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany Phone: +49 341 265 310 19 Web:<http://www.av-test.org> Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076) Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
On 10/23/2013 09:33 AM, Claudio Bley wrote:
Seems \s is buggy in this grep version with a non UTF-8 locale setting. Observe:
$ LANG=en_US.UTF-8 grep -nE '\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' src/conf/interface_conf.h $ LANG=C grep -nE '\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' src/conf/interface_conf.h
But 'syntax-check' should be already using grep in the C locale (if not, that's a bug upstream in gnulib).
According to grep's info pages \s should be equivalent to [[:space:]], but it is not, as the latter works alright:
$ LANG=C grep -nE '\<(int|unsigned) ([^(]*)*(i|j|k)([[:space:]]|,|;)' src/conf/interface_conf.h
If we go this route, we could simplify even further: $ LANG=C grep -nE '\<(int|unsigned) ([^(] *)*[ijk][:space:],:]' src/conf/interface_conf.h
So, I think the right fix would be to avoid \s altogether and use [[:space:]] instead.
The \s usage was good enough to work around the grep bug, which is all the more we need to get the build working on RHEL 5, so I'm not worried about further patches (unless upstream gnulib really does have a bug for not using C locale). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
At Wed, 23 Oct 2013 10:46:46 +0100, Eric Blake wrote:
On 10/23/2013 09:33 AM, Claudio Bley wrote:
Seems \s is buggy in this grep version with a non UTF-8 locale setting. Observe:
$ LANG=en_US.UTF-8 grep -nE '\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' src/conf/interface_conf.h $ LANG=C grep -nE '\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' src/conf/interface_conf.h
But 'syntax-check' should be already using grep in the C locale (if not, that's a bug upstream in gnulib).
But that's the point, the bug manifests itself with LANG=C, NOT with LANG=*.UTF-8
According to grep's info pages \s should be equivalent to [[:space:]], but it is not, as the latter works alright:
$ LANG=C grep -nE '\<(int|unsigned) ([^(]*)*(i|j|k)([[:space:]]|,|;)' src/conf/interface_conf.h
If we go this route, we could simplify even further:
$ LANG=C grep -nE '\<(int|unsigned) ([^(] *)*[ijk][:space:],:]' src/conf/interface_conf.h
So, I think the right fix would be to avoid \s altogether and use [[:space:]] instead.
The \s usage was good enough to work around the grep bug
I'm confused. Which bug are you talking about?
which is all the more we need to get the build working on RHEL 5
So, what does \s match with, when using LANG=C ? Claudio -- AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany Phone: +49 341 265 310 19 Web:<http://www.av-test.org> Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076) Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
On 10/23/2013 11:02 AM, Claudio Bley wrote:
Seems \s is buggy in this grep version with a non UTF-8 locale setting. Observe:
$ LANG=en_US.UTF-8 grep -nE '\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' src/conf/interface_conf.h $ LANG=C grep -nE '\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' src/conf/interface_conf.h
But 'syntax-check' should be already using grep in the C locale (if not, that's a bug upstream in gnulib).
But that's the point, the bug manifests itself with LANG=C, NOT with LANG=*.UTF-8
Ah, I see - the bug in RHEL 5 grep is in LANG=C.
So, I think the right fix would be to avoid \s altogether and use [[:space:]] instead.
The \s usage was good enough to work around the grep bug
I'm confused. Which bug are you talking about?
Oops, I typed one thing but meant another: the \> fix (that Martin has already pushed) is all the more we need to work around the RHEL 5 grep.
So, what does \s match with, when using LANG=C ?
It's supposed to match space, but that's a problem for the RHEL 5 team to answer. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
At Wed, 23 Oct 2013 11:08:48 +0100, Eric Blake wrote:
On 10/23/2013 11:02 AM, Claudio Bley wrote:
Seems \s is buggy in this grep version with a non UTF-8 locale setting. Observe:
$ LANG=en_US.UTF-8 grep -nE '\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' src/conf/interface_conf.h $ LANG=C grep -nE '\<(int|unsigned) ([^(]* )*(i|j|k)(\s|,|;)' src/conf/interface_conf.h
But 'syntax-check' should be already using grep in the C locale (if not, that's a bug upstream in gnulib).
But that's the point, the bug manifests itself with LANG=C, NOT with LANG=*.UTF-8
Ah, I see - the bug in RHEL 5 grep is in LANG=C.
So, I think the right fix would be to avoid \s altogether and use [[:space:]] instead.
The \s usage was good enough to work around the grep bug
I'm confused. Which bug are you talking about?
Oops, I typed one thing but meant another: the \> fix (that Martin has already pushed) is all the more we need to work around the RHEL 5 grep.
Well, since nobody knows what \s actually does to the state of the grep matcher, you can't be sure. Fact is, however, it avoids the syntax-check errors.
So, what does \s match with, when using LANG=C ?
It's supposed to match space, but that's a problem for the RHEL 5 team to answer.
Particularly when considering the other usages of \s throughout the code base... Claudio -- AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany Phone: +49 341 265 310 19 Web:<http://www.av-test.org> Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076) Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
participants (3)
-
Claudio Bley -
Eric Blake -
Martin Kletzander