| Message ID | 1586429086-22975-2-git-send-email-xuyang2018.jy@cn.fujitsu.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,01/10] lapi/loop.h: Add fallback for loop ioctl and flag | expand |
Hi! > +/* > + * Copyright (c) 2020 FUJITSU LIMITED. All rights reserved. > + * Author: Yang Xu <xuyang2018.jy@cn.fujitsu.com> > + */ > +#define TST_NO_DEFAULT_MAIN > +#include "ioctl_loop_support.h" > +#include "tst_test.h" > + > +void check_sys_value(char *path, int setvalue) > +{ > + int getvalue; > + > + SAFE_FILE_SCANF(path, "%d", &getvalue); > + if (setvalue == getvalue) > + tst_res(TPASS, "%s value is %d", path, setvalue); > + else > + tst_res(TFAIL, "%s value expected %d got %d", path, setvalue, getvalue); > +} > + > +void check_sys_string(char *path, char *setmessage) > +{ > + char getmessage[1024]; > + > + SAFE_FILE_SCANF(path, "%s", getmessage); > + if (strcmp(setmessage, getmessage)) > + tst_res(TFAIL, "%s expected %s got %s", path, setmessage, getmessage); > + else > + tst_res(TPASS, "%s string is %s", path, getmessage); > +} In the end I've renamed and moved these functions into the test library because the functionality is generic enough and I doubt that these tests would be the only one using it.
Hi Cyril > Hi! >> +/* >> + * Copyright (c) 2020 FUJITSU LIMITED. All rights reserved. >> + * Author: Yang Xu <xuyang2018.jy@cn.fujitsu.com> >> + */ >> +#define TST_NO_DEFAULT_MAIN >> +#include "ioctl_loop_support.h" >> +#include "tst_test.h" >> + >> +void check_sys_value(char *path, int setvalue) >> +{ >> + int getvalue; >> + >> + SAFE_FILE_SCANF(path, "%d", &getvalue); >> + if (setvalue == getvalue) >> + tst_res(TPASS, "%s value is %d", path, setvalue); >> + else >> + tst_res(TFAIL, "%s value expected %d got %d", path, setvalue, getvalue); >> +} >> + >> +void check_sys_string(char *path, char *setmessage) >> +{ >> + char getmessage[1024]; >> + >> + SAFE_FILE_SCANF(path, "%s", getmessage); >> + if (strcmp(setmessage, getmessage)) >> + tst_res(TFAIL, "%s expected %s got %s", path, setmessage, getmessage); >> + else >> + tst_res(TPASS, "%s string is %s", path, getmessage); >> +} > > In the end I've renamed and moved these functions into the test library > because the functionality is generic enough and I doubt that these > tests would be the only one using it. That's great. I remember prctl cases use this function. Also, in some cap cases, it needs bitwise operators(I only know prctl08.c). Maybe we can add TST_ASSERT_BITWISE? >
Hi Cyril > Hi Cyril > > >> Hi! >>> +/* >>> + * Copyright (c) 2020 FUJITSU LIMITED. All rights reserved. >>> + * Author: Yang Xu <xuyang2018.jy@cn.fujitsu.com> >>> + */ >>> +#define TST_NO_DEFAULT_MAIN >>> +#include "ioctl_loop_support.h" >>> +#include "tst_test.h" >>> + >>> +void check_sys_value(char *path, int setvalue) >>> +{ >>> + int getvalue; >>> + >>> + SAFE_FILE_SCANF(path, "%d", &getvalue); >>> + if (setvalue == getvalue) >>> + tst_res(TPASS, "%s value is %d", path, setvalue); >>> + else >>> + tst_res(TFAIL, "%s value expected %d got %d", path, >>> setvalue, getvalue); >>> +} >>> + >>> +void check_sys_string(char *path, char *setmessage) >>> +{ >>> + char getmessage[1024]; >>> + >>> + SAFE_FILE_SCANF(path, "%s", getmessage); >>> + if (strcmp(setmessage, getmessage)) >>> + tst_res(TFAIL, "%s expected %s got %s", path, setmessage, >>> getmessage); >>> + else >>> + tst_res(TPASS, "%s string is %s", path, getmessage); >>> +} >> >> In the end I've renamed and moved these functions into the test library >> because the functionality is generic enough and I doubt that these >> tests would be the only one using it. > That's great. I remember prctl cases use this function. Also, in some > cap cases, it needs bitwise operators(I only know prctl08.c). Maybe we > can add TST_ASSERT_BITWISE? This situation is rarely seen. I want to add two functions such as TST_ASSERT_FILE_STR/INT to compare file field string with expected string. So, I can use this api for prctl NoNewPrivs and Seccomp field in /proc/[pid]/status. What do you think about this? --- a/lib/tst_assert.c +++ b/lib/tst_assert.c @@ -22,6 +22,20 @@ void tst_assert_int(const char *file, const int lineno, const char *path, int va tst_res_(file, lineno, TFAIL, "%s != %d got %d", path, val, sys_val); } +void tst_assert_file_int(const char *file, const int lineno, const char *path, const char *buf, int val) +{ + int sys_val; + + SAFE_FILE_LINES_SCANF(path, buf, &sys_val); + + if (val == sys_val) { + tst_res_(file, lineno, TPASS, "%s %s field = %d", path, buf, sys_val); + return; + } + + tst_res_(file, lineno, TFAIL, "%s %s field != %d got %d", path, buf, val, sys_val); +} + void tst_assert_str(const char *file, const int lineno, const char *path, const char *val) { char sys_val[1024]; @@ -34,3 +48,17 @@ void tst_assert_str(const char *file, const int lineno, const char *path, const tst_res_(file, lineno, TFAIL, "%s != '%s' got '%s'", path, val, sys_val); } + +void tst_assert_file_str(const char *file, const int lineno, const char *path, const char *buf, const char *val) +{ + char sys_val[1024]; + + SAFE_FILE_LINES_SCANF(path, buf, sys_val); + + if (!strcmp(val, sys_val)) { + tst_res_(file, lineno, TPASS, "%s %s field = %s", path, buf, sys_val); + return; + } + + tst_res_(file, lineno, TFAIL, "%s %s field != %s got %s", path, buf, val, sys_val); +} Best Regards Yang Xu >>
Hi! > > In the end I've renamed and moved these functions into the test library > > because the functionality is generic enough and I doubt that these > > tests would be the only one using it. > That's great. I remember prctl cases use this function. Also, in some > cap cases, it needs bitwise operators(I only know prctl08.c). Maybe we > can add TST_ASSERT_BITWISE? I guess that we would need to pass to numbers to the bitwise operation, one would be mask for which bits should be used for the comparsion and one with the actual bits. Which tests would need that? I looked at capset and capget tests but there does not seem to any bitwise checks on values on proc or sys files.
Hi! > +void tst_assert_file_str(const char *file, const int lineno, const char > *path, const char *buf, const char *val) > +{ > + char sys_val[1024]; > + > + SAFE_FILE_LINES_SCANF(path, buf, sys_val); We should call the file_lines_scanf() here and pass the filename and lineno that this functions takes as parameter instead. Also I'm not sure that it's a good idea to pass the fmt to this function. I guess that it would be better to pass a prefix string such as "foo=" and to append the "%s" in this function. That way we could also encode the buffer length to the fmt string as well, in that case we would append "%1024s". > + if (!strcmp(val, sys_val)) { > + tst_res_(file, lineno, TPASS, "%s %s field = %s", path, > buf, sys_val); > + return; > + } > + > + tst_res_(file, lineno, TFAIL, "%s %s field != %s got %s", path, > buf, val, sys_val); > +}
Hi Cyril > Hi! >> +void tst_assert_file_str(const char *file, const int lineno, const char >> *path, const char *buf, const char *val) >> +{ >> + char sys_val[1024]; >> + >> + SAFE_FILE_LINES_SCANF(path, buf, sys_val); > > We should call the file_lines_scanf() here and pass the filename and > lineno that this functions takes as parameter instead. I don't understand the meaning of adding filename and line parameter(If fail, it will report tst_assert.c: 29). Do you want to report the correct parsed filename when failed? If so, I think lineno is meaningless and it also overides the place of the code meet problem . I prefer to add some path info in safe_file_ops.c. > > Also I'm not sure that it's a good idea to pass the fmt to this > function. I guess that it would be better to pass a prefix string such > as "foo=" and to append the "%s" in this function. That way we could > also encode the buffer length to the fmt string as well, in that case we > would append "%1024s". > Yes, it is a better way. ps: I will send a patch about this api. Best Regards Yang Xu >> + if (!strcmp(val, sys_val)) { >> + tst_res_(file, lineno, TPASS, "%s %s field = %s", path, >> buf, sys_val); >> + return; >> + } >> + >> + tst_res_(file, lineno, TFAIL, "%s %s field != %s got %s", path, >> buf, val, sys_val); >> +} > > >
Hi! > >> +void tst_assert_file_str(const char *file, const int lineno, const char > >> *path, const char *buf, const char *val) > >> +{ > >> + char sys_val[1024]; > >> + > >> + SAFE_FILE_LINES_SCANF(path, buf, sys_val); > > > > We should call the file_lines_scanf() here and pass the filename and > > lineno that this functions takes as parameter instead. > I don't understand the meaning of adding filename and line parameter(If > fail, it will report tst_assert.c: 29). Do you want to report the > correct parsed filename when failed? > > If so, I think lineno is meaningless and it also overides the place of > the code meet problem . I prefer to add some path info in safe_file_ops.c. What I want is to show the test source filename and line on failure, for that we have to pass the lineno and file to the file_lines_scanf() function. I think that it makes much more sense to print the line in a test where the problem has happened rather than some location in the library code.
Hi Cyril > Hi! >>>> +void tst_assert_file_str(const char *file, const int lineno, const char >>>> *path, const char *buf, const char *val) >>>> +{ >>>> + char sys_val[1024]; >>>> + >>>> + SAFE_FILE_LINES_SCANF(path, buf, sys_val); >>> >>> We should call the file_lines_scanf() here and pass the filename and >>> lineno that this functions takes as parameter instead. >> I don't understand the meaning of adding filename and line parameter(If >> fail, it will report tst_assert.c: 29). Do you want to report the >> correct parsed filename when failed? >> >> If so, I think lineno is meaningless and it also overides the place of >> the code meet problem . I prefer to add some path info in safe_file_ops.c. > > What I want is to show the test source filename and line on failure, for > that we have to pass the lineno and file to the file_lines_scanf() > function. I think that it makes much more sense to print the line in a > test where the problem has happened rather than some location in the > library code. Oh, I see. ie TST_ASSERT_INT, modify ioctl_loop01.c TST_ASSERT_INT(__FILE__, __LINE__, partscan_path, 0); if partscan_path doesn't exist, it will report error as below: safe_file_ops.c:142: BROK: Failed to open FILE '/sys/block/loop0/loop1/partscan' for reading at ioctl_loop01.c:46: ENOENT (2) --- a/lib/tst_assert.c +++ b/lib/tst_assert.c -void tst_assert_int(const char *file, const int lineno, const char *path, int val) +void tst_assert_int(const char *file, const int lineno, const char *source_path, + const int source_pos, const char *path, int val) { int sys_val; - SAFE_FILE_SCANF(path, "%d", &sys_val); + safe_file_scanf(source_path, source_pos, NULL, path, "%d", &sys_val diff --git a/include/tst_assert.h b/include/tst_assert.h index 913fff1b5..03e57bbd9 100644 --- a/include/tst_assert.h +++ b/include/tst_assert.h @@ -7,8 +7,8 @@ #ifndef TST_ASSERT_H__ #define TST_ASSERT_H__ -#define TST_ASSERT_INT(path, val) \ - tst_assert_int(__FILE__, __LINE__, path, val) +#define TST_ASSERT_INT(source_path, source_pos, path, val) \ + tst_assert_int(__FILE__, __LINE__, source_path, source_pos, path, val) /* * Asserts that integer value stored in file pointed by path equals to the @@ -16,6 +16,7 @@ * values in sysfs, procfs, etc. */ void tst_assert_int(const char *file, const int lineno, + const char *source_path, const int source_pos, const char *path, int val); #define TST_ASSERT_FILE_INT(path, buf, val) >
Hi! > >>>> +void tst_assert_file_str(const char *file, const int lineno, const char > >>>> *path, const char *buf, const char *val) > >>>> +{ > >>>> + char sys_val[1024]; > >>>> + > >>>> + SAFE_FILE_LINES_SCANF(path, buf, sys_val); > >>> > >>> We should call the file_lines_scanf() here and pass the filename and > >>> lineno that this functions takes as parameter instead. > >> I don't understand the meaning of adding filename and line parameter(If > >> fail, it will report tst_assert.c: 29). Do you want to report the > >> correct parsed filename when failed? > >> > >> If so, I think lineno is meaningless and it also overides the place of > >> the code meet problem . I prefer to add some path info in safe_file_ops.c. > > > > What I want is to show the test source filename and line on failure, for > > that we have to pass the lineno and file to the file_lines_scanf() > > function. I think that it makes much more sense to print the line in a > > test where the problem has happened rather than some location in the > > library code. > Oh, I see. ie TST_ASSERT_INT, modify ioctl_loop01.c > TST_ASSERT_INT(__FILE__, __LINE__, partscan_path, 0); > if partscan_path doesn't exist, it will report error as below: > safe_file_ops.c:142: BROK: Failed to open FILE > '/sys/block/loop0/loop1/partscan' for reading at ioctl_loop01.c:46: > ENOENT (2) I still think that we are not on the same page. The macros are to be used from tests, such as TST_ASSERT_INT() these macros call the corresponding functions with __FILE__ and __LINE__, in this case tst_assert_int(). From that point on anything that is called from inside of the tst_assert_int() function should pass down the file and lineno so that we get the filename and lineno from the line the call has been called in the test. So if we do TST_ASSERT_INT() in the test, we call tst_brk_() and pass the file and lineno so that we get the correct test line in case of a failure. And for the same reason if we are going to get an value from a file from inside of the assert function we have to call the safe_file_scanf() and pass down the file and lineno so that if the call fails the test filename and the line on which the call originated in the test is printed. Is it clear now?
Hi Cyril > Hi! >>>>>> +void tst_assert_file_str(const char *file, const int lineno, const char >>>>>> *path, const char *buf, const char *val) >>>>>> +{ >>>>>> + char sys_val[1024]; >>>>>> + >>>>>> + SAFE_FILE_LINES_SCANF(path, buf, sys_val); >>>>> >>>>> We should call the file_lines_scanf() here and pass the filename and >>>>> lineno that this functions takes as parameter instead. >>>> I don't understand the meaning of adding filename and line parameter(If >>>> fail, it will report tst_assert.c: 29). Do you want to report the >>>> correct parsed filename when failed? >>>> >>>> If so, I think lineno is meaningless and it also overides the place of >>>> the code meet problem . I prefer to add some path info in safe_file_ops.c. >>> >>> What I want is to show the test source filename and line on failure, for >>> that we have to pass the lineno and file to the file_lines_scanf() >>> function. I think that it makes much more sense to print the line in a >>> test where the problem has happened rather than some location in the >>> library code. >> Oh, I see. ie TST_ASSERT_INT, modify ioctl_loop01.c >> TST_ASSERT_INT(__FILE__, __LINE__, partscan_path, 0); >> if partscan_path doesn't exist, it will report error as below: >> safe_file_ops.c:142: BROK: Failed to open FILE >> '/sys/block/loop0/loop1/partscan' for reading at ioctl_loop01.c:46: >> ENOENT (2) > > I still think that we are not on the same page. > > The macros are to be used from tests, such as TST_ASSERT_INT() these > macros call the corresponding functions with __FILE__ and __LINE__, in > this case tst_assert_int(). From that point on anything that is called > from inside of the tst_assert_int() function should pass down the file > and lineno so that we get the filename and lineno from the line the call > has been called in the test. > > So if we do TST_ASSERT_INT() in the test, we call tst_brk_() and pass > the file and lineno so that we get the correct test line in case of a > failure. > > And for the same reason if we are going to get an value from a file from > inside of the assert function we have to call the safe_file_scanf() and > pass down the file and lineno so that if the call fails the test > filename and the line on which the call originated in the test is > printed. > > Is it clear now? OK. I guess I only need to use safe_file_scanf(file, lineno, NULL, path, "%d", &sys_val) in tst_assert_int function as you suggested at the beginning. >
diff --git a/testcases/kernel/syscalls/ioctl/Makefile b/testcases/kernel/syscalls/ioctl/Makefile index c2ff6c8e7..7bdc7daf0 100644 --- a/testcases/kernel/syscalls/ioctl/Makefile +++ b/testcases/kernel/syscalls/ioctl/Makefile @@ -7,6 +7,11 @@ include $(top_srcdir)/include/mk/testcases.mk INSTALL_TARGETS += test_ioctl +FILTER_OUT_MAKE_TARGETS += ioctl_loop_support + +LOOP_TARGETS := $(patsubst $(abs_srcdir)/%.c,%,$(wildcard $(abs_srcdir)/ioctl_loop[0-9]*.c)) +$(LOOP_TARGETS): %: ioctl_loop_support.o + ifeq ($(ANDROID),1) FILTER_OUT_MAKE_TARGETS += ioctl02 endif diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop_support.c b/testcases/kernel/syscalls/ioctl/ioctl_loop_support.c new file mode 100644 index 000000000..75c7b1b9a --- /dev/null +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop_support.c @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2020 FUJITSU LIMITED. All rights reserved. + * Author: Yang Xu <xuyang2018.jy@cn.fujitsu.com> + */ +#define TST_NO_DEFAULT_MAIN +#include "ioctl_loop_support.h" +#include "tst_test.h" + +void check_sys_value(char *path, int setvalue) +{ + int getvalue; + + SAFE_FILE_SCANF(path, "%d", &getvalue); + if (setvalue == getvalue) + tst_res(TPASS, "%s value is %d", path, setvalue); + else + tst_res(TFAIL, "%s value expected %d got %d", path, setvalue, getvalue); +} + +void check_sys_string(char *path, char *setmessage) +{ + char getmessage[1024]; + + SAFE_FILE_SCANF(path, "%s", getmessage); + if (strcmp(setmessage, getmessage)) + tst_res(TFAIL, "%s expected %s got %s", path, setmessage, getmessage); + else + tst_res(TPASS, "%s string is %s", path, getmessage); +} + +void check_support_cmd(int dev_fd, int ioctl_flag, int value, char *message) +{ + int ret = 0; + + ret = ioctl(dev_fd, ioctl_flag, value); + if (ret && errno == EINVAL) + tst_brk(TCONF, "Current environment doesn't support this flag(%s)", + message); +} diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop_support.h b/testcases/kernel/syscalls/ioctl/ioctl_loop_support.h new file mode 100644 index 000000000..ade64e82e --- /dev/null +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop_support.h @@ -0,0 +1,12 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2020 FUJITSU LIMITED. All rights reserved. + * Author: Yang Xu <xuyang2018.jy@cn.fujitsu.com> + */ +#ifndef IOCTL_LOOP_H +#define IOCTL_lOOP_H +#include <linux/loop.h> +void check_sys_value(char *path, int setvalue); +void check_sys_string(char *path, char *setmessage); +void check_support_cmd(int dev_fd, int ioctl_flag, int value, char *message); +#endif
Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> --- testcases/kernel/syscalls/ioctl/Makefile | 5 +++ .../syscalls/ioctl/ioctl_loop_support.c | 40 +++++++++++++++++++ .../syscalls/ioctl/ioctl_loop_support.h | 12 ++++++ 3 files changed, 57 insertions(+) create mode 100644 testcases/kernel/syscalls/ioctl/ioctl_loop_support.c create mode 100644 testcases/kernel/syscalls/ioctl/ioctl_loop_support.h