On 01/14/2014 02:55 PM, Pádraig Brady wrote:
> On 01/14/2014 12:38 PM, Bernhard Voelker wrote:
>>> * tests/cp/no-ctx.sh: Since the test diagnoses whether the
>>> intercepted lgetfilecon() calls are actually called or not,
>>
>> The witness file is only created for getfilecon() - not for
>> lgetfilecon().
>
> In the wrapper, lgetfilecon() calls getfilecon() ?
Ah, sure. I missed that, sorry.
>>> diff --git a/tests/cp/no-ctx.sh b/tests/cp/no-ctx.sh
>>> index 3b5eb82..6851785 100755
>>> --- a/tests/cp/no-ctx.sh
>>> +++ b/tests/cp/no-ctx.sh
>>> @@ -22,6 +22,7 @@
>>> . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
>>> print_ver_ cp
>>> require_gcc_shared_
>>> +requires_selinux_
>
> BTW that should be require_selinux_
> It's dangerous that we don't diagnose such typos.
> I wonder would it be appropriate to have a test_require_()
> wrapper that would catch such things, and be called like:
> test_require_ gcc_shared selinux
Hmm, but that would imply the same problem - if someone
misspells "test_require_" ... for which we could maybe add
a syntax-check rule.
I'm not sure if it's worth the effort - when adding/changing
a test, we have to look into the .log file anyway.
>>> # Replace each getfilecon and lgetfilecon call with a call to these stubs.
>>> cat > k.c <<'EOF' || framework_failure_
>>
>> I'm a bit biased about this patch. Okay, it's perfectly valid to
>> skip the test if the system doesn't support SELinux, but OTOH it may
>> be quite valuable to verify the exit codes like that on non-SELinux
>> systems,
>
> Well I did state that "The test cases are minimal on non SELinux systems
> and should be well covered by other tests"...
okay, I'm fine with that.
>> i.e., based on stderr of the last cp call, the "preloaded"
>> file must exist or not. The test could verify that. WDYT?
>
> ...and if the last cp fails it could be due to the wrapper running,
> or SELinux not being supported. We'd need something else to
> distinguish here, and require_selinux_ is the best I can think of
> at present.
>
> I suppose an alternative would be to refactor require_selinux_
> to a function that just determines if it's available and do:
>
> test -e preloaded ||
> { have_selinux_ && framework_failure_ 'LD_PRELOAD interception failed'; }
I think we should keep it as simple as possible, therefore I'd
now favor your initial version of the patch (with the typo corrected).
Thanks & have a nice day,
Berny
