Re: [PATCH i-g-t v2 04/11] lib/kunit: Parse KTAP report from the main process thread

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Janusz,
On 2023-10-10 at 19:49:35 +0200, Janusz Krzysztofik wrote:
> Hi Kamil,
> 
> Thanks for review.
> 
> On Tuesday, 10 October 2023 17:59:56 CEST Kamil Konieczny wrote:
> > Hi Janusz,
> > On 2023-10-09 at 14:27:55 +0200, Janusz Krzysztofik wrote:
> > > There was an attempt to parse KTAP reports in the background while a kunit
> > > test module is loading.  However, since dynamic sub-subtests can be
> > > executed only from the main thread, that attempt was not quite successful,
> > > as IGT results from all executed kunit test cases were generated only
> > > after loading of kunit test module completed.
> > > 
> > > Now that the parser maintains its state and we can call it separately for
> > > each input line of a KTAP report, it is perfectly possible to call the
> > > parser from the main thread while the module is loading in the background,
> > > and convert results from kunit test cases immediately to results of IGT
> > > dynamic sub-subtests by running an igt_dynamic() section for each result
> > > as soon as returned by the parser.
> > > 
> > > Drop igt_ktap_parser() thread and execute igt_dynamic() for each kunit
> > > result obtained from igt_ktap_parse() called from the main thread.
> > > 
> > > Also, drop no longer needed functions from igt_ktap soruces.
> > > 
> > > v3: Fix ktap structure not freed on lseek error,
> > >   - fix initial SIGCHLD handler not restored,
> > >   - fix missing handling of potential errors returned by sigaction,
> > >   - fix potential race of read() vs. ptherad_kill(), use robust mutex for
> > >     synchronization with modprobe thread,
> > >   - fix potentially illegal use of igt_assert() called outside of
> > >     dynamic sub-subtest section,
> > >   - fix unsupported exit code potentially passed to igt_fail(),
> > >   - no need to fail a dynamic sub-subtest on potential KTAP parser error
> > >     after a valid result from the parser has been processed,
> > >   - fix trailing newlines missing from error messages,
> > >   - add more debug statements,
> > >   - integrate common code around kunit_result_free() into it.
> > > v2: Interrupt blocking read() on modprobe failure.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxxxxxxxx>
> > > Acked-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> # v2
> > > ---
> > >  lib/igt_kmod.c | 261 +++++++++++++++++++----
> > >  lib/igt_ktap.c | 568 -------------------------------------------------
> > >  lib/igt_ktap.h |  22 --
> > >  3 files changed, 222 insertions(+), 629 deletions(-)
> > > 
> > > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > > index 426ae5b26f..7bca4cdaab 100644
> > > --- a/lib/igt_kmod.c
> > > +++ b/lib/igt_kmod.c
> > > @@ -1,5 +1,5 @@
> > >  /*
> > > - * Copyright © 2016 Intel Corporation
> > > + * Copyright © 2016-2023 Intel Corporation
> > >   *
> > >   * Permission is hereby granted, free of charge, to any person obtaining a
> > >   * copy of this software and associated documentation files (the "Software"),
> > > @@ -26,7 +26,12 @@
> > >  #include <errno.h>
> > >  #include <fcntl.h>
> > >  #include <pthread.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > >  #include <sys/utsname.h>
> > > +#include <unistd.h>
> > > +
> > > +#include "assembler/brw_compat.h"	/* [un]likely() */
> > ------------ ^^^^^^^^^
> > Do we really need this?
> 
> I think the correct question is if wee really need [un]likely().  I'm using it 
> to document unlikely cases, which is a widely accepted method of documenting 
> cases like that, I believe.  Having that clarified, I hope you just tell me if 
> you think we need those cases documented, and how, if not that way.
> 

Now I see, it is for unlikely() - ok it can stay.

> > 
> > >  
> > >  #include "igt_aux.h"
> > >  #include "igt_core.h"
> > > @@ -748,9 +753,12 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
> > >  }
> > >  
> > >  struct modprobe_data {
> > > +	pthread_t parent;
> > --- ^^^^^^^^^^^^^^^^
> > Please move it below to other related thread data.
> > Also consider a comment why(or for what purpose)
> > did you put this here.
> 
> No problem to move it to the bottom, if that's important to you, but regarding 
> the comment, do you think that the purpose of this field of the structure, 
> compared to other fields, is so unclear form review of the code, despite its 
> name, that it requires a comment, unlike the other fields?
> 

I am ok with only move.

> > 
> > >  	struct kmod_module *kmod;
> > >  	const char *opts;
> > >  	int err;
> > > +	pthread_mutex_t lock;
> > > +	pthread_t thread;
> > >  };
> > >  
> > >  static void *modprobe_task(void *arg)
> > > @@ -759,16 +767,132 @@ static void *modprobe_task(void *arg)
> > >  
> > >  	data->err = modprobe(data->kmod, data->opts);
> > >  
> > > +	if (igt_debug_on(data->err)) {
> > > +		int err;
> > > +
> > > +		while (err = pthread_mutex_trylock(&data->lock),
> > > +		       err && !igt_debug_on(err != EBUSY))
> > > +			igt_debug_on(pthread_kill(data->parent, SIGCHLD));
> > > +	} else {
> > > +		/* let main thread use mutex to detect modprobe completion */
> > > +		igt_debug_on(pthread_mutex_lock(&data->lock));
> > > +	}
> > > +
> > >  	return NULL;
> > >  }
> > >  
> > > +static void kunit_sigchld_handler(int signal)
> > > +{
> > > +	return;
> > --- ^^^^^^^
> > Why not removing this? checkpatch complains about return from void.
> 
> OK.
> 
> > 
> > > +}
> > > +
> > > +static int kunit_kmsg_result_get(struct igt_list_head *results,
> > > +				 struct modprobe_data *modprobe,
> > > +				 int fd, struct igt_ktap_results *ktap)
> > > +{
> > > +	struct sigaction sigchld = { .sa_handler = kunit_sigchld_handler, },
> > > +			 *saved;
> > > +	char record[BUF_LEN + 1], *buf;
> > > +	unsigned long taints;
> > > +	int ret;
> > > +
> > > +	do {
> > > +		int err;
> > > +
> > > +		if (igt_debug_on(igt_kernel_tainted(&taints)))
> > > +			return -ENOTRECOVERABLE;
> > > +
> > > +		err = igt_debug_on(sigaction(SIGCHLD, &sigchld, saved));
> > > +		if (err == -1)
> > > +			return -errno;
> > > +		else if (unlikely(err))
> > > +			return err;
> > > +
> > > +		err = pthread_mutex_lock(&modprobe->lock);
> > > +		switch (err) {
> > > +		case EOWNERDEAD:
> > > +			/* leave the mutex unrecoverable */
> > > +			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
> > > +			__attribute__ ((fallthrough));
> > > +		case ENOTRECOVERABLE:
> > > +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> > > +			if (igt_debug_on(modprobe->err))
> > > +				return modprobe->err;
> > 
> > Why no 'return -err;' here?
> 
> Because modprobe->err, which can tell us more about what has gone bad in the 
> modprobe thread, is more important here than err which only informs us about 
> completion of the modprobe thread, also if successful, isn't it?
> 

Thank you for explanation.

> > 
> > > +			break;
> > > +		case 0:
> > > +			break;
> > > +		default:
> > > +			igt_debug("pthread_mutex_lock() error: %d\n", err);
> > > +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> > > +			return -err;
> > > +		}
> > > +
> > > +		ret = read(fd, record, BUF_LEN);
> > > +
> > > +		if (!err) {
> >         ^^^^^^^^^^^
> > Looks strange here.
> 
> Here err still carries a return code from pthread_mutex_lock(), then !err 
> means that the modprobe thread was still running and we have 1) acquired 
> the mutex which now we have to release, 2) installed a non-default 
> SIGCHLD handler that we should now reset.  Have you got a better idea 
> how to encode that so it looks less strange?  Would a comment that reminds 
> a reader where that err comes from be sufficient?
> 

Yes please add comment here.

> > 
> > > +			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
> > > +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> > > +		}
> > > +
> > > +		if (igt_debug_on(!ret))
> > > +			return -ENODATA;
> > > +		if (igt_debug_on(ret == -1))
> > > +			return -errno;
> > > +		if (unlikely(igt_debug_on(ret < 0)))
> > > +			break;
> > > +
> > > +		/* skip kmsg continuation lines */
> > > +		if (igt_debug_on(*record == ' '))
> > ----------- ^^^^^^^^^^^^
> > Why debug here? imho it is not needed.
> 
> Because we don't expect continuation lines while reading a KTAP report. Since 
> the kernel log may still contain them, we want to know about them when 
> something wrong happens to our KTAP parser.  That's the purpose of all those 
> debug_on() calls: provide as much information on unexpected conditions as 
> possible when the test fails, isn't it?
> 

Ok.

> > 
> > > +			continue;
> > > +
> > > +		/* NULL-terminate the record */
> > > +		record[ret] = '\0';
> > > +
> > > +		/* detect start of log message, continue if not found */
> > > +		buf = strchrnul(record, ';');
> > > +		if (igt_debug_on(*buf == '\0'))
> > ----------- ^^^^^^^^^^^^
> > Same here, you just continue loop if no ';'?
> 
> That means we are getting incorrectly formatted records from /dev/kmsg, and 
> I'm sure we would like to be informed about that if something goes wrong with 
> the test, wouldn't we?
> 

Ok.

