All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] common/rc: fix MKSWAP_PROG quoting
@ 2021-08-31  3:04 Murphy Zhou
  2021-08-31  5:26 ` Zorro Lang
  0 siblings, 1 reply; 9+ messages in thread
From: Murphy Zhou @ 2021-08-31  3:04 UTC (permalink / raw)
  To: fstests; +Cc: Luis Chamberlain

After commit
  0e4dd8b9 common/rc: fix ignoring of errors on
we are getting this error message when running swapfiles tests:
  +./common/rc: line 2553: MKSWAP_PROG: command not found

Signed-off-by: Murphy Zhou <xzhou@redhat.com>
---
 common/rc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 46b6b220..0597de13 100644
--- a/common/rc
+++ b/common/rc
@@ -2550,7 +2550,7 @@ _format_swapfile() {
 	$CHATTR_PROG +C "$fname" > /dev/null 2>&1
 	_pwrite_byte 0x61 0 "$sz" "$fname" >> $seqres.full
 	# Ignore permission complaints on filesystems that don't support perms
-	$(MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full
+	($MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full
 }
 
 _swapon_file() {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] common/rc: fix MKSWAP_PROG quoting
  2021-08-31  3:04 [PATCH] common/rc: fix MKSWAP_PROG quoting Murphy Zhou
@ 2021-08-31  5:26 ` Zorro Lang
  2021-08-31  7:38   ` Pavel Reichl
  0 siblings, 1 reply; 9+ messages in thread
From: Zorro Lang @ 2021-08-31  5:26 UTC (permalink / raw)
  To: Murphy Zhou; +Cc: fstests, Luis Chamberlain

On Tue, Aug 31, 2021 at 11:04:26AM +0800, Murphy Zhou wrote:
> After commit
>   0e4dd8b9 common/rc: fix ignoring of errors on
> we are getting this error message when running swapfiles tests:
>   +./common/rc: line 2553: MKSWAP_PROG: command not found
> 
> Signed-off-by: Murphy Zhou <xzhou@redhat.com>
> ---
>  common/rc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index 46b6b220..0597de13 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2550,7 +2550,7 @@ _format_swapfile() {
>  	$CHATTR_PROG +C "$fname" > /dev/null 2>&1
>  	_pwrite_byte 0x61 0 "$sz" "$fname" >> $seqres.full
>  	# Ignore permission complaints on filesystems that don't support perms
> -	$(MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full
> +	($MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full

The change history is:

Commit 0c95c8ac tried to "hide permision warning", so did:
-       $MKSWAP_PROG "$fname" >> $seqres.full
+       # Ignore permission complaints on filesystems that don't support perms
+       $MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2) >> $seqres.full

Then commit 0e4dd8b9 said "it broke older versions of bash such as 4.4.23", so tried
to use "a $(foo) to run the executable":
-       $MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2) >> $seqres.full
+       $(MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full

Now this patch try to do ($FOO_PROG):
($MKSWAP_PROG "$fname" .....)

I'm *not* a bash expert, if I'm wrong feel free to correct me:) If the original problem
is trying to hide permision warning from stderr, and to avoid new syntax breaking old
bash, Why we must struggle with this complex syntax which isn't compatible. How about:

$MKSWAP_PROG "$fname" >> $seqres.full 2>&1 | grep -v "insecure permission"

Or other better and compatible way ?

Thanks,
Zorro

>  }
>  
>  _swapon_file() {
> -- 
> 2.20.1
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] common/rc: fix MKSWAP_PROG quoting
  2021-08-31  5:26 ` Zorro Lang
@ 2021-08-31  7:38   ` Pavel Reichl
  2021-08-31 13:42     ` Zorro Lang
  0 siblings, 1 reply; 9+ messages in thread
From: Pavel Reichl @ 2021-08-31  7:38 UTC (permalink / raw)
  To: Murphy Zhou, fstests, Luis Chamberlain


