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

[Atul Kumar Pant <atulpant.linux@gmail.com>]

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 ?

> 
> > +    }
> > +
> > +    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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees