The same issue, but arrange the fix in each separated file
Pingfan Liu (4): kdump-lib.sh: fix incorrect usage with pipe as input for grep -q dracut-module-setup.sh: fix incorrect usage with pipe as input for grep -q kdumpctl: fix incorrect usage with pipe as input for grep -q dracut-kdump.sh: fix incorrect usage with pipe as input for grep -q
dracut-kdump.sh | 2 +- dracut-module-setup.sh | 4 ++-- kdump-lib.sh | 12 ++++++------ kdumpctl | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-)
For -q option, as man grep says: Exit immediately with zero status if any match is found, even if an error was detected. So when matching, the read side of pipe is closed by "grep -q", while the write side still try to write more data, which cause SIGPIPE to the process, and the shell can not exit with 0.
Signed-off-by: Pingfan Liu piliu@redhat.com --- kdump-lib.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index e3b5a01..21e05f1 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -63,7 +63,7 @@ is_pcs_fence_kdump() [ -x $FENCE_KDUMP_SEND ] || return 1
# fence kdump not configured? - (pcs cluster cib | grep -q 'type="fence_kdump"') &> /dev/null || return 1 + (pcs cluster cib | grep 'type="fence_kdump"') &> /dev/null || return 1 }
# Check if fence_kdump is configured using kdump options @@ -233,7 +233,7 @@ is_atomic()
is_ipv6_address() { - echo $1 | grep -q ":" + (echo $1 | grep ":") &> /dev/null }
# get ip address or hostname from nfs/ssh config value @@ -257,7 +257,7 @@ is_hostname() if [ -n "$_hostname" ]; then return 1 fi - echo $1 | grep -q "[a-zA-Z]" + (echo $1 | grep "[a-zA-Z]") &> /dev/null }
# Copied from "/etc/sysconfig/network-scripts/network-functions" @@ -303,8 +303,8 @@ is_nm_running()
is_nm_handling() { - LANG=C nmcli -t --fields device,state dev status 2>/dev/null \ - | grep -q "^(${1}:connected)|(${1}:connecting.*)$" + (LANG=C nmcli -t --fields device,state dev status \ + | grep "^(${1}:connected)|(${1}:connecting.*)$") &> /dev/null }
# $1: netdev name @@ -377,7 +377,7 @@ is_wdt_mod_omitted() { [[ -z $1 ]] && break case $1 in -o|--omit) - echo $2 | grep -qw "watchdog" + (echo $2 | grep -w "watchdog") &> /dev/null [[ $? == 0 ]] && ret=0 break esac
On Tuesday 07 February 2017 11:59 AM, Pingfan Liu wrote:
For -q option, as man grep says: Exit immediately with zero status if any match is found, even if an error was detected. So when matching, the read side of pipe is closed by "grep -q", while the write side still try to write more data, which cause SIGPIPE to the process, and the shell can not exit with 0.
While it seems from the above logic that there might be some corner cases where we will have SIGPIPE, but do we really need to replace it everywhere. `grep -q` could be faster than `grep`.
Signed-off-by: Pingfan Liu piliu@redhat.com
Anyway, as Dave suggested, an example of failure case could be good.
Otherwise, patches looks fine.
~Pratyush
kdump-lib.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index e3b5a01..21e05f1 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -63,7 +63,7 @@ is_pcs_fence_kdump() [ -x $FENCE_KDUMP_SEND ] || return 1
# fence kdump not configured?
- (pcs cluster cib | grep -q 'type="fence_kdump"') &> /dev/null || return 1
- (pcs cluster cib | grep 'type="fence_kdump"') &> /dev/null || return 1
}
# Check if fence_kdump is configured using kdump options @@ -233,7 +233,7 @@ is_atomic()
is_ipv6_address() {
- echo $1 | grep -q ":"
- (echo $1 | grep ":") &> /dev/null
}
# get ip address or hostname from nfs/ssh config value @@ -257,7 +257,7 @@ is_hostname() if [ -n "$_hostname" ]; then return 1 fi
- echo $1 | grep -q "[a-zA-Z]"
- (echo $1 | grep "[a-zA-Z]") &> /dev/null
}
# Copied from "/etc/sysconfig/network-scripts/network-functions" @@ -303,8 +303,8 @@ is_nm_running()
is_nm_handling() {
- LANG=C nmcli -t --fields device,state dev status 2>/dev/null \
| grep -q "^\(${1}:connected\)\|\(${1}:connecting.*\)$"
- (LANG=C nmcli -t --fields device,state dev status \
| grep "^\(${1}:connected\)\|\(${1}:connecting.*\)$") &> /dev/null}
# $1: netdev name @@ -377,7 +377,7 @@ is_wdt_mod_omitted() { [[ -z $1 ]] && break case $1 in -o|--omit)
echo $2 | grep -qw "watchdog"
(echo $2 | grep -w "watchdog") &> /dev/null [[ $? == 0 ]] && ret=0 break
----- Original Message -----
From: "Pratyush Anand" panand@redhat.com To: "Pingfan Liu" piliu@redhat.com, kexec@lists.fedoraproject.org Cc: "Dave Young" dyoung@redhat.com, "Xunlei Pang" xpang@redhat.com, "Baoquan He" bhe@redhat.com Sent: Wednesday, February 8, 2017 1:33:12 PM Subject: Re: [f25 PATCH 1/4] kdump-lib.sh: fix incorrect usage with pipe as input for grep -q
On Tuesday 07 February 2017 11:59 AM, Pingfan Liu wrote:
For -q option, as man grep says: Exit immediately with zero status if any match is found, even if an error was detected. So when matching, the read side of pipe is closed by "grep -q", while the write side still try to write more data, which cause SIGPIPE to the process, and the shell can not exit with 0.
While it seems from the above logic that there might be some corner cases where we will have SIGPIPE, but do we really need to replace it everywhere. `grep -q` could be faster than `grep`.
Yeah. But its behavior relies on the kernel implement of pipe. I think >4KB can cause the problem
Thx, Pingfan
Signed-off-by: Pingfan Liu piliu@redhat.com
Anyway, as Dave suggested, an example of failure case could be good.
Otherwise, patches looks fine.
~Pratyush
kdump-lib.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index e3b5a01..21e05f1 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -63,7 +63,7 @@ is_pcs_fence_kdump() [ -x $FENCE_KDUMP_SEND ] || return 1
# fence kdump not configured?
- (pcs cluster cib | grep -q 'type="fence_kdump"') &> /dev/null ||
return 1
- (pcs cluster cib | grep 'type="fence_kdump"') &> /dev/null || return 1
}
# Check if fence_kdump is configured using kdump options @@ -233,7 +233,7 @@ is_atomic()
is_ipv6_address() {
- echo $1 | grep -q ":"
- (echo $1 | grep ":") &> /dev/null
}
# get ip address or hostname from nfs/ssh config value @@ -257,7 +257,7 @@ is_hostname() if [ -n "$_hostname" ]; then return 1 fi
- echo $1 | grep -q "[a-zA-Z]"
- (echo $1 | grep "[a-zA-Z]") &> /dev/null
}
# Copied from "/etc/sysconfig/network-scripts/network-functions" @@ -303,8 +303,8 @@ is_nm_running()
is_nm_handling() {
- LANG=C nmcli -t --fields device,state dev status 2>/dev/null \
| grep -q "^\(${1}:connected\)\|\(${1}:connecting.*\)$"
- (LANG=C nmcli -t --fields device,state dev status \
| grep "^\(${1}:connected\)\|\(${1}:connecting.*\)$") &>/dev/null }
# $1: netdev name @@ -377,7 +377,7 @@ is_wdt_mod_omitted() { [[ -z $1 ]] && break case $1 in -o|--omit)
echo $2 | grep -qw "watchdog"
(echo $2 | grep -w "watchdog") &> /dev/null [[ $? == 0 ]] && ret=0 break
For -q option, as man grep says: Exit immediately with zero status if any match is found, even if an error was detected. So when matching, the read side of pipe is closed by "grep -q", while the write side still try to write more data, which cause SIGPIPE to the process, and the shell can not exit with 0.
Signed-off-by: Pingfan Liu piliu@redhat.com --- dracut-module-setup.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh index 1f96bb8..476e274 100755 --- a/dracut-module-setup.sh +++ b/dracut-module-setup.sh @@ -92,7 +92,7 @@ kdump_setup_dns() { _dns=$(echo $_nameserver | cut -d' ' -f2) [ -z "$_dns" ] && continue
- if [ ! -f $_dnsfile ] || [ ! $(cat $_dnsfile | grep -q $_dns) ]; then + if [ ! -f $_dnsfile ] || [ ! $((cat $_dnsfile | grep $_dns) &> /dev/null) ]; then echo "nameserver=$_dns" >> "$_dnsfile" fi done < "/etc/resolv.conf" @@ -319,7 +319,7 @@ kdump_setup_netdev() {
get_ip_route_field() { - if `echo $1 | grep -q $2`; then + if [ ! $((echo $1 | grep $2) &> /dev/null) ]; then echo ${1##*$2} | cut -d ' ' -f1 fi }
For -q option, as man grep says: Exit immediately with zero status if any match is found, even if an error was detected. So when matching, the read side of pipe is closed by "grep -q", while the write side still try to write more data, which cause SIGPIPE to the process, and the shell can not exit with 0.
Signed-off-by: Pingfan Liu piliu@redhat.com --- kdumpctl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kdumpctl b/kdumpctl index 4d68be0..c755835 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1126,7 +1126,7 @@ start() return 1 fi
- if sestatus 2>/dev/null | grep -q "SELinux status.*enabled"; then + if [ $((sestatus | grep "SELinux status.*enabled") &> /dev/null) ]; then selinux_relabel fi save_raw
For -q option, as man grep says: Exit immediately with zero status if any match is found, even if an error was detected. So when matching, the read side of pipe is closed by "grep -q", while the write side still try to write more data, which cause SIGPIPE to the process, and the shell can not exit with 0.
Signed-off-by: Pingfan Liu piliu@redhat.com --- dracut-kdump.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dracut-kdump.sh b/dracut-kdump.sh index b75c2a5..b5b4f74 100755 --- a/dracut-kdump.sh +++ b/dracut-kdump.sh @@ -55,7 +55,7 @@ dump_raw()
echo "kdump: saving to raw disk $_raw"
- if ! $(echo -n $CORE_COLLECTOR|grep -q makedumpfile); then + if [ ! $((echo -n $CORE_COLLECTOR|grep makedumpfile) &> /dev/null) ]; then _src_size=`ls -l /proc/vmcore | cut -d' ' -f5` _src_size_mb=$(($_src_size / 1048576)) monitor_dd_progress $_src_size_mb &
On 02/07/17 at 02:29pm, Pingfan Liu wrote:
The same issue, but arrange the fix in each separated file
Pingfan Liu (4): kdump-lib.sh: fix incorrect usage with pipe as input for grep -q dracut-module-setup.sh: fix incorrect usage with pipe as input for grep -q kdumpctl: fix incorrect usage with pipe as input for grep -q dracut-kdump.sh: fix incorrect usage with pipe as input for grep -q
dracut-kdump.sh | 2 +- dracut-module-setup.sh | 4 ++-- kdump-lib.sh | 12 ++++++------ kdumpctl | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-)
-- 2.7.4 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
Hi Pingfan
I suppose it is for Fedora rawhide (master branch) instead of F25? We post patches to master (rawhide) first, then if people request the bug as critical we can evaluate and backport to released versions.
So can you repost with dropping f25 prefix?
Also about the `grep -q` can you give out a concrect example of the failure in some sample shell script?
Thanks Dave
----- Original Message -----
From: "Dave Young" dyoung@redhat.com To: "Pingfan Liu" piliu@redhat.com Cc: kexec@lists.fedoraproject.org, "Xunlei Pang" xpang@redhat.com, "Baoquan He" bhe@redhat.com Sent: Wednesday, February 8, 2017 11:31:13 AM Subject: Re: [f25 PATCH 0/4] patchset to correct the usage of "grep -q"
On 02/07/17 at 02:29pm, Pingfan Liu wrote:
The same issue, but arrange the fix in each separated file
Pingfan Liu (4): kdump-lib.sh: fix incorrect usage with pipe as input for grep -q dracut-module-setup.sh: fix incorrect usage with pipe as input for grep -q kdumpctl: fix incorrect usage with pipe as input for grep -q dracut-kdump.sh: fix incorrect usage with pipe as input for grep -q
dracut-kdump.sh | 2 +- dracut-module-setup.sh | 4 ++-- kdump-lib.sh | 12 ++++++------ kdumpctl | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-)
-- 2.7.4 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
Hi Pingfan
I suppose it is for Fedora rawhide (master branch) instead of F25? We post patches to master (rawhide) first, then if people request the bug as critical we can evaluate and backport to released versions.
So can you repost with dropping f25 prefix?
OK
Also about the `grep -q` can you give out a concrect example of the failure in some sample shell script?
set -o pipefail cat test.file | grep -q "keyword" echo $?
If test.file is big enough, it always returns 141. In my test, 7KB is enough, but I think the detail depends on the implement of pipe in kernel. Should I append this into commit log?
Thx, Pingfan
-
Dave Young -
Pingfan Liu -
Pratyush Anand