On Wed, 16 Oct 2019, Brendan Higgins wrote:
On Fri, Oct 11, 2019 at 11:25:33AM +0100, Alan Maguire wrote:
On Fri, 11 Oct 2019, Brendan Higgins wrote:
Sorry for the delayed reply. I will be on vacation until Wednesday, October 16th.
On Wed, Oct 9, 2019 at 9:36 AM Alan Maguire alan.maguire@oracle.com wrote:
On Tue, 8 Oct 2019, Brendan Higgins wrote:
On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote:
[...]
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index e6d17aa..e4f3a97 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream,
return 0;} +EXPORT_SYMBOL_GPL(string_stream_vadd);
Is this actually needed by anything other than lib/kunit/test.c right now? Maybe we should move the include file into the kunit/ directory to hide these so no one else can use them.
I tried this, and it's the right answer I think but it exposes a problem with symbol visibility when kunit is compiled as a module. More on this below...
int string_stream_add(struct string_stream *stream, const char *fmt, ...) { @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...)
return result;} +EXPORT_SYMBOL_GPL(string_stream_add);
[...]
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c83c0fa..e7896f1 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c
[...]
@@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) * For more background on this topic, see: * https://mike-bland.com/2011/11/01/small-medium-large.html */ +#ifndef MODULE
Why is this block of code "ifndef MODULE"?
Symbol visibility is the problem again; sysctl_hung_task_timeout_secs isn't exported so when kunit is a module it can't find the symbol.
I think I saw Kees mentioned something about symbol lookup too; in KTF Knut solved this by defining ktf_find_symbol(). I'd suggest we may need a kunit_find_symbol() with a function signature
Based on what you said below, I think the kunit_find_symbol() may have value for writing tests; however, I do not think it is the right way to handle resources needed by test.c. I think exporting the symbols in this case is the lesser of the two evils.
The only symbol we need in core kunit from the kernel when compiling kunit as a module is sysctl_hung_task_timeout_secs; it's a core kernel symbol. There's no issue with symbols within kunit in this case.
I've come up with a new way to handle variables and functions in the v3 patch set 3 [1] I've sent out. While not being perfect, it attempts to satisfy some of the requirements you describe below. It will generate compiler errors if there is a mismatch between local symbol definition and the target symbol type.
Symbol variable definitions are handled such that the same symbol name can be used; see try-catch.c in patch 5 where we assign sysctl_hung_task_timeout_secs.
Unfortunately the same scheme won't work for functions. The reason for this is we've already #included a definiton of the function, so if we attempt to redefine that same name as a function pointer we get a compile-time that we are redefining the symbol. As a consequence the approach I took is for us to define a local function pointer and it gets assigned either to
- the results of kunit_find_symbol() (module case) - the function itself (builtin case)
The latter will trigger a compile-time error if our local definition is out of sync.
I am still suprised that you need to export a symbol that is
getting
compiled into and is only used by the kunit module.
see above - kunit needs a non-exported global kernel symbol (sysctl_hung_task_timeout_secs).
In fact, I think I found an example in the kernel where someone else managed this. Checkout stp_policy_node_priv(). Looks like the symbol is used here[1] and is defined here[2]. You can see here[3] and here[4] that the files end up in the same module. Do you mind taking a look why it works for stm, but not here?
I thought we were just talking about exposing symbols for linking outside of a compilation unit (static vs. not static); nevertheless, I think you are right that it is relevant here. Kees, thoughts?
void *kunit_find_symbol(const char *modname, const char *symbol_name);
...which does a [module_]kallsyms_lookup_sym().
If the above makes sense I can look at adding it as a patch (and adding a test of it of course!). What do you think?
So that won't work if you are trying to link against a symbol not in a module, right? Also, it won't work against a static symbol, right?
Nope, works in both cases with the proviso that we need to use an
Nifty! That sounds great!
alternative name for symbols when compiling built-in. For example
Can you elaborate on "need[ing] to use an alternative name"?
See above and patch 4 in the v3 patchset.
in the case of the string-stream tests, we'd use a test init callback to initialize used symbols:
static int string_stream_test_init(struct kunit *test) { _alloc_string_stream = kunit_find_symbol("alloc_string_stream"); _string_stream_add = kunit_find_symbol("string_stream_add"); _string_stream_get_string = kunit_find_symbol("string_stream_get_string"); _string_stream_is_empty = kunit_find_symbol("string_stream_is_empty"); if (IS_ERR(_alloc_string_stream) || IS_ERR(_string_stream_add) || IS_ERR(_string_stream_get_string) || IS_ERR(_string_stream_is_empty)) return EINVAL; return 0; }
I've tested this when string-stream-test is compiled built-in and as a module. We can of course create a wrapper macro to handle these assignments.
I've got mixed feelings on this. On one hand, that has the potential to solve a lot of problems with visibility and modules in a way that doesn't immediately cause code under test to change in undesirable ways. On the other hand, I feel that this has the potential to be really prone to breakage. It would be much nicer if the compiler could tell you that your symbol changed rather than having to wait until you run the test. Just having the test tell you that a symbol doesn't exist anymore would be mildly annoying, but having the signature of the symbol change could get downright frustrating using this method.
See above; if compiled as builtin compiler errors will be generated.
Thanks!
Alan