[LTP] [PATCH] syscalls/memfd_create: test memfd flags separately
Jakub Raček
jracek@redhat.com
Wed Jun 7 14:13:50 CEST 2017
Hi,
On 06/06/2017 03:15 PM, Cyril Hrubis wrote:
> Hi!
>> This patch fixes the test on some kernels where memfd_create doesn't
>> have have sealing support. Calls to memfd_create are done so that we
>> know what flags are available. Depending on that, some tests are skipped
>> instead of incorrectly reporting breakage.
>>
>> Signed-off-by: Jakub Racek <jracek@redhat.com>
>> ---
>> .../kernel/syscalls/memfd_create/memfd_create01.c | 16 ++++++-
>> .../kernel/syscalls/memfd_create/memfd_create02.c | 13 +++++-
>> .../syscalls/memfd_create/memfd_create_common.c | 50 +++++++++++++++++++---
>> .../syscalls/memfd_create/memfd_create_common.h | 19 ++++++--
>> 4 files changed, 86 insertions(+), 12 deletions(-)
>>
>> diff --git a/testcases/kernel/syscalls/memfd_create/memfd_create01.c b/testcases/kernel/syscalls/memfd_create/memfd_create01.c
>> index 54c817b..c5e76f2 100644
>> --- a/testcases/kernel/syscalls/memfd_create/memfd_create01.c
>> +++ b/testcases/kernel/syscalls/memfd_create/memfd_create01.c
>> @@ -22,6 +22,7 @@
>>
>> #define _GNU_SOURCE
>>
>> +#include <errno.h>
>> #include "tst_test.h"
>> #include "memfd_create_common.h"
>>
>> @@ -259,7 +260,20 @@ static void verify_memfd_create(unsigned int n)
>>
>> static void setup(void)
>> {
>> - ASSERT_HAVE_MEMFD_CREATE();
>> +
>> + int available_flags;
>> +
>> + /*
>> + * For now, all tests in this file require MFD_ALLOW_SEALING flag
>> + * to be implemented, even though that flag isn't always set when
>> + * memfd is created. So don't check anything else and TCONF right away
>> + * is this flag is missing.
>> + */
>> + available_flags = GET_MFD_ALL_AVAILABLE_FLAGS();
>> + if ((available_flags & MFD_ALLOW_SEALING) != MFD_ALLOW_SEALING) {
>> + tst_brk(TCONF | TTERRNO,
>> + "memfd_create(%u) not implemented", MFD_ALLOW_SEALING);
>> + }
>
> Can't we call the IS_MFD_AVAILABLE_WITH_FLAGS(flag) here directly
> instead? That would be more straightforward way to figure out if we
> support sealing.
>
> Also better name would be MFD_FLAGS_AVAILABLE() or something and the
> function name to get all available flags should be shorther and to the
> point as well, maybe MFD_ALL_AVAILABLE_FLAGS().
>
>> }
>>
>> static struct tst_test test = {
>> diff --git a/testcases/kernel/syscalls/memfd_create/memfd_create02.c b/testcases/kernel/syscalls/memfd_create/memfd_create02.c
>> index d65e496..bc01e44 100644
>> --- a/testcases/kernel/syscalls/memfd_create/memfd_create02.c
>> +++ b/testcases/kernel/syscalls/memfd_create/memfd_create02.c
>> @@ -31,6 +31,8 @@
>> static char buf[2048];
>> static char term_buf[2048];
>>
>> +static int available_flags;
>> +
>> static const struct tcase {
>> char *descr;
>> char *memfd_name;
>> @@ -62,7 +64,8 @@ static const struct tcase {
>>
>> static void setup(void)
>> {
>> - ASSERT_HAVE_MEMFD_CREATE();
>> +
>> + available_flags = GET_MFD_ALL_AVAILABLE_FLAGS();
>>
>> memset(buf, 0xff, sizeof(buf));
>>
>> @@ -73,8 +76,16 @@ static void setup(void)
>> static void verify_memfd_create_errno(unsigned int n)
>> {
>> const struct tcase *tc;
>> + int needed_flags;
>>
>> tc = &tcases[n];
>> + needed_flags = tc->flags & FLAGS_ALL_MASK;
>> +
>> + if ((available_flags & needed_flags) != needed_flags) {
>> + tst_res(TCONF, "test '%s' skipped, flag not implemented",
>> + tc->descr);
>> + return;
>> + }
>>
>> TEST(sys_memfd_create(tc->memfd_name, tc->flags));
>> if (TEST_ERRNO != tc->memfd_create_exp_err)
>> diff --git a/testcases/kernel/syscalls/memfd_create/memfd_create_common.c b/testcases/kernel/syscalls/memfd_create/memfd_create_common.c
>> index 4cf3bd5..939b046 100644
>> --- a/testcases/kernel/syscalls/memfd_create/memfd_create_common.c
>> +++ b/testcases/kernel/syscalls/memfd_create/memfd_create_common.c
>> @@ -103,20 +103,56 @@ void check_ftruncate_fail(const char *filename, const int lineno,
>> "ftruncate(%d, %ld) failed as expected", fd, length);
>> }
>>
>> -void assert_have_memfd_create(const char *filename, const int lineno)
>> +int get_mfd_all_available_flags(const char *filename, const int lineno)
>> {
>> - TEST(sys_memfd_create("dummy_call", 0));
>> + unsigned int i;
>> + int flag;
>> + int flags2test[] = FLAGS_ALL_ARRAY_INITIALIZER;
>> + int flags_available = 0;
>> + int any_available = 0;
>> +
>> + if (!IS_MFD_AVAILABLE_WITH_FLAGS(0)) {
>> + tst_brk_(filename, lineno, TCONF,
>> + "memfd_create(0) not implemented");
>> + }
>> +
>> + for (i = 0; i < ARRAY_SIZE(flags2test); i++) {
>> + flag = flags2test[i];
>> +
>> + TEST(IS_MFD_AVAILABLE_WITH_FLAGS(flag));
>
> Since we check errno in the is_mfd_...() function and call tst_brk_() on
> failure there is no point in doing anything else here than:
>
> if (is_mfd_...(flag))
> flags_available |= flag;
>
>> + if (!TEST_RETURN) {
>> + tst_res_(filename, lineno, TINFO | TTERRNO,
>> + "memfd_create(%u) not implemented", flag);
>> + } else {
>> + flags_available |= flag;
>> + any_available = 1;
>> + }
>> + }
>> +
>> + if (!any_available) {
>> + tst_brk_(filename, lineno, TCONF,
>> + "memfd_create() not implemented at all");
>> + }
>
> Isn't this check useless now? Since we assert that at least 0 passed as
> flags is supported at the start of this function we should just return
> flags_available here which would be 0 in this case.
>
>> + return flags_available;
>> +}
>> +
>> +int is_mfd_available_with_flags(const char *filename, const int lineno,
>> + unsigned int flags)
>> +{
>> + TEST(sys_memfd_create("dummy_call", flags));
>> if (TEST_RETURN < 0) {
>> - if (TEST_ERRNO == EINVAL) {
>> - tst_brk_(filename, lineno, TCONF | TTERRNO,
>> - "memfd_create() not implemented");
>> + if (TEST_ERRNO != EINVAL) {
>> + tst_brk_(filename, lineno, TBROK | TTERRNO,
>> + "memfd_create() failed");
>> }
>>
>> - tst_brk_(filename, lineno, TBROK | TTERRNO,
>> - "memfd_create() failed");
>> + return 0;
>> }
>>
>> SAFE_CLOSE(TEST_RETURN);
>> +
>> + return 1;
>> }
>>
>> int check_mfd_new(const char *filename, const int lineno,
>> diff --git a/testcases/kernel/syscalls/memfd_create/memfd_create_common.h b/testcases/kernel/syscalls/memfd_create/memfd_create_common.h
>> index 6329ac3..616c7ef 100644
>> --- a/testcases/kernel/syscalls/memfd_create/memfd_create_common.h
>> +++ b/testcases/kernel/syscalls/memfd_create/memfd_create_common.h
>> @@ -20,10 +20,19 @@
>> #include <lapi/fcntl.h>
>> #include <lapi/memfd.h>
>>
>> +#define COMMA ,
>> +#define BITWISE_OR |
>> +#define OPERATE_ON_FLAGS(x) MFD_CLOEXEC x MFD_ALLOW_SEALING
>> +#define FLAGS_ALL_ARRAY_INITIALIZER {OPERATE_ON_FLAGS(COMMA)}
>> +#define FLAGS_ALL_MASK (OPERATE_ON_FLAGS(BITWISE_OR))
>
> Really?
>
> Do you realize that just defining the FLAGS_ALL_MASK and array
> initializer directly as MFD_CLOEXEC | MFD_ALLOW_SEALING and
> {MFD_CLOEXEC, MFD_ALLOW_SEALING} is not only less complex but shorther
> as well?
>
It depends on what you see as simpler. I tried to avoid defining the
same flags on two lines (Single Point of Definition). If API ever
changes (e.g. more flags get added), then someone may easily change the
first line, but forget the second. But I get your point and I've added a
comment instead.
Thank you for your comments. Sending V3 now.
> What do we need this macro obscurity for?
>
>> #define MFD_DEF_SIZE 8192
>>
>> -#define ASSERT_HAVE_MEMFD_CREATE() \
>> - assert_have_memfd_create(__FILE__, __LINE__)
>> +#define GET_MFD_ALL_AVAILABLE_FLAGS() \
>> + get_mfd_all_available_flags(__FILE__, __LINE__)
>> +
>> +#define IS_MFD_AVAILABLE_WITH_FLAGS(flags) \
>> + is_mfd_available_with_flags(__FILE__, __LINE__, (flags))
>>
>> #define CHECK_MFD_NEW(name, sz, flags) \
>> check_mfd_new(__FILE__, __LINE__, (name), (sz), (flags))
>> @@ -89,7 +98,11 @@
>> #define CHECK_MFD_NON_GROWABLE_BY_WRITE(fd) \
>> check_mfd_non_growable_by_write(__FILE__, __LINE__, (fd))
>>
>> -void assert_have_memfd_create(const char *filename, const int lineno);
>> +int is_mfd_available_with_flags(const char *filename, const int lineno,
>> + unsigned int flags);
>> +
>> +int get_mfd_all_available_flags(const char *filename, const int lineno);
>> +
>> int sys_memfd_create(const char *name, unsigned int flags);
>>
>> int check_fallocate(const char *filename, const int lineno, int fd,
>> --
>> 1.8.3.1
>>
>>
>> --
>> Mailing list info: https://lists.linux.it/listinfo/ltp
>
--
Regards,
Jakub
More information about the ltp
mailing list