[PATCH v2 0/2] generic/079 test fails if 'nobody' and 'daemon' don't exist

[PATCH v2 0/2] generic/079 test fails if 'nobody' and 'daemon' don't exist

[Luis Henriques]

Hi!

Here's v2 of this humble patchset.  Here's what changed since v1:

- 'user' variable in _require_user_exists() is now local, as suggested
  by Eric Biggers.

(I've also added Darrick's Reviewed-by tag to the 2nd patch only.)

Original cover-letter:

Test generic/079 currently fails if for some reason the 'nobody' or
'daemon' users don't exist:

*** starting up
add_acl(/mnt/scratch/079/immutable.f) did not set errno == EPERM
add_acl(/mnt/scratch/079/immutable.d) did not set errno == EPERM
add_acl(/mnt/scratch/079/append-only.f) did not set errno == EPERM
add_acl(/mnt/scratch/079/append-only.d) did not set errno == EPERM
testing immutable...FAILED! (2 tests failed)
testing append-only...FAILED! (2 tests failed)
*** cleaning up

The first patch that follows adds a new function _require_user_exists()
that will verify a user exists in the system (and refactors existent
_require_user() to use the new function).

The second patch is the actual test fix.

Luis Henriques (2):
  common/rc: add _require_user_exists() to check if a user exists
  generic/079: make sure users 'nobody' and 'daemon' exist

 common/rc         | 27 ++++++++++++++++++---------
 tests/generic/079 |  2 ++
 2 files changed, 20 insertions(+), 9 deletions(-)

[PATCH v2 1/2] common/rc: add _require_user_exists() to check if a user exists

[Luis Henriques]

Function _require_user() does check if a user exists *and* if it is able
to execute commands.  Add a new function to simply check if a user exists.

Signed-off-by: Luis Henriques <lhenriques@suse.de>
---
 common/rc | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/common/rc b/common/rc
index 154bc2dd7e94..de9ba56eefcf 100644
--- a/common/rc
+++ b/common/rc
@@ -2289,18 +2289,27 @@ _cat_group()
 	cat /etc/group
 }
 
-# check for a user on the machine, fsgqa as default
+# check if a user exists in the system
+#
+_require_user_exists()
+{
+	local user=$1
+	_cat_passwd | grep -q $user
+	[ "$?" == "0" ] || _notrun "$user user not defined."
+}
+
+# check if a user exists and is able to execute commands.
+# Uses 'fsgqa' user as default.
 #
 _require_user()
 {
-    qa_user=fsgqa
-    if [ -n "$1" ];then
-        qa_user=$1
-    fi
-    _cat_passwd | grep -q $qa_user
-    [ "$?" == "0" ] || _notrun "$qa_user user not defined."
-    echo /bin/true | su $qa_user
-    [ "$?" == "0" ] || _notrun "$qa_user cannot execute commands."
+	qa_user=fsgqa
+	if [ -n "$1" ];then
+		qa_user=$1
+	fi
+	_require_user_exists $qa_user
+	echo /bin/true | su $qa_user
+	[ "$?" == "0" ] || _notrun "$qa_user cannot execute commands."
 }
 
 # check for a chown support

Re: [PATCH v2 1/2] common/rc: add _require_user_exists() to check if a user exists

[Zorro Lang]

On Fri, Sep 24, 2021 at 10:19:10AM +0100, Luis Henriques wrote:
> Function _require_user() does check if a user exists *and* if it is able
> to execute commands.  Add a new function to simply check if a user exists.
> 
> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> ---
>  common/rc | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 154bc2dd7e94..de9ba56eefcf 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2289,18 +2289,27 @@ _cat_group()
>  	cat /etc/group
>  }
>  
> -# check for a user on the machine, fsgqa as default
> +# check if a user exists in the system
> +#
> +_require_user_exists()
> +{
> +	local user=$1
> +	_cat_passwd | grep -q $user
> +	[ "$?" == "0" ] || _notrun "$user user not defined."

As _require_user() does "su $qa_user" after the "grep", so it really make sure
there's an "user". But if the _require_user_exists() only trys to grep /etc/passed
to make sure there's an "$user", I'd like to make the "grep" condition be more exact.
For example, if there's an user "myuser100" in the /etc/passwd, then _require_user_exists
"myuser" or "user100" or "user1" or "user10" all return 0.

So how about:
  _cat_passwd | grep -qw $user

Or more exact:
  _cat_passwd | cut -d: -f1 | grep -qw $user

Or other better command line:)

Thanks,
Zorro

> +}
> +
> +# check if a user exists and is able to execute commands.
> +# Uses 'fsgqa' user as default.
>  #
>  _require_user()
>  {
> -    qa_user=fsgqa
> -    if [ -n "$1" ];then
> -        qa_user=$1
> -    fi
> -    _cat_passwd | grep -q $qa_user
> -    [ "$?" == "0" ] || _notrun "$qa_user user not defined."
> -    echo /bin/true | su $qa_user
> -    [ "$?" == "0" ] || _notrun "$qa_user cannot execute commands."
> +	qa_user=fsgqa
> +	if [ -n "$1" ];then
> +		qa_user=$1
> +	fi
> +	_require_user_exists $qa_user
> +	echo /bin/true | su $qa_user
> +	[ "$?" == "0" ] || _notrun "$qa_user cannot execute commands."
>  }
>  
>  # check for a chown support
> 

Re: [PATCH v2 1/2] common/rc: add _require_user_exists() to check if a user exists

[Zorro Lang]

On Sun, Sep 26, 2021 at 06:13:54PM +0800, Zorro Lang wrote:
> On Fri, Sep 24, 2021 at 10:19:10AM +0100, Luis Henriques wrote:
> > Function _require_user() does check if a user exists *and* if it is able
> > to execute commands.  Add a new function to simply check if a user exists.
> > 
> > Signed-off-by: Luis Henriques <lhenriques@suse.de>
> > ---
> >  common/rc | 27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index 154bc2dd7e94..de9ba56eefcf 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -2289,18 +2289,27 @@ _cat_group()
> >  	cat /etc/group
> >  }
> >  
> > -# check for a user on the machine, fsgqa as default
> > +# check if a user exists in the system
> > +#
> > +_require_user_exists()
> > +{
> > +	local user=$1
> > +	_cat_passwd | grep -q $user
> > +	[ "$?" == "0" ] || _notrun "$user user not defined."
> 
> As _require_user() does "su $qa_user" after the "grep", so it really make sure
> there's an "user". But if the _require_user_exists() only trys to grep /etc/passed
> to make sure there's an "$user", I'd like to make the "grep" condition be more exact.
> For example, if there's an user "myuser100" in the /etc/passwd, then _require_user_exists
> "myuser" or "user100" or "user1" or "user10" all return 0.
> 
> So how about:
>   _cat_passwd | grep -qw $user
> 
> Or more exact:
>   _cat_passwd | cut -d: -f1 | grep -qw $user
> 
> Or other better command line:)

Oh, the "-w" doesn't work with an user with "_", likes myuser_123. So
how about:

  _cat_passwd | grep -q ^$user:

Thanks,
Zorro

> 
> Thanks,
> Zorro
> 
> > +}
> > +
> > +# check if a user exists and is able to execute commands.
> > +# Uses 'fsgqa' user as default.
> >  #
> >  _require_user()
> >  {
> > -    qa_user=fsgqa
> > -    if [ -n "$1" ];then
> > -        qa_user=$1
> > -    fi
> > -    _cat_passwd | grep -q $qa_user
> > -    [ "$?" == "0" ] || _notrun "$qa_user user not defined."
> > -    echo /bin/true | su $qa_user
> > -    [ "$?" == "0" ] || _notrun "$qa_user cannot execute commands."
> > +	qa_user=fsgqa
> > +	if [ -n "$1" ];then
> > +		qa_user=$1
> > +	fi
> > +	_require_user_exists $qa_user
> > +	echo /bin/true | su $qa_user
> > +	[ "$?" == "0" ] || _notrun "$qa_user cannot execute commands."
> >  }
> >  
> >  # check for a chown support
> > 

Re: [PATCH v2 1/2] common/rc: add _require_user_exists() to check if a user exists

[Luis Henriques]

On Sun, Sep 26, 2021 at 06:22:17PM +0800, Zorro Lang wrote:
> On Sun, Sep 26, 2021 at 06:13:54PM +0800, Zorro Lang wrote:
> > On Fri, Sep 24, 2021 at 10:19:10AM +0100, Luis Henriques wrote:
> > > Function _require_user() does check if a user exists *and* if it is able
> > > to execute commands.  Add a new function to simply check if a user exists.
> > > 
> > > Signed-off-by: Luis Henriques <lhenriques@suse.de>
> > > ---
> > >  common/rc | 27 ++++++++++++++++++---------
> > >  1 file changed, 18 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index 154bc2dd7e94..de9ba56eefcf 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -2289,18 +2289,27 @@ _cat_group()
> > >  	cat /etc/group
> > >  }
> > >  
> > > -# check for a user on the machine, fsgqa as default
> > > +# check if a user exists in the system
> > > +#
> > > +_require_user_exists()
> > > +{
> > > +	local user=$1
> > > +	_cat_passwd | grep -q $user
> > > +	[ "$?" == "0" ] || _notrun "$user user not defined."
> > 
> > As _require_user() does "su $qa_user" after the "grep", so it really make sure
> > there's an "user". But if the _require_user_exists() only trys to grep /etc/passed
> > to make sure there's an "$user", I'd like to make the "grep" condition be more exact.
> > For example, if there's an user "myuser100" in the /etc/passwd, then _require_user_exists
> > "myuser" or "user100" or "user1" or "user10" all return 0.
> > 
> > So how about:
> >   _cat_passwd | grep -qw $user
> > 
> > Or more exact:
> >   _cat_passwd | cut -d: -f1 | grep -qw $user
> > 
> > Or other better command line:)
> 
> Oh, the "-w" doesn't work with an user with "_", likes myuser_123. So
> how about:
> 
>   _cat_passwd | grep -q ^$user:

Yep, that seems to make sense.  I'll prepare v3 and send it out soon.

Cheers,
--
Luís

[PATCH v2 2/2] generic/079: make sure users 'nobody' and 'daemon' exist

[Luis Henriques]

Test 'src/t_immutable' assumes the existence of users 'nobody' and
'daemon' on the system.  If any of these users don't exist, the test will
fail with:

  add_acl(/media/scratch/079/immutable.f) did not set errno == EPERM
  ...

because it tries to set ACLs such as:

  u::rwx,g::rwx,o::rwx,u:daemon:rwx,m::rwx

Use the new _require_user_exists() function to ensure these users exist.

Signed-off-by: Luis Henriques <lhenriques@suse.de>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/generic/079 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/generic/079 b/tests/generic/079
index 829275db8cdf..9e7ccd31c8e2 100755
--- a/tests/generic/079
+++ b/tests/generic/079
@@ -26,6 +26,8 @@ _cleanup()
 _supported_fs generic
 
 _require_chattr ia
+_require_user_exists "nobody"
+_require_user_exists "daemon"
 _require_test_program "t_immutable"
 _require_scratch