> > 
> > > +			continue;
> > > +		buf++;
> > > +
> > > +		ret = igt_ktap_parse(buf, ktap);
> > > +		if (!ret || igt_debug_on(ret != -EINPROGRESS))
> > > +			break;
> > > +	} while (igt_list_empty(results));
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static void kunit_result_free(struct igt_ktap_result **r,
> > > +			      char **suite_name, char **case_name)
> > > +{
> > > +	if (!*r)
> > > +		return;
> > > +
> > > +	igt_list_del(&(*r)->link);
> > > +
> > > +	if ((*r)->suite_name != *suite_name) {
> > > +		free(*suite_name);
> > > +		*suite_name = (*r)->suite_name;
> > > +	}
> > > +
> > > +	if ((*r)->case_name != *case_name) {
> > > +		free(*case_name);
> > > +		*case_name = (*r)->case_name;
> > > +	}
> > > +
> > > +	free((*r)->msg);
> > > +	free(*r);
> > > +	*r = NULL;
> > > +}
> > > +
> > >  static void __igt_kunit(struct igt_ktest *tst, const char *opts)
> > >  {
> > > -	struct modprobe_data modprobe = { tst->kmod, opts, 0, };
> > > -	struct kmod_module *kunit_kmod;
> > > -	bool is_builtin;
> > > -	struct ktap_test_results *results;
> > > -	pthread_t modprobe_thread;
> > > +	struct modprobe_data modprobe = { pthread_self(), tst->kmod, opts, 0, };
> > > +	char *suite_name = NULL, *case_name = NULL;
> > > +	struct igt_ktap_result *r, *rn;
> > > +	struct igt_ktap_results *ktap;
> > > +	pthread_mutexattr_t attr;
> > > +	IGT_LIST_HEAD(results);
> > >  	unsigned long taints;
> > >  	int flags, ret;
> > >  
> > > @@ -780,60 +904,119 @@ static void __igt_kunit(struct igt_ktest *tst, const char *opts)
> > >  
> > >  	igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0);
> > >  
> > > -	igt_skip_on(kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod));
> > > -	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
> > > -	kmod_module_unref(kunit_kmod);
> > > +	igt_skip_on(pthread_mutexattr_init(&attr));
> > > +	igt_skip_on(pthread_mutexattr_setrobust(&attr, PTHREAD_MUTEX_ROBUST));
> > > +	igt_skip_on(pthread_mutex_init(&modprobe.lock, &attr));
> > >  
> > > -	results = ktap_parser_start(tst->kmsg, is_builtin);
> > > +	ktap = igt_ktap_alloc(&results);
> > > +	igt_require(ktap);
> > >  
> > > -	if (igt_debug_on(pthread_create(&modprobe_thread, NULL,
> > > +	if (igt_debug_on(pthread_create(&modprobe.thread, NULL,
> > >  					modprobe_task, &modprobe))) {
> > > -		ktap_parser_cancel();
> > > -		igt_ignore_warn(ktap_parser_stop());
> > > +		igt_ktap_free(ktap);
> > >  		igt_skip("Failed to create a modprobe thread\n");
> > >  	}
> > >  
> > > -	while (READ_ONCE(results->still_running) || !igt_list_empty(&results->list))
> > > -	{
> > > -		struct ktap_test_results_element *result;
> > > -
> > > -		if (!pthread_tryjoin_np(modprobe_thread, NULL) && modprobe.err) {
> > > -			ktap_parser_cancel();
> > > +	do {
> > > +		ret = kunit_kmsg_result_get(&results, &modprobe,
> > > +					    tst->kmsg, ktap);
> > > +		if (igt_debug_on(ret && ret != -EINPROGRESS))
> > >  			break;
> > > -		}
> > >  
> > > -		if (igt_kernel_tainted(&taints)) {
> > > -			ktap_parser_cancel();
> > > -			pthread_cancel(modprobe_thread);
> > > +		if (igt_debug_on(igt_list_empty(&results)))
> > >  			break;
> > > -		}
> > >  
> > > -		pthread_mutex_lock(&results->mutex);
> > > -		if (igt_list_empty(&results->list)) {
> > > -			pthread_mutex_unlock(&results->mutex);
> > > -			continue;
> > > -		}
> > > +		r = igt_list_first_entry(&results, r, link);
> > >  
> > > -		result = igt_list_first_entry(&results->list, result, link);
> > > +		igt_dynamic_f("%s-%s", r->suite_name, r->case_name) {
> > > +			if (r->code == IGT_EXIT_INVALID) {
> > > +				/* parametrized test case, get actual result */
> > > +				kunit_result_free(&r, &suite_name, &case_name);
> > >  
> > > -		igt_list_del(&result->link);
> > > -		pthread_mutex_unlock(&results->mutex);
> > > +				igt_assert(igt_list_empty(&results));
> > >  
> > > -		igt_dynamic(result->test_name) {
> > > -			igt_assert(READ_ONCE(result->passed));
> > > +				ret = kunit_kmsg_result_get(&results, &modprobe,
> > > +							    tst->kmsg, ktap);
> > > +				if (ret != -EINPROGRESS)
> > > +					igt_fail_on(ret);
> > > +
> > > +				igt_fail_on(igt_list_empty(&results));
> > > +
> > > +				r = igt_list_first_entry(&results, r, link);
> > > +
> > > +				igt_fail_on_f(strcmp(r->suite_name, suite_name),
> > > +					      "suite_name expected: %s, got: %s\n",
> > > +					      suite_name, r->suite_name);
> > > +				igt_fail_on_f(strcmp(r->case_name, case_name),
> > > +					      "case_name expected: %s, got: %s\n",
> > > +					      case_name, r->case_name);
> > > +			}
> > >  
> > > -			if (!pthread_tryjoin_np(modprobe_thread, NULL))
> > > +			igt_assert_neq(r->code, IGT_EXIT_INVALID);
> > > +
> > > +			if (r->msg && *r->msg) {
> > > +				igt_skip_on_f(r->code == IGT_EXIT_SKIP,
> > > +					      "%s\n", r->msg);
> > > +				igt_fail_on_f(r->code == IGT_EXIT_FAILURE,
> > > +					      "%s\n", r->msg);
> > > +				igt_abort_on_f(r->code == IGT_EXIT_ABORT,
> > > +					      "%s\n", r->msg);
> > > +			} else {
> > > +				igt_skip_on(r->code == IGT_EXIT_SKIP);
> > > +				igt_fail_on(r->code == IGT_EXIT_FAILURE);
> > > +				if (r->code == IGT_EXIT_ABORT)
> > > +					igt_fail(r->code);
> > > +			}
> > > +			igt_assert_eq(r->code, IGT_EXIT_SUCCESS);
> > > +
> > > +			switch (pthread_mutex_lock(&modprobe.lock)) {
> > > +			case 0:
> > > +				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > > +				break;
> > > +			case EOWNERDEAD:
> > > +				/* leave the mutex unrecoverable */
> > > +				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > > +				__attribute__ ((fallthrough));
> > > +			case ENOTRECOVERABLE:
> > >  				igt_assert_eq(modprobe.err, 0);
> > > +				break;
> > 
> > imho we should also break a do-while loop in case of error.
> 
> We are inside igt_dynamic() body, then I don't think we can safely break the 
> outer loop from here, only pass, skip, or fail.
> 
> If we are not able to check whether the modprobe thread hasn't returned an 
> error then I don't think that's a good reason for failing or skipping the 
> current dynamic sub-subtest since we have its KTAP results already collected 
> and we should just report them in that case, I believe.
> 
> If the error persists then the loop will get broken on next attempt to lock 
> the mutex before entering blocking read(), next dynamic sub-subtests won't be 
> executed, and the whole subtest will inform the user about that incompleteness 
> of results from dynamic sub-subtests by returning SKIP.  Isn't that 
> sufficient?
> 

Yes it is, thank you for explaining it.

> > 
> > > +			default:
> > > +				igt_debug("pthread_mutex_lock() failed\n");
> > > +				break;
> > > +			}
> > >  
> > >  			igt_assert_eq(igt_kernel_tainted(&taints), 0);
> > >  		}
> > >  
> > > -		free(result);
> > > +		kunit_result_free(&r, &suite_name, &case_name);
> > > +
> > > +	} while (ret == -EINPROGRESS);
> > > +
> > > +	igt_list_for_each_entry_safe(r, rn, &results, link)
> > > +		kunit_result_free(&r, &suite_name, &case_name);
> > > +
> > > +	free(case_name);
> > > +	free(suite_name);
> > > +
> > > +	switch (pthread_mutex_lock(&modprobe.lock)) {
> > > +	case 0:
> > > +		igt_debug_on(pthread_cancel(modprobe.thread));
> > > +		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > > +		igt_debug_on(pthread_join(modprobe.thread, NULL));
> > > +		break;
> > > +	case EOWNERDEAD:
> > > +		/* leave the mutex unrecoverable */
> > > +		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > > +		break;
> > > +	case ENOTRECOVERABLE:
> > > +		break;
> > > +	default:
> > > +		igt_debug("pthread_mutex_lock() failed\n");
> > > +		igt_debug_on(pthread_join(modprobe.thread, NULL));
> > > +		break;
> > >  	}
> > >  
> > > -	pthread_join(modprobe_thread, NULL);
> > > -
> > > -	ret = ktap_parser_stop();
> > > +	igt_ktap_free(ktap);
> > >  
> > >  	igt_skip_on(modprobe.err);
> > >  	igt_skip_on(igt_kernel_tainted(&taints));
> > > diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
> > > index 5eac102417..53a6c63288 100644
> > > --- a/lib/igt_ktap.c
> > > +++ b/lib/igt_ktap.c
> > > @@ -4,17 +4,11 @@
> > >   * Copyright © 2023 Intel Corporation
> > >   */
> > >  
> > > -#include <ctype.h>
> > > -#include <limits.h>
> > > -#include <libkmod.h>
> > > -#include <pthread.h>
> > >  #include <errno.h>
> > >  #include <stdio.h>
> > >  #include <stdlib.h>
> > >  #include <string.h>
> > > -#include <unistd.h>
> > >  
> > > -#include "igt_aux.h"
> > >  #include "igt_core.h"
> > >  #include "igt_ktap.h"
> > >  #include "igt_list.h"
> > > @@ -312,565 +306,3 @@ void igt_ktap_free(struct igt_ktap_results *ktap)
> > >  {
> > >  	free(ktap);
> > >  }
> > > -
> > > -#define DELIMITER "-"
> > > -
> > > -struct ktap_parser_args {
> > > -	int fd;
> > > -	bool is_builtin;
> > > -	int ret;
> > > -} ktap_args;
> > > -
> > > -static struct ktap_test_results results;
> > > -
> > > -static int log_to_end(enum igt_log_level level, int fd,
> > > -		      char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
> > > -
> > > -/**
> > > - * log_to_end:
> > > - * @level: #igt_log_level
> > > - * @record: record to store the read data
> > > - * @format: format string
> > > - * @...: optional arguments used in the format string
> > > - *
> > > - * This is an altered version of the generic structured logging helper function
> > > - * igt_log capable of reading to the end of a given line.
> > > - *
> > > - * Returns: 0 for success, or -2 if there's an error reading from the file
> > > - */
> > > -static int log_to_end(enum igt_log_level level, int fd,
> > > -		      char *record, const char *format, ...)
> > > -{
> > > -	va_list args;
> > > -	const char *lend;
> > > -
> > > -	/* Cutoff after newline character, in order to not display garbage */
> > > -	char *cutoff = strchr(record, '\n');
> > > -	if (cutoff) {
> > > -		if (cutoff - record < BUF_LEN)
> > > -			cutoff[1] = '\0';
> > > -	}
> > > -
> > > -	va_start(args, format);
> > > -	igt_vlog(IGT_LOG_DOMAIN, level, format, args);
> > > -	va_end(args);
> > > -
> > > -	lend = strchrnul(record, '\n');
> > > -	while (*lend == '\0') {
> > > -		igt_log(IGT_LOG_DOMAIN, level, "%s", record);
> > > -
> > > -		if (read(fd, record, BUF_LEN) < 0) {
> > > -			if (errno == EPIPE)
> > > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > > -			else
> > > -				igt_warn("an error occurred while reading kmsg: %m\n");
> > > -
> > > -			return -2;
> > > -		}
> > > -
> > > -		lend = strchrnul(record, '\n');
> > > -	}
> > > -	return 0;
> > > -}
> > > -
> > > -/**
> > > - * lookup_value:
> > > - * @haystack: the string to search in
> > > - * @needle: the string to search for
> > > - *
> > > - * Returns: the value of the needle in the haystack, or -1 if not found.
> > > - */
> > > -static long lookup_value(const char *haystack, const char *needle)
> > > -{
> > > -	const char *needle_rptr;
> > > -	char *needle_end;
> > > -	long num;
> > > -
> > > -	needle_rptr = strcasestr(haystack, needle);
> > > -
> > > -	if (needle_rptr == NULL)
> > > -		return -1;
> > > -
> > > -	/* Skip search string and whitespaces after it */
> > > -	needle_rptr += strlen(needle);
> > > -
> > > -	num = strtol(needle_rptr, &needle_end, 10);
> > > -
> > > -	if (needle_rptr == needle_end)
> > > -		return -1;
> > > -
> > > -	if (num == LONG_MIN || num == LONG_MAX)
> > > -		return 0;
> > > -
> > > -	return num > 0 ? num : 0;
> > > -}
> > > -
> > > -/**
> > > - * tap_version_present:
> > > - * @record: buffer with tap data
> > > - * @print_info: whether tap version should be printed or not
> > > - *
> > > - * Returns:
> > > - * 0 if not found
> > > - * 1 if found
> > > - */
> > > -static int tap_version_present(char* record, bool print_info)
> > > -{
> > > -	/*
> > > -	 * "(K)TAP version XX" should be the first line on all (sub)tests as per
> > > -	 * https://kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines
> > 
> > Could you save these two lines comment in some other place?
> 
> Why?  Do you think that just that information from a multi-paragraph 
> description of KTAP format is of particular importance?
> 
> Instead, let me add just the reference to the KTAP standard to a comment above 
> the current parser implementation, OK?

Seems good, please add the reference.

Regards,
Kamil

> 
> Thanks,
> Janusz
> 
> > 
> > Regards,
> > Kamil
> > 
> > > -	 *
> > > -	 * but actually isn't, as it currently depends on the KUnit module
> > > -	 * being built-in, so we can't rely on it every time
> > > -	 */
> > > -	const char *version_rptr = strcasestr(record, "TAP version ");
> > > -	char *cutoff;
> > > -
> > > -	if (version_rptr == NULL)
> > > -		return 0;
> > > -
> > > -	/* Cutoff after newline character, in order to not display garbage */
> > > -	cutoff = strchr(version_rptr, '\n');
> > > -	if (cutoff)
> > > -		cutoff[0] = '\0';
> > > -
> > > -	if (print_info)
> > > -		igt_info("%s\n", version_rptr);
> > > -
> > > -	return 1;
> > > -}
> > > -
> > > -/**
> > > - * find_next_tap_subtest:
> > > - * @fd: file descriptor
> > > - * @record: buffer used to read fd
> > > - * @is_builtin: whether KUnit is built-in or not
> > > - *
> > > - * Returns:
> > > - * 0 if there's missing information
> > > - * -1 if not found
> > > - * -2 if there are problems while reading the file.
> > > - * any other value corresponds to the amount of cases of the next (sub)test
> > > - */
> > > -static int find_next_tap_subtest(int fd, char *record, char *test_name, bool is_builtin)
> > > -{
> > > -	const char *test_lookup_str, *subtest_lookup_str, *name_rptr;
> > > -	long test_count;
> > > -	char *cutoff;
> > > -
> > > -	test_name[0] = '\0';
> > > -	test_name[BUF_LEN] = '\0';
> > > -
> > > -	test_lookup_str = " subtest: ";
> > > -	subtest_lookup_str = " test: ";
> > > -
> > > -	if (!tap_version_present(record, true))
> > > -		return -1;
> > > -
> > > -	if (is_builtin) {
> > > -		if (read(fd, record, BUF_LEN) < 0) {
> > > -			if (errno == EPIPE)
> > > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > > -			else
> > > -				igt_warn("an error occurred while reading kmsg: %m\n");
> > > -
> > > -			return -2;
> > > -		}
> > > -	}
> > > -
> > > -	name_rptr = strcasestr(record, test_lookup_str);
> > > -	if (name_rptr != NULL) {
> > > -		name_rptr += strlen(test_lookup_str);
> > > -	} else {
> > > -		name_rptr = strcasestr(record, subtest_lookup_str);
> > > -		if (name_rptr != NULL)
> > > -			name_rptr += strlen(subtest_lookup_str);
> > > -	}
> > > -
> > > -	if (name_rptr == NULL) {
> > > -		if (!is_builtin)
> > > -			/* We've probably found nothing */
> > > -			return -1;
> > > -		igt_info("Missing test name\n");
> > > -	} else {
> > > -		strncpy(test_name, name_rptr, BUF_LEN);
> > > -		/* Cutoff after newline character, in order to not display garbage */
> > > -		cutoff = strchr(test_name, '\n');
> > > -		if (cutoff)
> > > -			cutoff[0] = '\0';
> > > -
> > > -		if (read(fd, record, BUF_LEN) < 0) {
> > > -			if (errno == EPIPE)
> > > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > > -			else
> > > -				igt_warn("unknown error reading kmsg (%m)\n");
> > > -
> > > -			return -2;
> > > -		}
> > > -
> > > -		/* Now we can be sure we found tests */
> > > -		if (!is_builtin)
> > > -			igt_info("KUnit is not built-in, skipping version check...\n");
> > > -	}
> > > -
> > > -	/*
> > > -	 * Total test count will almost always appear as 0..N at the beginning
> > > -	 * of a run, so we use it to reliably identify a new run
> > > -	 */
> > > -	test_count = lookup_value(record, "..");
> > > -
> > > -	if (test_count <= 0) {
> > > -		igt_info("Missing test count\n");
> > > -		if (test_name[0] == '\0')
> > > -			return 0;
> > > -		if (log_to_end(IGT_LOG_INFO, fd, record,
> > > -				"Running some tests in: %s\n",
> > > -				test_name) < 0)
> > > -			return -2;
> > > -		return 0;
> > > -	} else if (test_name[0] == '\0') {
> > > -		igt_info("Running %ld tests...\n", test_count);
> > > -		return 0;
> > > -	}
> > > -
> > > -	if (log_to_end(IGT_LOG_INFO, fd, record,
> > > -			"Executing %ld tests in: %s\n",
> > > -			test_count, test_name) < 0)
> > > -		return -2;
> > > -
> > > -	return test_count;
> > > -}
> > > -
> > > -/**
> > > - * parse_kmsg_for_tap:
> > > - * @fd: file descriptor
> > > - * @record: buffer used to read fd
> > > - * @test_name: buffer to store the test name
> > > - *
> > > - * Returns:
> > > - * 1 if no results were found
> > > - * 0 if a test succeded
> > > - * -1 if a test failed
> > > - * -2 if there are problems reading the file
> > > - */
> > > -static int parse_kmsg_for_tap(int fd, char *record, char *test_name)
> > > -{
> > > -	const char *lstart, *ok_lookup_str, *nok_lookup_str,
> > > -	      *ok_rptr, *nok_rptr, *comment_start, *value_parse_start;
> > > -	char *test_name_end;
> > > -
> > > -	ok_lookup_str = "ok ";
> > > -	nok_lookup_str = "not ok ";
> > > -
> > > -	lstart = strchrnul(record, ';');
> > > -
> > > -	if (*lstart == '\0') {
> > > -		igt_warn("kmsg truncated: output malformed (%m)\n");
> > > -		return -2;
> > > -	}
> > > -
> > > -	lstart++;
> > > -	while (isspace(*lstart))
> > > -		lstart++;
> > > -
> > > -	nok_rptr = strstr(lstart, nok_lookup_str);
> > > -	if (nok_rptr != NULL) {
> > > -		nok_rptr += strlen(nok_lookup_str);
> > > -		while (isdigit(*nok_rptr) || isspace(*nok_rptr) || *nok_rptr == '-')
> > > -			nok_rptr++;
> > > -		test_name_end = strncpy(test_name, nok_rptr, BUF_LEN);
> > > -		while (!isspace(*test_name_end))
> > > -			test_name_end++;
> > > -		*test_name_end = '\0';
> > > -		if (log_to_end(IGT_LOG_WARN, fd, record,
> > > -			       "%s", lstart) < 0)
> > > -			return -2;
> > > -		return -1;
> > > -	}
> > > -
> > > -	comment_start = strchrnul(lstart, '#');
> > > -
> > > -	/* Check if we're still in a subtest */
> > > -	if (*comment_start != '\0') {
> > > -		comment_start++;
> > > -		value_parse_start = comment_start;
> > > -
> > > -		if (lookup_value(value_parse_start, "fail: ") > 0) {
> > > -			if (log_to_end(IGT_LOG_WARN, fd, record,
> > > -				       "%s", lstart) < 0)
> > > -				return -2;
> > > -			return -1;
> > > -		}
> > > -	}
> > > -
> > > -	ok_rptr = strstr(lstart, ok_lookup_str);
> > > -	if (ok_rptr != NULL) {
> > > -		ok_rptr += strlen(ok_lookup_str);
> > > -		while (isdigit(*ok_rptr) || isspace(*ok_rptr) || *ok_rptr == '-')
> > > -			ok_rptr++;
> > > -		test_name_end = strncpy(test_name, ok_rptr, BUF_LEN);
> > > -		while (!isspace(*test_name_end))
> > > -			test_name_end++;
> > > -		*test_name_end = '\0';
> > > -		return 0;
> > > -	}
> > > -
> > > -	return 1;
> > > -}
> > > -
> > > -/**
> > > - * parse_tap_level:
> > > - * @fd: file descriptor
> > > - * @base_test_name: test_name from upper recursion level
> > > - * @test_count: test_count of this level
> > > - * @failed_tests: top level failed_tests pointer
> > > - * @found_tests: top level found_tests pointer
> > > - * @is_builtin: whether the KUnit module is built-in or not
> > > - *
> > > - * Returns:
> > > - * 0 if succeded
> > > - * -1 if error occurred
> > > - */
> > > -__maybe_unused
> > > -static int parse_tap_level(int fd, char *base_test_name, int test_count, bool *failed_tests,
> > > -			   bool *found_tests, bool is_builtin)
> > > -{
> > > -	char record[BUF_LEN + 1];
> > > -	struct ktap_test_results_element *r;
> > > -	int internal_test_count;
> > > -	char test_name[BUF_LEN + 1];
> > > -	char base_test_name_for_next_level[BUF_LEN + 1];
> > > -
> > > -	for (int i = 0; i < test_count; i++) {
> > > -		if (read(fd, record, BUF_LEN) < 0) {
> > > -			if (errno == EPIPE)
> > > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > > -			else
> > > -				igt_warn("error reading kmsg (%m)\n");
> > > -
> > > -			return -1;
> > > -		}
> > > -
> > > -		/* Sublevel found */
> > > -		if (tap_version_present(record, false))
> > > -		{
> > > -			internal_test_count = find_next_tap_subtest(fd, record, test_name,
> > > -								    is_builtin);
> > > -			switch (internal_test_count) {
> > > -			case -2:
> > > -				/* No more data to read */
> > > -				return -1;
> > > -			case -1:
> > > -				/* No test found */
> > > -				return -1;
> > > -			case 0:
> > > -				/* Tests found, but they're missing info */
> > > -				*found_tests = true;
> > > -				return -1;
> > > -			default:
> > > -				*found_tests = true;
> > > -
> > > -				memcpy(base_test_name_for_next_level, base_test_name, BUF_LEN);
> > > -				if (strlen(base_test_name_for_next_level) < BUF_LEN - 1 &&
> > > -				    base_test_name_for_next_level[0])
> > > -					strncat(base_test_name_for_next_level, DELIMITER,
> > > -						BUF_LEN - strlen(base_test_name_for_next_level));
> > > -				memcpy(base_test_name_for_next_level + strlen(base_test_name_for_next_level),
> > > -				       test_name, BUF_LEN - strlen(base_test_name_for_next_level));
> > > -
> > > -				if (parse_tap_level(fd, base_test_name_for_next_level,
> > > -						    internal_test_count, failed_tests, found_tests,
> > > -						    is_builtin) == -1)
> > > -					return -1;
> > > -				break;
> > > -			}
> > > -		}
> > > -
> > > -		switch (parse_kmsg_for_tap(fd, record, test_name)) {
> > > -		case -2:
> > > -			return -1;
> > > -		case -1:
> > > -			*failed_tests = true;
> > > -
> > > -			r = malloc(sizeof(*r));
> > > -
> > > -			memcpy(r->test_name, base_test_name, BUF_LEN);
> > > -			if (strlen(r->test_name) < BUF_LEN - 1)
> > > -				if (r->test_name[0])
> > > -					strncat(r->test_name, DELIMITER,
> > > -						BUF_LEN - strlen(r->test_name));
> > > -			memcpy(r->test_name + strlen(r->test_name), test_name,
> > > -			       BUF_LEN - strlen(r->test_name));
> > > -			r->test_name[BUF_LEN] = '\0';
> > > -
> > > -			r->passed = false;
> > > -
> > > -			pthread_mutex_lock(&results.mutex);
> > > -			igt_list_add_tail(&r->link, &results.list);
> > > -			pthread_mutex_unlock(&results.mutex);
> > > -
> > > -			test_name[0] = '\0';
> > > -			break;
> > > -		case 0:
> > > -			r = malloc(sizeof(*r));
> > > -
> > > -			memcpy(r->test_name, base_test_name, BUF_LEN);
> > > -			if (strlen(r->test_name) < BUF_LEN - 1)
> > > -				if (r->test_name[0])
> > > -					strncat(r->test_name, DELIMITER,
> > > -						BUF_LEN - strlen(r->test_name));
> > > -			memcpy(r->test_name + strlen(r->test_name), test_name,
> > > -			       BUF_LEN - strlen(r->test_name));
> > > -			r->test_name[BUF_LEN] = '\0';
> > > -
> > > -			r->passed = true;
> > > -
> > > -			pthread_mutex_lock(&results.mutex);
> > > -			igt_list_add_tail(&r->link, &results.list);
> > > -			pthread_mutex_unlock(&results.mutex);
> > > -
> > > -			test_name[0] = '\0';
> > > -			break;
> > > -		default:
> > > -			break;
> > > -		}
> > > -	}
> > > -	return 0;
> > > -}
> > > -
> > > -/**
> > > - * igt_ktap_parser:
> > > - *
> > > - * This function parses the output of a ktap script and passes it to main thread.
> > > - */
> > > -void *igt_ktap_parser(void *unused)
> > > -{
> > > -	char record[BUF_LEN + 1], *buf, *suite_name = NULL, *case_name = NULL;
> > > -	struct igt_ktap_results *ktap = NULL;
> > > -	int fd = ktap_args.fd;
> > > -	IGT_LIST_HEAD(list);
> > > -	int err;
> > > -
> > > -	ktap = igt_ktap_alloc(&list);
> > > -	if (igt_debug_on(!ktap))
> > > -		goto igt_ktap_parser_end;
> > > -
> > > -	while (err = read(fd, record, BUF_LEN), err > 0) {
> > > -		struct igt_ktap_result *r, *rn;
> > > -
> > > -		/* skip kmsg continuation lines */
> > > -		if (igt_debug_on(*record == ' '))
> > > -			continue;
> > > -
> > > -		/* NULL-terminate the record */
> > > -		record[err] = '\0';
> > > -
> > > -		/* detect start of log message, continue if not found */
> > > -		buf = strchrnul(record, ';');
> > > -		if (igt_debug_on(*buf == '\0'))
> > > -			continue;
> > > -		buf++;
> > > -
> > > -		err = igt_ktap_parse(buf, ktap);
> > > -
> > > -		/* parsing error */
> > > -		if (err && err != -EINPROGRESS)
> > > -			goto igt_ktap_parser_end;
> > > -
> > > -		igt_list_for_each_entry_safe(r, rn, &list, link) {
> > > -			struct ktap_test_results_element *result = NULL;
> > > -			int code = r->code;
> > > -
> > > -			if (code != IGT_EXIT_INVALID)
> > > -				result = calloc(1, sizeof(*result));
> > > -
> > > -			if (result) {
> > > -				snprintf(result->test_name, sizeof(result->test_name),
> > > -					 "%s-%s", r->suite_name, r->case_name);
> > > -
> > > -				if (code == IGT_EXIT_SUCCESS)
> > > -					result->passed = true;
> > > -			}
> > > -
> > > -			igt_list_del(&r->link);
> > > -			if (r->suite_name != suite_name) {
> > > -				free(suite_name);
> > > -				suite_name = r->suite_name;
> > > -			}
> > > -			if (r->case_name != case_name) {
> > > -				free(case_name);
> > > -				case_name = r->case_name;
> > > -			}
> > > -			free(r->msg);
> > > -			free(r);
> > > -
> > > -			/*
> > > -			 * no extra result record expected on start
> > > -			 * of parametrized test case -- skip it
> > > -			 */
> > > -			if (code == IGT_EXIT_INVALID)
> > > -				continue;
> > > -
> > > -			if (!result) {
> > > -				err = -ENOMEM;
> > > -				goto igt_ktap_parser_end;
> > > -			}
> > > -
> > > -			pthread_mutex_lock(&results.mutex);
> > > -			igt_list_add_tail(&result->link, &results.list);
> > > -			pthread_mutex_unlock(&results.mutex);
> > > -		}
> > > -
> > > -		/* end of KTAP report */
> > > -		if (!err)
> > > -			goto igt_ktap_parser_end;
> > > -	}
> > > -
> > > -	if (err < 0) {
> > > -		if (errno == EPIPE)
> > > -			igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > > -		else
> > > -			igt_warn("error reading kmsg (%m)\n");
> > > -	}
> > > -
> > > -igt_ktap_parser_end:
> > > -	free(suite_name);
> > > -	free(case_name);
> > > -
> > > -	if (!err)
> > > -		ktap_args.ret = IGT_EXIT_SUCCESS;
> > > -
> > > -	results.still_running = false;
> > > -
> > > -	if (ktap)
> > > -		igt_ktap_free(ktap);
> > > -
> > > -	return NULL;
> > > -}
> > > -
> > > -static pthread_t ktap_parser_thread;
> > > -
> > > -struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin)
> > > -{
> > > -	IGT_INIT_LIST_HEAD(&results.list);
> > > -	pthread_mutex_init(&results.mutex, NULL);
> > > -	results.still_running = true;
> > > -
> > > -	ktap_args.fd = fd;
> > > -	ktap_args.is_builtin = is_builtin;
> > > -	ktap_args.ret = IGT_EXIT_FAILURE;
> > > -	pthread_create(&ktap_parser_thread, NULL, igt_ktap_parser, NULL);
> > > -
> > > -	return &results;
> > > -}
> > > -
> > > -void ktap_parser_cancel(void)
> > > -{
> > > -	pthread_cancel(ktap_parser_thread);
> > > -}
> > > -
> > > -int ktap_parser_stop(void)
> > > -{
> > > -	pthread_join(ktap_parser_thread, NULL);
> > > -	return ktap_args.ret;
> > > -}
> > > diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
> > > index 6f8da3eab6..c422636bfc 100644
> > > --- a/lib/igt_ktap.h
> > > +++ b/lib/igt_ktap.h
> > > @@ -27,8 +27,6 @@
> > >  
> > >  #define BUF_LEN 4096
> > >  
> > > -#include <pthread.h>
> > > -
> > >  #include "igt_list.h"
> > >  
> > >  struct igt_ktap_result {
> > > @@ -45,24 +43,4 @@ struct igt_ktap_results *igt_ktap_alloc(struct igt_list_head *results);
> > >  int igt_ktap_parse(const char *buf, struct igt_ktap_results *ktap);
> > >  void igt_ktap_free(struct igt_ktap_results *ktap);
> > >  
> > > -void *igt_ktap_parser(void *unused);
> > > -
> > > -typedef struct ktap_test_results_element {
> > > -	char test_name[BUF_LEN + 1];
> > > -	bool passed;
> > > -	struct igt_list_head link;
> > > -} ktap_test_results_element;
> > > -
> > > -struct ktap_test_results {
> > > -	struct igt_list_head list;
> > > -	pthread_mutex_t mutex;
> > > -	bool still_running;
> > > -};
> > > -
> > > -
> > > -
> > > -struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin);
> > > -void ktap_parser_cancel(void);
> > > -int ktap_parser_stop(void);
> > > -
> > >  #endif /* IGT_KTAP_H */
> > 
> 
> 
> 
> 



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux