[1/3] login/tst-grantpt: Convert to support framework, more error checking

  The test now requires working /dev/pts pseudo-terminals.

A new subtest (test_not_ptmx) attempts to call grantpt on a
pseudo-terminal that is not a ptmx device.  POSIX requires an EINVAL
error in this case.
---
 login/tst-grantpt.c | 93 ++++++++++++++++++++++++++++-----------------
 1 file changed, 58 insertions(+), 35 deletions(-)
  

On 05/08/2020 04:14, Florian Weimer via Libc-alpha wrote:
> The test now requires working /dev/pts pseudo-terminals.
> 
> A new subtest (test_not_ptmx) attempts to call grantpt on a
> pseudo-terminal that is not a ptmx device.  POSIX requires an EINVAL
> error in this case.

LGTM thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  login/tst-grantpt.c | 93 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 58 insertions(+), 35 deletions(-)
> 
> diff --git a/login/tst-grantpt.c b/login/tst-grantpt.c
> index 65bb344909..1d7a220fcf 100644
> --- a/login/tst-grantpt.c
> +++ b/login/tst-grantpt.c
> @@ -1,3 +1,21 @@
> +/* Test for grantpt error corner cases.
> +   Copyright (C) 2001-2020 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <fcntl.h>
> @@ -6,76 +24,81 @@
>  #include <errno.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include <support/check.h>
> +#include <support/temp_file.h>
> +#include <support/xunistd.h>
>  
> -static int
> +/* Test grantpt with a closed descriptor.  */
> +static void
>  test_ebadf (void)
>  {
>    int fd, ret, err;
>  
>    fd = posix_openpt (O_RDWR);
>    if (fd == -1)
> -    {
> -      printf ("posix_openpt(O_RDWR) failed\nerrno %d (%s)\n",
> -	      errno, strerror (errno));
> -      /* We don't fail because of this; maybe the system does not have
> -	 SUS pseudo terminals.  */
> -      return 0;
> -    }
> -  unlockpt (fd);
> -  close (fd);
> +    FAIL_EXIT1 ("posix_openpt(O_RDWR) failed\nerrno %d (%m)\n", errno);
> +  TEST_COMPARE (unlockpt (fd), 0);
>  

Ok.

> +  xclose (fd);
>    ret = grantpt (fd);
>    err = errno;
>    if (ret != -1 || err != EBADF)
>      {
> +      support_record_failure ();
>        printf ("grantpt(): expected: return = %d, errno = %d\n", -1, EBADF);
>        printf ("           got: return = %d, errno = %d\n", ret, err);
> -      return 1;
>      }
> -  return 0;
>  }

Ok (although maybe use CHECK_VERIFY?).

>  
> -static int
> +/* Test grantpt on a regular file.  */
> +static void
>  test_einval (void)
>  {
>    int fd, ret, err;
> -  const char file[] = "./grantpt-einval";
>  
> -  fd = open (file, O_RDWR | O_CREAT, 0600);
> -  if (fd == -1)
> -    {
> -      printf ("open(\"%s\", O_RDWR) failed\nerrno %d (%s)\n",
> -	      file, errno, strerror (errno));
> -      return 0;
> -    }
> -  unlink (file);
> +  fd = create_temp_file ("tst-grantpt-", NULL);
> +  TEST_VERIFY_EXIT (fd >= 0);
>  

Ok.  As a side note, maybe we should add a xcreate_temp_file.

>    ret = grantpt (fd);
>    err = errno;
>    if (ret != -1 || err != EINVAL)
>      {
> +      support_record_failure ();
>        printf ("grantpt(): expected: return = %d, errno = %d\n", -1, EINVAL);
>        printf ("           got: return = %d, errno = %d\n", ret, err);
> -      ret = 1;
>      }
> -  else
> -    ret = 0;
>  
> -  close (fd);
> +  xclose (fd);
> +}

Ok (although same previous suggestion applies here suggestion).

> +
> +/* Test grantpt on a non-ptmx pseudo-terminal.  */
> +static void
> +test_not_ptmx (void)
> +{
> +  int ptmx = posix_openpt (O_RDWR);
> +  TEST_VERIFY_EXIT (ptmx >= 0);
> +  TEST_COMPARE (grantpt (ptmx), 0);
> +  TEST_COMPARE (unlockpt (ptmx), 0);
> +
> +  const char *name = ptsname (ptmx);
> +  TEST_VERIFY_EXIT (name != NULL);
> +  int pts = open (name, O_RDWR | O_NOCTTY);
> +  TEST_VERIFY_EXIT (pts >= 0);
> +
> +  TEST_COMPARE (grantpt (pts), -1);
> +  TEST_COMPARE (errno, EINVAL);
>  
> -  return ret;
> +  xclose (pts);
> +  xclose (ptmx);
>  }
>  

Ok.

>  static int
>  do_test (void)
>  {
> -  int result = 0;
> -
> -  result += test_ebadf ();
> -  result += test_einval ();
> -
> -  return result;
> +  test_ebadf ();
> +  test_einval ();
> +  test_not_ptmx ();
> +  return 0;
>  }
>  
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> +#include <support/test-driver.c>
> 

Ok.
  

* Adhemerval Zanella via Libc-alpha:

> > +  xclose (fd);
> >    ret = grantpt (fd);
> >    err = errno;
> >    if (ret != -1 || err != EBADF)
> >      {
> > +      support_record_failure ();
> >        printf ("grantpt(): expected: return = %d, errno = %d\n", -1, EBADF);
> >        printf ("           got: return = %d, errno = %d\n", ret, err);
> > -      return 1;
> >      }
> > -  return 0;
> >  }
>
> Ok (although maybe use CHECK_VERIFY?).

Do you mean TEST_VERIFY?  That can't produce a log message.  We
probably should have something that logs a failure unconditionally.

>> -  unlink (file);
>> +  fd = create_temp_file ("tst-grantpt-", NULL);
>> +  TEST_VERIFY_EXIT (fd >= 0);
>>  
>
> Ok.  As a side note, maybe we should add a xcreate_temp_file.

I think we should change the semantics of the existing function
instead.
  

On 13/08/2020 15:31, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>>> +  xclose (fd);
>>>    ret = grantpt (fd);
>>>    err = errno;
>>>    if (ret != -1 || err != EBADF)
>>>      {
>>> +      support_record_failure ();
>>>        printf ("grantpt(): expected: return = %d, errno = %d\n", -1, EBADF);
>>>        printf ("           got: return = %d, errno = %d\n", ret, err);
>>> -      return 1;
>>>      }
>>> -  return 0;
>>>  }
>>
>> Ok (although maybe use CHECK_VERIFY?).
> 
> Do you mean TEST_VERIFY?  That can't produce a log message.  We
> probably should have something that logs a failure unconditionally.
> 

Yeap, TEST_VERIFY.  The test failure output will have all the required
information anyway (either if return code or errno does not match
expectations).

>>> -  unlink (file);
>>> +  fd = create_temp_file ("tst-grantpt-", NULL);
>>> +  TEST_VERIFY_EXIT (fd >= 0);
>>>  
>>
>> Ok.  As a side note, maybe we should add a xcreate_temp_file.
> 
> I think we should change the semantics of the existing function
> instead.
> 

The posix/tst-spawn3.c uses create_temp_file to fill all possible file-descriptor
to check if posix_spawn file actions does not use extra descriptors.  Afaik it
is a exception, all other tests expect that create_temp_file does not fail.