On 8/31/21 7:26 AM, Zorro Lang wrote:
> On Tue, Aug 31, 2021 at 11:04:26AM +0800, Murphy Zhou wrote:
>> After commit
>>    0e4dd8b9 common/rc: fix ignoring of errors on
>> we are getting this error message when running swapfiles tests:
>>    +./common/rc: line 2553: MKSWAP_PROG: command not found
>>
>> Signed-off-by: Murphy Zhou <xzhou@redhat.com>
>> ---
>>   common/rc | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 46b6b220..0597de13 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -2550,7 +2550,7 @@ _format_swapfile() {
>>   	$CHATTR_PROG +C "$fname" > /dev/null 2>&1
>>   	_pwrite_byte 0x61 0 "$sz" "$fname" >> $seqres.full
>>   	# Ignore permission complaints on filesystems that don't support perms
>> -	$(MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full
>> +	($MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full
> The change history is:
>
> Commit 0c95c8ac tried to "hide permision warning", so did:
> -       $MKSWAP_PROG "$fname" >> $seqres.full
> +       # Ignore permission complaints on filesystems that don't support perms
> +       $MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2) >> $seqres.full
>
> Then commit 0e4dd8b9 said "it broke older versions of bash such as 4.4.23", so tried
> to use "a $(foo) to run the executable":
> -       $MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2) >> $seqres.full
> +       $(MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full
>
> Now this patch try to do ($FOO_PROG):
> ($MKSWAP_PROG "$fname" .....)
>
> I'm *not* a bash expert, if I'm wrong feel free to correct me:) If the original problem
> is trying to hide permision warning from stderr, and to avoid new syntax breaking old
> bash, Why we must struggle with this complex syntax which isn't compatible. How about:

Hi, sorry I do suck at bash, but:

>
> $MKSWAP_PROG "$fname" >> $seqres.full 2>&1 | grep -v "insecure permission"

 >> redirects stdout to the seqres.full and the 2>&1 redirects stderr to 
wherever stdout goes...so $MKSWAP_PROG "$fname" >> $seqres.full 2>&1 
sends both stdout and stderr to the file so nothing is piped to grep. Or 
do I miss something (which is entirely possible)?


Thanks!


>
> Or other better and compatible way ?
>
> Thanks,
> Zorro
>
>>   }
>>   
>>   _swapon_file() {
>> -- 
>> 2.20.1
>>


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] common/rc: fix MKSWAP_PROG quoting
  2021-08-31  7:38   ` Pavel Reichl
@ 2021-08-31 13:42     ` Zorro Lang
  2021-08-31 17:05       ` Theodore Ts'o
  0 siblings, 1 reply; 9+ messages in thread
From: Zorro Lang @ 2021-08-31 13:42 UTC (permalink / raw)
  To: Pavel Reichl; +Cc: Murphy Zhou, fstests, Luis Chamberlain

On Tue, Aug 31, 2021 at 09:38:25AM +0200, Pavel Reichl wrote:
> 
> On 8/31/21 7:26 AM, Zorro Lang wrote:
> > On Tue, Aug 31, 2021 at 11:04:26AM +0800, Murphy Zhou wrote:
> > > After commit
> > >    0e4dd8b9 common/rc: fix ignoring of errors on
> > > we are getting this error message when running swapfiles tests:
> > >    +./common/rc: line 2553: MKSWAP_PROG: command not found
> > > 
> > > Signed-off-by: Murphy Zhou <xzhou@redhat.com>
> > > ---
> > >   common/rc | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index 46b6b220..0597de13 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -2550,7 +2550,7 @@ _format_swapfile() {
> > >   	$CHATTR_PROG +C "$fname" > /dev/null 2>&1
> > >   	_pwrite_byte 0x61 0 "$sz" "$fname" >> $seqres.full
> > >   	# Ignore permission complaints on filesystems that don't support perms
> > > -	$(MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full
> > > +	($MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full
> > The change history is:
> > 
> > Commit 0c95c8ac tried to "hide permision warning", so did:
> > -       $MKSWAP_PROG "$fname" >> $seqres.full
> > +       # Ignore permission complaints on filesystems that don't support perms
> > +       $MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2) >> $seqres.full
> > 
> > Then commit 0e4dd8b9 said "it broke older versions of bash such as 4.4.23", so tried
> > to use "a $(foo) to run the executable":
> > -       $MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2) >> $seqres.full
> > +       $(MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full
> > 
> > Now this patch try to do ($FOO_PROG):
> > ($MKSWAP_PROG "$fname" .....)
> > 
> > I'm *not* a bash expert, if I'm wrong feel free to correct me:) If the original problem
> > is trying to hide permision warning from stderr, and to avoid new syntax breaking old
> > bash, Why we must struggle with this complex syntax which isn't compatible. How about:
> 
> Hi, sorry I do suck at bash, but:
> 
> > 
> > $MKSWAP_PROG "$fname" >> $seqres.full 2>&1 | grep -v "insecure permission"
> 
> >> redirects stdout to the seqres.full and the 2>&1 redirects stderr to
> wherever stdout goes...so $MKSWAP_PROG "$fname" >> $seqres.full 2>&1 sends
> both stdout and stderr to the file so nothing is piped to grep. Or do I miss
> something (which is entirely possible)?

Sorry, I reversed the order. I mean:

$MKSWAP_PROG "$fname" 2>&1 >> $seqres.full | grep -v "insecure permission"

Thanks,
Zorro

> 
> 
> Thanks!
> 
> 
> > 
> > Or other better and compatible way ?
> > 
> > Thanks,
> > Zorro
> > 
> > >   }
> > >   _swapon_file() {
> > > -- 
> > > 2.20.1
> > > 
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] common/rc: fix MKSWAP_PROG quoting
  2021-08-31 13:42     ` Zorro Lang
@ 2021-08-31 17:05       ` Theodore Ts'o
  2021-09-01  6:16         ` [PATCH v2] " Murphy Zhou
  0 siblings, 1 reply; 9+ messages in thread
From: Theodore Ts'o @ 2021-08-31 17:05 UTC (permalink / raw)
  To: Pavel Reichl, Murphy Zhou, fstests, Luis Chamberlain

On Tue, Aug 31, 2021 at 09:42:07PM +0800, Zorro Lang wrote:
> 
> Sorry, I reversed the order. I mean:
> 
> $MKSWAP_PROG "$fname" 2>&1 >> $seqres.full | grep -v "insecure permission"

That looks right, although I might have done something like this:

filter_mkswap_stderr()
{
   grep -v "insecure permission"
}

...

$MKSWAP_PROG "$fname" 2>&1 >> $seqres.full | filter_mkswap_stderr

More for the documentation effect rather than the abstraction.  But I
wouldn't insist on it; some folks might consider this to be excess
levels abstraction.

					- Ted

^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH v2] common/rc: fix MKSWAP_PROG quoting
  2021-08-31 17:05       ` Theodore Ts'o
@ 2021-09-01  6:16         ` Murphy Zhou
  2021-09-01  7:15           ` Dave Chinner
  0 siblings, 1 reply; 9+ messages in thread
From: Murphy Zhou @ 2021-09-01  6:16 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Pavel Reichl, Murphy Zhou, fstests, Luis Chamberlain

After commit
  0e4dd8b9 common/rc: fix ignoring of errors on
we are getting this error message when running swapfiles tests:
  +./common/rc: line 2553: MKSWAP_PROG: command not found

Rewrite the line and add a filter for mkswap.

Signed-off-by: Murphy Zhou <xzhou@redhat.com>
---

Thanks all for the suggestions!

v2:
  rewrite the line and add a filter
  source filters for generic/643

 common/filter     | 6 ++++++
 common/rc         | 2 +-
 tests/generic/643 | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/common/filter b/common/filter
index 2efbbd99..0883a3fd 100644
--- a/common/filter
+++ b/common/filter
@@ -661,5 +661,11 @@ _filter_quota_report()
 		s|^(.*?) (\d+) (\d+) (\d+)|$1 @{[$2 * 1024 /'$bsize']} @{[$3 * 1024 /'$bsize']} @{[$4 * 1024 /'$bsize']}|'
 }
 
+# Filter mkswap warning about the permissions
+_filter_mkswap_stderr()
+{
+	grep -v "insecure permission"
+}
+
 # make sure this script returns success
 /bin/true
diff --git a/common/rc b/common/rc
index 46b6b220..0a3933a5 100644
--- a/common/rc
+++ b/common/rc
@@ -2550,7 +2550,7 @@ _format_swapfile() {
 	$CHATTR_PROG +C "$fname" > /dev/null 2>&1
 	_pwrite_byte 0x61 0 "$sz" "$fname" >> $seqres.full
 	# Ignore permission complaints on filesystems that don't support perms
-	$(MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full
+	$MKSWAP_PROG "$fname" 2>&1 >> $seqres.full | _filter_mkswap_stderr
 }
 
 _swapon_file() {
diff --git a/tests/generic/643 b/tests/generic/643
index 2bb9d220..9b067ea2 100755
--- a/tests/generic/643
+++ b/tests/generic/643
@@ -21,6 +21,7 @@ _cleanup()
 	rm -f $tmp.*
 	test -n "$swapfile" && swapoff $swapfile &> /dev/null
 }
+. ./common/filter
 
 # real QA test starts here
 _supported_fs generic
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH v2] common/rc: fix MKSWAP_PROG quoting
  2021-09-01  6:16         ` [PATCH v2] " Murphy Zhou
@ 2021-09-01  7:15           ` Dave Chinner
  2021-09-01  9:40             ` [PATCH v3] " Xiong Zhou
  0 siblings, 1 reply; 9+ messages in thread
From: Dave Chinner @ 2021-09-01  7:15 UTC (permalink / raw)
  To: Murphy Zhou; +Cc: Theodore Ts'o, Pavel Reichl, fstests, Luis Chamberlain

On Wed, Sep 01, 2021 at 02:16:20PM +0800, Murphy Zhou wrote:
> After commit
>   0e4dd8b9 common/rc: fix ignoring of errors on
> we are getting this error message when running swapfiles tests:
>   +./common/rc: line 2553: MKSWAP_PROG: command not found
> 
> Rewrite the line and add a filter for mkswap.
> 
> Signed-off-by: Murphy Zhou <xzhou@redhat.com>
> ---
> 
> Thanks all for the suggestions!
> 
> v2:
>   rewrite the line and add a filter
>   source filters for generic/643
> 
>  common/filter     | 6 ++++++
>  common/rc         | 2 +-
>  tests/generic/643 | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/common/filter b/common/filter
> index 2efbbd99..0883a3fd 100644
> --- a/common/filter
> +++ b/common/filter
> @@ -661,5 +661,11 @@ _filter_quota_report()
>  		s|^(.*?) (\d+) (\d+) (\d+)|$1 @{[$2 * 1024 /'$bsize']} @{[$3 * 1024 /'$bsize']} @{[$4 * 1024 /'$bsize']}|'
>  }
>  
> +# Filter mkswap warning about the permissions
> +_filter_mkswap_stderr()
> +{
> +	grep -v "insecure permission"
> +}
> +

This is a one-off, specific filter and the "stderr" part of the name
makes no sense. If you want this to be a generic filter, the name
is "_filter_insecure_permission()", not it's calling context.

But that just shows that this does not need to be in the common
filter code - doing so just makes readers have to go look up what
the filter is. That is:

>  # make sure this script returns success
>  /bin/true
> diff --git a/common/rc b/common/rc
> index 46b6b220..0a3933a5 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2550,7 +2550,7 @@ _format_swapfile() {
>  	$CHATTR_PROG +C "$fname" > /dev/null 2>&1
>  	_pwrite_byte 0x61 0 "$sz" "$fname" >> $seqres.full
>  	# Ignore permission complaints on filesystems that don't support perms
> -	$(MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full
> +	$MKSWAP_PROG "$fname" 2>&1 >> $seqres.full | _filter_mkswap_stderr

	$MKSWAP_PROG "$fname" 2>&1 >> $seqres.full | \
		grep -v "insecure permission"

will do what you want and is much more readable and easier to
understand.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH v3] common/rc: fix MKSWAP_PROG quoting
  2021-09-01  7:15           ` Dave Chinner
@ 2021-09-01  9:40             ` Xiong Zhou
  2021-09-03 17:48               ` Darrick J. Wong
  0 siblings, 1 reply; 9+ messages in thread
From: Xiong Zhou @ 2021-09-01  9:40 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Murphy Zhou, Theodore Ts'o, Pavel Reichl, fstests, Luis Chamberlain

After commit
  0e4dd8b9 common/rc: fix ignoring of errors on
we are getting this error message when running swapfiles tests:
  +./common/rc: line 2553: MKSWAP_PROG: command not found

Rewrite the line.

Signed-off-by: Murphy Zhou <xzhou@redhat.com>
---

Thanks all for the suggestions!

v2:
  rewrite the line and add a filter
  source filters for generic/643
v3:
  drop the filter, use grep

 common/rc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 46b6b220..00f3023f 100644
--- a/common/rc
+++ b/common/rc
@@ -2550,7 +2550,8 @@ _format_swapfile() {
 	$CHATTR_PROG +C "$fname" > /dev/null 2>&1
 	_pwrite_byte 0x61 0 "$sz" "$fname" >> $seqres.full
 	# Ignore permission complaints on filesystems that don't support perms
-	$(MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full
+	$MKSWAP_PROG "$fname" 2>&1 >> $seqres.full | \
+		grep -v "insecure permission"
 }
 
 _swapon_file() {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH v3] common/rc: fix MKSWAP_PROG quoting
  2021-09-01  9:40             ` [PATCH v3] " Xiong Zhou
@ 2021-09-03 17:48               ` Darrick J. Wong
  0 siblings, 0 replies; 9+ messages in thread
From: Darrick J. Wong @ 2021-09-03 17:48 UTC (permalink / raw)
  To: Xiong Zhou
  Cc: Dave Chinner, Theodore Ts'o, Pavel Reichl, fstests, Luis Chamberlain

On Wed, Sep 01, 2021 at 05:40:24PM +0800, Xiong Zhou wrote:
> After commit
>   0e4dd8b9 common/rc: fix ignoring of errors on
> we are getting this error message when running swapfiles tests:
>   +./common/rc: line 2553: MKSWAP_PROG: command not found
> 
> Rewrite the line.
> 
> Signed-off-by: Murphy Zhou <xzhou@redhat.com>

Aha, I was wondering why the test counts were lower after last Sunday's
rebase.  Works for me, so:

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
> 
> Thanks all for the suggestions!
> 
> v2:
>   rewrite the line and add a filter
>   source filters for generic/643
> v3:
>   drop the filter, use grep
> 
>  common/rc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index 46b6b220..00f3023f 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2550,7 +2550,8 @@ _format_swapfile() {
>  	$CHATTR_PROG +C "$fname" > /dev/null 2>&1
>  	_pwrite_byte 0x61 0 "$sz" "$fname" >> $seqres.full
>  	# Ignore permission complaints on filesystems that don't support perms
> -	$(MKSWAP_PROG "$fname" 2> >(grep -v 'insecure permission' >&2)) >> $seqres.full
> +	$MKSWAP_PROG "$fname" 2>&1 >> $seqres.full | \
> +		grep -v "insecure permission"
>  }
>  
>  _swapon_file() {
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-09-03 17:48 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-31  3:04 [PATCH] common/rc: fix MKSWAP_PROG quoting Murphy Zhou
2021-08-31  5:26 ` Zorro Lang
2021-08-31  7:38   ` Pavel Reichl
2021-08-31 13:42     ` Zorro Lang
2021-08-31 17:05       ` Theodore Ts'o
2021-09-01  6:16         ` [PATCH v2] " Murphy Zhou
2021-09-01  7:15           ` Dave Chinner
2021-09-01  9:40             ` [PATCH v3] " Xiong Zhou
2021-09-03 17:48               ` Darrick J. Wong

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.