Re: [PATCH] selftests: capabilities: convert error output to TAP13 ksft framework

[Shuah Khan]

Hi Andy,

On 07/28/2017 03:39 PM, Shuah Khan wrote:
> On 07/28/2017 09:41 AM, Shuah Khan wrote:
>> On 07/27/2017 08:13 PM, Andy Lutomirski wrote:
>>> On Thu, Jul 27, 2017 at 1:32 PM, Shuah Khan <shuah@xxxxxxxxxx> wrote:
>>>> On 07/27/2017 12:50 PM, Andy Lutomirski wrote:
>>>>> On Wed, Jul 26, 2017 at 2:18 PM, Shuah Khan <shuahkh@xxxxxxxxxxxxxxx> wrote:
>>>>>> Convert errx() and err() usage to appropriate TAP13 ksft API.
>>>>>>
>>>>>> Signed-off-by: Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>
>>>>>> ---
>>>>>>  tools/testing/selftests/capabilities/test_execve.c | 105 ++++++++++++---------
>>>>>>  .../testing/selftests/capabilities/validate_cap.c  |   9 +-
>>>>>>  2 files changed, 65 insertions(+), 49 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/testing/selftests/capabilities/test_execve.c b/tools/testing/selftests/capabilities/test_execve.c
>>>>>> index 7c38233292b0..cf6778441381 100644
>>>>>> --- a/tools/testing/selftests/capabilities/test_execve.c
>>>>>> +++ b/tools/testing/selftests/capabilities/test_execve.c
>>>>>> @@ -1,7 +1,6 @@
>>>>>>  #define _GNU_SOURCE
>>>>>>
>>>>>>  #include <cap-ng.h>
>>>>>> -#include <err.h>
>>>>>>  #include <linux/capability.h>
>>>>>>  #include <stdbool.h>
>>>>>>  #include <string.h>
>>>>>> @@ -39,29 +38,32 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list
>>>>>>         int buf_len;
>>>>>>
>>>>>>         buf_len = vsnprintf(buf, sizeof(buf), fmt, ap);
>>>>>> -       if (buf_len < 0) {
>>>>>> -               err(1, "vsnprintf failed");
>>>>>> -       }
>>>>>> -       if (buf_len >= sizeof(buf)) {
>>>>>> -               errx(1, "vsnprintf output truncated");
>>>>>> -       }
>>>>>> +       if (buf_len < 0)
>>>>>> +               ksft_exit_fail_msg("vsnprintf failed - %s\n", strerror(errno));
>>>>>
>>>>> Could this not be a hypothetical ksft_exit_fail_msg_err or similar?
>>>>> Or a shorter name like ksft_fatal_err()?
>>>>>
>>>>>
>>>>
>>>> Is there a reason to add _err() suffix?
>>>>
>>>> ksft_exit_fail_msg() is a generic routine for a test to exit
>>>> with a test failure and print a message. The message doesn't
>>>> necessarily need to be a standard error message such as the
>>>> one err() or errx() or strerror() generate.
>>>>
>>>> In some cases test could fail with a standard error condition,
>>>> but not always. In that context, it doesn't make sense to add
>>>> _err suffix. I leveraged this generic function to replace err()
>>>> and errx() usages adding strerror() not loose the important
>>>> information.
>>>
>>> The idea behind the _err version is to avoid the extra typing to
>>> report errno.  I suppose you could always report errno, but there are
>>> contexts where errno is meaningless or garbage.
>>>>>
>>
>> Thinking about this some more, using strerror() as a replacements does
>> drop some information compared to what _err() and _errx() provide.
>>
>> capabilities is the first test I came across that uses err() and errx()
>> heavily. I am sure there are more that do that. It might be useful to
>> provide a equivalent ksft_ framework replacement. I will work on it.
>>
> 
> Okay. More on this. Both err() and errx() will not return. So, if we were
> to preserve the usage either not replacing with hypothetical ksft_err() and
> ksft_errx() will result in the error message being the last message of the
> test output after the test results summary. Example:
> 
> Say the test ran into a failure that uses err() or errx() in the middle:
> 
> If err() and errx() aren't replaced:
> 
> Test will end without printing results summary:
> "error message from err() or errx()"
> 
> 
> If err() and errx() are replaced:
> 
> not ok 9 failed
> Pass 9 Fail 1 Xfail 0 Xpass 0 Skip 0
> 1..9
> "error message from err() or errx()"
> 
> Not replacing err() and errx() isn't good. Replacing addresses the
> issue of printing counts, but test ends with an error message. This
> isn't great from TAP13 compliance and it is lot cleaner to see:
> 
> "error message from err() or errx()"
> not ok 9 failed
> Pass 9 Fail 1 Xfail 0 Xpass 0 Skip 0
> 1..9
> 
> In the interest if presenting cleaner and easier to understand output,
> we might have to opt for using strerror(errno) assuming that in the
> cases when err() and errx() are in use, errno will be valid.
> 

Are you okay with this change? If you think it is very important to
have the information, there is an option to add ksft versions for
err() and errx() that do everything err() and errx() do except not
exit. There is a downside to this that we have to maintain these.

Please let me know your preference.

thanks,
-- Shuah