[dbscripts,4/8] Use `grep &>/dev/null` instead of `grep -q` when operating on piped stdin.

Message ID 20180314015205.20781-5-lukeshu@lukeshu.com
State Accepted
Headers show
Series
  • Backports from Parabola
Related show

Commit Message

Luke Shumaker March 14, 2018, 1:52 a.m. UTC
From: Luke Shumaker <lukeshu@parabola.nu>

`grep -q` may exit as soon as it finds a match; this is a good optimization
for when the input is a file.  However, if the input is the output of
another program, then that other program will receive SIGPIPE, and further
writes will fail.  When this happens, it might (bsdtar does) print a
message about a "write error" to stderr.  Which is going to confuse and
alarm the user.

In one of the cases, this had already been mitigated by wrapping
bsdtar in "echo "$(bsdtar ...)", as Bash builtin echo doesn't complain
if it gets SIGPIPE.  However, that means we're storing the entire
output of bsdtar in memory, which is silly.
---
 db-functions         | 2 +-
 test/lib/common.bash | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Eli Schwartz via arch-projects March 14, 2018, 4:11 a.m. UTC | #1
On 03/13/2018 09:52 PM, Luke Shumaker wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> `grep -q` may exit as soon as it finds a match; this is a good optimization
> for when the input is a file.  However, if the input is the output of
> another program, then that other program will receive SIGPIPE, and further
> writes will fail.  When this happens, it might (bsdtar does) print a
> message about a "write error" to stderr.  Which is going to confuse and
> alarm the user.
> 
> In one of the cases, this had already been mitigated by wrapping
> bsdtar in "echo "$(bsdtar ...)", as Bash builtin echo doesn't complain
> if it gets SIGPIPE.  However, that means we're storing the entire
> output of bsdtar in memory, which is silly.
> ---
>  db-functions         | 2 +-
>  test/lib/common.bash | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/db-functions b/db-functions
> index 58b753a..ee390ff 100644
> --- a/db-functions
> +++ b/db-functions
> @@ -303,7 +303,7 @@ check_pkgfile() {
>  
>  	in_array "${pkgarch}" "${ARCHES[@]}" 'any' || return 1
>  
> -	if echo "${pkgfile##*/}" | grep -q "^${pkgname}-${pkgver}-${pkgarch}"; then
> +	if echo "${pkgfile##*/}" | grep "^${pkgname}-${pkgver}-${pkgarch}" &>/dev/null; then

But echo should be fine anyway?

Regardless this could be so much more elegant.

if [[ $pkgfile = $pkgname-$pkgver-$pkgrel-$arch* ]]; then

>  		return 0
>  	else
>  		return 1
> diff --git a/test/lib/common.bash b/test/lib/common.bash
> index 45e4800..ab805dd 100644
> --- a/test/lib/common.bash
> +++ b/test/lib/common.bash
> @@ -215,7 +215,7 @@ checkPackageDB() {
>  
>  				for db in ${DBEXT} ${FILESEXT}; do
>  					[ -r "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" ]
> -					bsdtar -xf "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" -O | grep -q "${pkgfile%${PKGEXT}}"
> +					bsdtar -xf "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" -O | grep "${pkgfile%${PKGEXT}}" &>/dev/null
>  				done
>  			done
>  		done
> @@ -269,7 +269,7 @@ checkRemovedPackageDB() {
>  		for tarch in ${tarches[@]}; do
>  			if [ -r "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" ]; then
>  				for pkgname in ${pkgnames[@]}; do
> -					echo "$(bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O)" | grep -qv ${pkgname}
> +					bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O | grep -v ${pkgname} &>/dev/null
>  				done
>  			fi
>  		done
>
Luke Shumaker March 14, 2018, 4:16 a.m. UTC | #2
On Tue, 13 Mar 2018 21:52:01 -0400,
Luke Shumaker wrote:
> 
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> `grep -q` may exit as soon as it finds a match; this is a good optimization
> for when the input is a file.  However, if the input is the output of
> another program, then that other program will receive SIGPIPE, and further
> writes will fail.  When this happens, it might (bsdtar does) print a
> message about a "write error" to stderr.  Which is going to confuse and
> alarm the user.
> 
> In one of the cases, this had already been mitigated by wrapping
> bsdtar in "echo "$(bsdtar ...)", as Bash builtin echo doesn't complain
> if it gets SIGPIPE.  However, that means we're storing the entire
> output of bsdtar in memory, which is silly.
> ---

> -					echo "$(bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O)" | grep -qv ${pkgname}
> +					bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O | grep -v ${pkgname} &>/dev/null

This is broken.

As the commit message said, the subshell soaks up the full output to
avoid SIGPIPE.  But it also has a nother subtle purpose: to ensure
that at least one "\n" is written to grep's stdin (as echo appends
"\n").  Otherwise, if the db is now empty, grep will fail because it
didn't recieve anything.

The correct command is

	! bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O | grep ${pkgname} &>/dev/null
Luke Shumaker March 14, 2018, 4:59 a.m. UTC | #3
On Wed, 14 Mar 2018 00:11:12 -0400,
Eli Schwartz wrote:
> 
> On 03/13/2018 09:52 PM, Luke Shumaker wrote:
> > From: Luke Shumaker <lukeshu@parabola.nu>
> > 
> > `grep -q` may exit as soon as it finds a match; this is a good optimization
> > for when the input is a file.  However, if the input is the output of
> > another program, then that other program will receive SIGPIPE, and further
> > writes will fail.  When this happens, it might (bsdtar does) print a
> > message about a "write error" to stderr.  Which is going to confuse and
> > alarm the user.
> > 
> > In one of the cases, this had already been mitigated by wrapping
> > bsdtar in "echo "$(bsdtar ...)", as Bash builtin echo doesn't complain
> > if it gets SIGPIPE.  However, that means we're storing the entire
> > output of bsdtar in memory, which is silly.
> > ---
> >  db-functions         | 2 +-
> >  test/lib/common.bash | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/db-functions b/db-functions
> > index 58b753a..ee390ff 100644
> > --- a/db-functions
> > +++ b/db-functions
> > @@ -303,7 +303,7 @@ check_pkgfile() {
> >  
> >  	in_array "${pkgarch}" "${ARCHES[@]}" 'any' || return 1
> >  
> > -	if echo "${pkgfile##*/}" | grep -q "^${pkgname}-${pkgver}-${pkgarch}"; then
> > +	if echo "${pkgfile##*/}" | grep "^${pkgname}-${pkgver}-${pkgarch}" &>/dev/null; then
> 
> But echo should be fine anyway?

Yeah, in this case it's for consistency with the others.  It's easier
to remember "don't use `grep -q` on other commands' stdout" that it is
to work out when it's ok and when it isn't.

> Regardless this could be so much more elegant.
> 
> if [[ $pkgfile = $pkgname-$pkgver-$pkgrel-$arch* ]]; then

You're right, that is better.

Patch

diff --git a/db-functions b/db-functions
index 58b753a..ee390ff 100644
--- a/db-functions
+++ b/db-functions
@@ -303,7 +303,7 @@  check_pkgfile() {
 
 	in_array "${pkgarch}" "${ARCHES[@]}" 'any' || return 1
 
-	if echo "${pkgfile##*/}" | grep -q "^${pkgname}-${pkgver}-${pkgarch}"; then
+	if echo "${pkgfile##*/}" | grep "^${pkgname}-${pkgver}-${pkgarch}" &>/dev/null; then
 		return 0
 	else
 		return 1
diff --git a/test/lib/common.bash b/test/lib/common.bash
index 45e4800..ab805dd 100644
--- a/test/lib/common.bash
+++ b/test/lib/common.bash
@@ -215,7 +215,7 @@  checkPackageDB() {
 
 				for db in ${DBEXT} ${FILESEXT}; do
 					[ -r "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" ]
-					bsdtar -xf "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" -O | grep -q "${pkgfile%${PKGEXT}}"
+					bsdtar -xf "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" -O | grep "${pkgfile%${PKGEXT}}" &>/dev/null
 				done
 			done
 		done
@@ -269,7 +269,7 @@  checkRemovedPackageDB() {
 		for tarch in ${tarches[@]}; do
 			if [ -r "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" ]; then
 				for pkgname in ${pkgnames[@]}; do
-					echo "$(bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O)" | grep -qv ${pkgname}
+					bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O | grep -v ${pkgname} &>/dev/null
 				done
 			fi
 		done