Re: [PATCH] selftests: Improves rtctest error handling.

[Alexandre Belloni via Linux-kernel-mentees <linux-kernel-mentees@lists.linuxfoundation.org>]

On 11/04/2023 10:21:15+0530, Atul Kumar Pant wrote:
> On Sat, Apr 08, 2023 at 01:50:15PM +0200, Alexandre Belloni wrote:
> > On 08/04/2023 13:10:59+0530, Atul Kumar Pant wrote:
> > > When running the rtctest without root privileges the test fails
> > > expectedly, but prints the logs that are not useful to point to the issue.
> > > Similarly, if we pass wrong rtc device file as an argument, the test
> > > output failure logs do not point to the issue that the rtc file is
> > > invalid.
> > > To handle these issues, this patch adds checks to verify uid with which
> > > the test is run and also if the rtc_file is valid.
> > > 
> > > Signed-off-by: Atul Kumar Pant <atulpant.linux@gmail.com>
> > > ---
> > >  tools/testing/selftests/rtc/rtctest.c | 34 +++++++++++++++++++--------
> > >  1 file changed, 24 insertions(+), 10 deletions(-)
> > > 
> > 
> > Please run your patch through checkpatch.pl, the indentation and comment
> > style are not correct.
> 
> Sure I'll fix this is next patch.
> 
> > 
> > > diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c
> > > index 2b9d929a24ed..9564346c63eb 100644
> > > --- a/tools/testing/selftests/rtc/rtctest.c
> > > +++ b/tools/testing/selftests/rtc/rtctest.c
> > > @@ -388,16 +388,30 @@ __constructor_order_last(void)
> > >  
> > >  int main(int argc, char **argv)
> > >  {
> > > -	switch (argc) {
> > > -	case 2:
> > > -		rtc_file = argv[1];
> > > -		/* FALLTHROUGH */
> > > -	case 1:
> > > -		break;
> > > -	default:
> > > -		fprintf(stderr, "usage: %s [rtcdev]\n", argv[0]);
> > > -		return 1;
> > > +    int ret = -1;
> > > +
> > > +    // Verify if the test is run as root
> > > +    if (getuid() != 0) {
> > > +		ksft_exit_skip("[ERROR]: Please run the test as root - Exiting.\n");
> > 
> > If this is an error, should we really return skip? This may end up being
> > silently ignored whereas the expectation is to test the rtc.
> > 
> 
> Yes, we should not skip the test. In that case we can use 'ksft_exit_fail_msg'.
> I'll fix this in next patch.
> 
> > Also, you may use the rtc as non root so I guess the access() test
> > should be enough.
> 
> I added this check for the use case when we use the rtc file from /dev/ which will require root
> permissions. Should I keep this check ?

You don't need to be root, you need CAP_SYS_TIME and CAP_SYS_RESOURCE, I
wouldn't check for uid 0.

> 
> > 
> > > +    }
> > > +
> > > +    switch (argc) {
> > > +    case 2:
> > > +        rtc_file = argv[1];
> > > +        /* FALLTHROUGH */
> > > +    case 1:
> > > +        break;
> > > +    default:
> > > +        fprintf(stderr, "usage: %s [rtcdev]\n", argv[0]);
> > > +        return 1;
> > >  	}
> > >  
> > > -	return test_harness_run(argc, argv);
> > > +    // Run the test if rtc_file is valid
> > > +    if (access(rtc_file, F_OK) == 0) {
> > > +	    ret = test_harness_run(argc, argv);
> > > +    } else {
> > > +		ksft_exit_skip("[ERROR]: %s : File does not exists - Exiting\n", rtc_file);
> > 
> > 
> > > +    }
> > > +
> > > +    return ret;
> > >  }
> > > -- 
> > > 2.25.1
> > > 
> > 
> > -- 
> > Alexandre Belloni, co-owner and COO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees