[libcamera-devel] [PATCH 2/3] test: log/process: check CAP_SYS_ADMIN in test init

[Laurent Pinchart]

Hi You-Sheng,

Thank you for the patch.

On Sat, Jul 25, 2020 at 08:24:41PM +0800, You-Sheng Yang wrote:
> While these tests may be executed as normal user at build time,
> unshare() call will fail and so are tests log_process and process_test.
> This change checks if one is granted with necessary capabilities so that
> we don't fail the build unexpectedly.
> 
> Signed-off-by: You-Sheng Yang <vicamo.yang at canonical.com>
> ---
>  test/log/log_process.cpp      | 20 ++++++++++++++++++++
>  test/log/meson.build          |  2 +-
>  test/meson.build              |  2 ++
>  test/process/meson.build      |  2 +-
>  test/process/process_test.cpp | 23 +++++++++++++++++++++++
>  5 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
> index d46d5e3..876da22 100644
> --- a/test/log/log_process.cpp
> +++ b/test/log/log_process.cpp
> @@ -9,6 +9,7 @@
>  #include <iostream>
>  #include <random>
>  #include <string.h>
> +#include <sys/capability.h>
>  #include <sys/stat.h>
>  #include <sys/types.h>
>  #include <unistd.h>
> @@ -55,6 +56,25 @@ class LogProcessTest : public Test
>  protected:
>  	int init()
>  	{
> +		int ret = TestPass;
> +
> +		cap_t caps = cap_get_proc();
> +		if (caps == NULL) {
> +			cerr << "failed to check process capabilities" << endl;
> +			return TestFail;
> +		}
> +
> +		/* Check required permissions: CAP_SYS_ADMIN: unshare */
> +		cap_flag_value_t fv;
> +		if ((cap_get_flag(caps, CAP_SYS_ADMIN, CAP_EFFECTIVE, &fv) < 0) || (fv != CAP_SET)) {
> +			cerr << "skip due to insufficient capability" << endl;
> +			ret = TestSkip;
> +		}

Would it make sense to add this as a helper function to the base Test
class ?

> +
> +		cap_free(caps);
> +		if (ret != TestPass)
> +			return ret;
> +
>  		random_device random;
>  		num_ = random();
>  		logPath_ = "/tmp/libcamera.worker.test." +
> diff --git a/test/log/meson.build b/test/log/meson.build
> index 8cd664e..000f980 100644
> --- a/test/log/meson.build
> +++ b/test/log/meson.build
> @@ -7,7 +7,7 @@ log_test = [
>  
>  foreach t : log_test
>      exe = executable(t[0], t[1],
> -                     dependencies : libcamera_dep,
> +                     dependencies : [libcamera_dep, libcap],
>                       link_with : test_libraries,
>                       include_directories : test_includes_internal)
>  
> diff --git a/test/meson.build b/test/meson.build
> index f41d6e7..b4db328 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
> +libcap = dependency('libcap', required : true)

'true' is the default value for 'required', you can omit it. However,
I'd like to keep the dependency optional, as we try to also support
resource-constrainted embedded systems (based on musl or uclibc for
instance, and/or without udev).

I have an idea how to do that, I'll try to submit a patch shortly.

> +
>  subdir('libtest')
>  
>  subdir('camera')
> diff --git a/test/process/meson.build b/test/process/meson.build
> index c215fa7..828c17b 100644
> --- a/test/process/meson.build
> +++ b/test/process/meson.build
> @@ -6,7 +6,7 @@ process_tests = [
>  
>  foreach t : process_tests
>      exe = executable(t[0], t[1],
> -                     dependencies : libcamera_dep,
> +                     dependencies : [libcamera_dep, libcap],
>                       link_with : test_libraries,
>                       include_directories : test_includes_internal)
>  
> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> index ce0cc7c..ffa2143 100644
> --- a/test/process/process_test.cpp
> +++ b/test/process/process_test.cpp
> @@ -5,6 +5,8 @@
>   * process_test.cpp - Process test
>   */
>  
> +#include <sys/capability.h>
> +
>  #include <iostream>
>  #include <unistd.h>
>  #include <vector>
> @@ -41,6 +43,27 @@ public:
>  	}
>  
>  protected:
> +	int init()
> +	{
> +		int ret = TestPass;
> +
> +		cap_t caps = cap_get_proc();
> +		if (caps == NULL) {
> +			cerr << "failed to check process capabilities" << endl;
> +			return TestFail;
> +		}
> +
> +		/* Check required permissions: CAP_SYS_ADMIN: unshare */
> +		cap_flag_value_t fv;
> +		if ((cap_get_flag(caps, CAP_SYS_ADMIN, CAP_EFFECTIVE, &fv) < 0) || (fv != CAP_SET)) {
> +			cerr << "skip due to insufficient capability" << endl;
> +			ret = TestSkip;
> +		}
> +
> +		cap_free(caps);
> +		return ret;
> +	}
> +
>  	int run()
>  	{
>  		EventDispatcher *dispatcher = Thread::current()->eventDispatcher();

-- 
Regards,

Laurent Pinchart