From mboxrd@z Thu Jan 1 00:00:00 1970 From: Richard Palethorpe Date: Mon, 31 Jul 2017 14:17:48 +0200 Subject: [LTP] [PATCH v4 1/2] tutorial: Add a step-by-step C test tutorial Message-ID: <20170731121749.31271-1-rpalethorpe@suse.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: ltp@lists.linux.it A tutorial for writing a complete, if simple test. Signed-off-by: Richard Palethorpe --- V4 - Use lapi/syscalls.h doc/c-test-tutorial-simple.txt | 1096 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 1096 insertions(+) create mode 100644 doc/c-test-tutorial-simple.txt diff --git a/doc/c-test-tutorial-simple.txt b/doc/c-test-tutorial-simple.txt new file mode 100644 index 000000000..4d5710dda --- /dev/null +++ b/doc/c-test-tutorial-simple.txt @@ -0,0 +1,1096 @@ +C Test Case Tutorial +==================== + +This is a step-by-step tutorial on writing a simple C LTP test, where topics +of the LTP and Linux kernel testing will be introduced gradually using a +concrete example. Most sections will include exercises, some trivial and +others not so much. If you find an exercise is leading you off at too much of +a tangent, just leave it for later and move on. + +LTP tests can be written in C or Shell script. This tutorial is only for tests +written in C using the new LTP test API. Note that while we go into some +detail on using Git, this is not intended as a canonical or complete guide +for Git. + +0. Assumptions & Feedback +------------------------- + +We assume the reader is familiar with C, Git and common Unix/Linux/GNU tools +and has some general knowledge of Operating Systems. Experienced Linux +developers may find it too verbose while people new to system level Linux +development may find it overwhelming. + +Comments and feedback are welcome, please direct them to the mailing list (see ++README+). + +1. Getting Started +------------------ + +Git-clone the main LTP repository as described in the +README+ and change +directory to the checked-out Git repository. We recommend installing the LTP +and running one of the tests mentioned in the Quick guide (in the +README+) to +ensure you are starting from a good state. + +We also recommended cloning the Linux kernel repository for reference, this +guide will refer to files and directories within the mainline kernel 4.12. + +[source,shell] +------------------------------------------------------------------------------ +$ git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git +------------------------------------------------------------------------------ + +There are a number of other repositories which are useful for reference as +well, including the GNU C library +glibc+ and the alternative C library ++musl+. Some system calls are partially or even entirely implemented in user +land as part of the standard C library. So in these cases, the C library is an +important reference. +glibc+ is the most common C library for Linux, however ++musl+ is generally easier to understand. + +How system calls are implemented varies from one architecture to another and +across kernel and C library versions. To find out whether a system call is +actually accessing the kernel (whether it is actually a system call) on any +given machine you can use the +strace+ utility. This intercepts system calls +made by an executable and prints them. We will use this later in the tutorial. + +2. Choose a System Call to test +------------------------------- + +We will use the +statx()+ system call, to provide a concrete example of a +test. At the time of writing there is no test for this call which was +introduced in Linux kernel version 4.11. + +Linux system call specific tests are primarily contained in ++testcases/kernel/syscalls+, but you should also +git grep+ the entire LTP +repository to check for any existing usages of a system call. + +One way to find a system call which is not currently tested by the LTP is to +look at +include/linux/syscalls.h+ in the kernel tree. + +Something the LTP excels at is ensuring bug-fixes are back ported to +maintenance releases, so targeting a specific regression is another +option. + +2.1. Find an untested System call +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Try to find an untested system call which has a manual page (i.e. +man +syscall+ produces a result). It is a good idea to Git-clone the latest kernel +man-pages repository. + +[source,shell] +------------------------------------------------------------------------------ +$ git clone git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git +------------------------------------------------------------------------------ + +At the time of writing the difference between the latest man-pages release and +the +HEAD+ of the repository (usually the latest commit) is well over 100 +commits. This represents about 9 weeks of changes. If you are using a stable +Linux distribution, your man-pages package may well be years old. So as with +the kernel, it is best to have the Git repository as a reference. + +You could also find a system call with untested parameters or use whatever it +is you are planning to use the LTP for. + +3. Create the test skeleton +--------------------------- + +I shall call my test +statx01.c+, by the time you read this that file name +will probably be taken, so increment the number in the file name as +appropriate or replace +statx+ with the system call chosen in exercise 2.1. + +[source,shell] +------------------------------------------------------------------------------ +$ mkdir testcases/kernel/syscalls/statx +$ cd testcases/kernel/syscalls/statx +$ echo statx >> .gitignore +------------------------------------------------------------------------------ + +Next open +statx01.c+ and add the following boilerplate. Make sure to change +the copy right notice to your name/company, correct the test name and minimum +kernel version if necessary. I will explain what the code does below. + +[source,c] +------------------------------------------------------------------------------ +/* + * Copyright (c) 2017 Instruction Ignorer <"can't"@be.bothered.com> + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +/* + * Test statx + * + * All tests should start with a description of _what_ we are testing. + * Non-trivial explanations of _how_ the code works should also go here. + * Include relevant links, Git commit hashes and CVE numbers. + * Inline comments should be avoided. + */ + +#include "tst_test.h" + +static void run(void) +{ + tst_res(TPASS, "Doing hardly anything is easy"); +} + +static struct tst_test test = { + .test_all = run, + .min_kver = "4.11", +}; +------------------------------------------------------------------------------ + +Starting with the +#include+ statement we copy in the main LTP test library +headers. This includes the most common test API functions and the test harness +initialisation code. It is important to note that this is a completely +ordinary, independent C program, however +main()+ is missing because it is +implemented in +tst_test.h+. + +We specify what code we want to run as part of the test using the +tst_test +test+ structure. Various callbacks can be set by the test writer, including ++test.test_all+, which we have set to +run()+. The test harness will execute +this callback in a separate process (using +fork()+), forcibly terminating it +if it does not return after +test.timeout+ seconds. + +We have also set +test.min_kver+ to the kernel version where +statx+ was +introduced. The test library will determine the kernel version at runtime. If +the version is less than 4.11 then the test harness will return +TCONF+, +indicating that this test is not suitable for the current system +configuration. + +Occasionally features are back ported to older kernel versions, so +statx+ may +exist on kernels with a lower version. However we don't need to worry about +that unless there is evidence of it happening. + +As mentioned in the code itself, you should specify what you are testing and +the expected outcome, even if it is relatively simple. If your program flow is +necessarily complex and difficult to understand (which is often the case when +trying to manipulate the kernel into doing something bad), then a detailed +explanation of how the code works is welcome. + +What you should not do, is use inline comments or include the same level of +explanation which is written here. As a general rule, if something is easy to +document, then the code should also be easy to read. So don't document the easy +stuff (except for the basic test specification). + +Before continuing we should compile this and check that the basics work. In +order to compile the test we need a +Makefile+ in the same subdirectory. If +one already exists, then nothing needs to be done, otherwise add one with the +following contents. + +[source,make] +------------------------------------------------------------------------------ +# Copyright (c) 2017 Linux Test Project +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation; either version 2 of +# the License, or (at your option) any later version. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +top_srcdir ?= ../../../.. + +include $(top_srcdir)/include/mk/testcases.mk + +include $(top_srcdir)/include/mk/generic_leaf_target.mk + +------------------------------------------------------------------------------ + +This will automatically add +statx01.c+ as a build target producing a ++statx01+ executable. Unless you have heavily deviated from the tutorial, and +probably need to change +top_srcdir+, nothing else needs to be done. + +Normally, if you were starting a Makefile from scratch, then you would need to +add +statx01+ as a build target. Specifying that you would like to run some +program (e.g. +gcc+ or +clang+) to transform +statx01.c+ into +statx01+. Here +we don't need to do that, but sometimes it is still necessary. For example, if +we needed to link to the POSIX threading library, then we could add the +following line after +testcases.mk+. + +[source,make] +-------------------------------------------------------------------------------- +statx01: CFLAGS += -pthread +-------------------------------------------------------------------------------- + +Assuming you are in the test's subdirectory +testcases/kernel/syscalls/statx+, +do + +[source,shell] +-------------------------------------------------------------------------------- +$ make +$ ./statx01 +-------------------------------------------------------------------------------- + +This should build the test and then run it. However, even though the test is +in the +syscalls+ directory it won't be automatically ran as part of the +_syscalls_ test group (remember +./runltp -f syscalls+ from the +README+?). For +this we need to add it to the +runtest+ file. So open +runtest/statx+ and add +the lines starting with a +++. + +[source,diff] +-------------------------------------------------------------------------------- + statvfs01 statvfs01 + statvfs02 statvfs02 + ++statx01 statx01 ++ + stime01 stime01 + stime02 stime02 + +-------------------------------------------------------------------------------- + +The +runtest+ files are in a two column format. The first column is the test +name, which is mainly used by test runners for reporting and filtering. It is +just a single string of text with no spaces. The second column, which can +contain spaces, is passed to the shell in order to execute the test. Often it +is just the executable name, but some tests also take arguments (the LTP has a +library for argument parsing, by the way). + +If you haven't done so already, we should add all these new files to Git. It +is vitally important that you do not make changes to the master branch. If you +do then pulling changes from upstream becomes a major issue. So first of all +create a new branch. + +[source,shell] +-------------------------------------------------------------------------------- +$ git checkout -b statx01 master +-------------------------------------------------------------------------------- + +Now we want to add the files we have created or modified, but before doing a +commit make sure you have configured Git correctly. You need to at least set +your Name and e-mail address in +~/.gitconfig+, but there are some other +settings which come in handy too. My relatively simple configuration is similar to +the below + +[source,conf] +-------------------------------------------------------------------------------- +[user] + name = Sarah Jane + email = sjane@e-mail.address +[core] + editor = emacs +[sendemail] + smtpServer = smtp.server.address +-------------------------------------------------------------------------------- + +Obviously you need to at least change your name and e-mail. The SMTP server is +useful for +git send-mail+, which we will discuss later. The editor value is +used for things like writing commits (without the +-m+ option). + +[source,shell] +-------------------------------------------------------------------------------- +$ git add -v :/testcases/kernel/syscalls/statx :/runtest/syscalls +$ git commit -m "statx01: Add new test for statx syscall" +-------------------------------------------------------------------------------- + +This should add all the new files in the +statx+ directory and the +runtest+ +file. It is good practice to commit early and often. Later on we will do a +Git-rebase, which allows us to clean up the commit history. So don't worry +about how presentable your commit log is for now. Also don't hesitate to +create a new branch when doing the exercises or experimenting. This will allow +you to diverge from the tutorial and then easily come back again. + +I can't emphasize enough that Git makes things easy through branching and that +things quickly get complicated if you don't do it. However if you do get into +a mess, Git-reflog and Git-reset, will usually get you out of it. If you also +mess that up then it may be possible to cherry pick 'dangling' commits out of +the database into a branch. + +3.1 Report TCONF instead of TPASS +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Maybe the test should report "TCONF: Not implemented" instead or perhaps ++TBROK+. Try changing it do so (see +doc/test-writing-guidelines.txt+ or +https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines[the +Wiki]). + +3.2 Check Git ignores the executable +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Is your +.gitignore+ correct? + +3.3 Run checkpatch.pl on the source file +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The LTP follows the Linux style guidelines where possible. Check what happens +if you run +kernel/linux/scripts/checkpatch.pl --no-tree -f statx01.c+ and +correct any style issues. + +3.4 Install the LTP and run the test with runtest +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Run +statx01+ on its own; similar to the +madvise+ tests in the +README+. + +4. Call the system call +----------------------- + +At the time of writing +statx+ has no +glibc+ wrapper. It is also fairly common +for a distribution's C library version to be older than its kernel or it may use a +cut down C library in comparison to the GNU one. So we must call +statx()+ +using the general +syscall()+ interface. + +The LTP contains a library for dealing with the +syscall+ interface, which is +located in +include/lapi+. System call numbers are listed against the relevant +call in the +*.in+ files (e.g. +x86_64.in+) which are used to generate ++syscalls.h+, which is the header you should include. On rare occasions you +may find the system call number is missing from the +*.in+ files and will need +to add it (see include/lapi/syscall/strip_syscall.awk). + +System call numbers vary between architectures, hence why there are multiple ++*.in+ files for each architecture. You can find the various values for the ++statx+ system call across a number of +uinstd.h+ files in the Linux kernel. + +Note that we don't use the system-call-identifier value available in ++/usr/include/linux/uinstd.h+ because the kernel might be much newer than the +user land development packages. + +For +statx+ we had to add +statx 332+ to +testcases/kernel/include/x86_64.in+, ++statx 383+ to +testcases/kernel/include/powerpc.in+, etc. Now lets look at +the code, which I will explain in more detail further down. + +[source,c] +-------------------------------------------------------------------------------- +/* + * Test statx + * + * Check if statx exists and what error code it returns when we give it dodgy + * data. + */ + +#include +#include "tst_test.h" +#include "lapi/syscalls.h" + +struct statx_timestamp { + int64_t tv_sec; + uint32_t tv_nsec; + int32_t __reserved; +}; + +struct statx { + uint32_t stx_mask; + uint32_t stx_blksize; + uint64_t stx_attributes; + uint32_t stx_nlink; + uint32_t stx_uid; + uint32_t stx_gid; + uint16_t stx_mode; + uint16_t __spare0[1]; + uint64_t stx_ino; + uint64_t stx_size; + uint64_t stx_blocks; + uint64_t stx_attributes_mask; + struct statx_timestamp stx_atime; + struct statx_timestamp stx_btime; + struct statx_timestamp stx_ctime; + struct statx_timestamp stx_mtime; + uint32_t stx_rdev_major; + uint32_t stx_rdev_minor; + uint32_t stx_dev_major; + uint32_t stx_dev_minor; + uint64_t __spare2[14]; +}; + +static int sys_statx(int dirfd, const char *pathname, int flags, + unsigned int mask, struct statx *statxbuf) +{ + return tst_syscall(__NR_statx, dirfd, pathname, flags, mask, statxbuf); +} + +... +-------------------------------------------------------------------------------- + +So the top part of the code is now boiler plate for calling +statx+. It is +common for the kernel to be newer than the user land libraries and headers. So +for new system calls like +statx+, we copy, with a few modifications, the +relevant definitions into the LTP. This is somewhat like 'vendoring', although +we are usually just copying headers required for interacting with the Kernel's +ABI (Application Binary Interface), rather than internalising actual +functionality. + +So from the top we include the +stdint.h+ library which gives us the standard ++(u)int*_t+ type definitions. We use these in place of the Kernel type +definitions such as +__u64+ in +linux/types.h+. We then have a couple of +structure definitions which form part of the +statx+ API. These were copied +from +include/uapi/linux/stat.h+ in the Kernel tree. + +After that, there is a wrapper function, which saves us from writing ++tst_syscall(__NR_statx, ...+, every time we want to make a call to ++statx+. This also provides a stub for when +statx+ is eventually integrated +into the LTP library and also implemented by the C library. At that point we +can switch to using the C library implementation if available or fallback to +our own. + +The advantage of using the C library implementation is that it will often be +better supported across multiple architectures. It will also mean we are using +the system call in the same way most real programs would. Sometimes there are +advantages to bypassing the C library, but in general it should not be our +first choice. + +The final test should do a check during configuration (i.e. when we run ++./configure+ before building) which checks if the +statx+ system call and +associated structures exists. This requires writing an +m4+ file for use with ++configure.ac+ which is processed during +make autotools+ and produces the +configure script. + +For the time being though we shall just ignore this. All you need to know for +now is that this is a problem which eventually needs to be dealt with and that +there is a system in place to handle it. + +[source,c] +-------------------------------------------------------------------------------- +... + +static void run(void) +{ + struct statx statxbuf = { 0 }; + + TEST(sys_statx(0, NULL, 0, 0, &statxbuf)); + + if (TEST_RETURN == 0) + tst_res(TFAIL, "statx thinks it can stat NULL"); + else if (TEST_ERRNO == EFAULT) + tst_res(TPASS, "statx set errno to EFAULT as expected"); + else + tst_res(TFAIL | TERRNO, "statx set errno to some unexpected value"); +} + +static struct tst_test test = { + .test_all = run, + .min_kver = "4.11", +}; +-------------------------------------------------------------------------------- + +The +TEST+ macro sets +TEST_RETURN+ to the return value of +tst_statx()+ and ++TEST_ERRNO+ to the value of +errno+ immediately after the functions +return. This is mainly just for convenience, although it potentially could +have other uses. + +We check whether the return value indicates success and if it doesn't also +check the value of +errno+. The last call to +tst_res+ includes +TERRNO+, +which will print the current error number and associated description in +addition to the message we have provided. Note that it uses the current value +of +errno+ not +TEST_ERRNO+. + +What we should have done in the example above is use +TTERRNO+ which takes the +value of +TEST_ERRNO+. + +If we try to run the test on a kernel where +statx+ does not exist, then ++tst_syscall+ will cause it to fail gracefully with +TCONF+. Where +TCONF+ +indicates the test is not applicable to our configuration. + +The function +tst_syscall+ calls +tst_brk(TCONF,...)+ on failure. +tst_brk+ +causes the test to exit immediately, which prevents any further test code from +being run. + +4.1 What are the differences between tst_brk and tst_res? +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +See +include/tst_test.h+ and the +https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines[test +writing guide]. Also what do they have in common? + +4.2 What happens if you call tst_res(TINFO, ...) after sys_statx? +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Does the test still function correctly? + +4.3 Extend the test to handle other basic error conditions. +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +For example, see if you can trigger +ENOENT+ instead. You shouldn't +have to create any files, which is discussed in the next section. + +5. Setup, Cleanup and files +--------------------------- + +Some tests require resources to be allocated, or system settings to be +changed, before the test begins. This 'setup' only has to be done once at the +beginning and at the end of the test needs to be removed or reverted. The +'cleanup' also has to be done regardless of whether the test breaks. + +Fortunately, like most test libraries, we have setup and cleanup (teardown) +callbacks. +setup+ is called once before +run+ and +cleanup+ is called once +afterwards. Note that +run+ itself can be called multiple times by the test +harness, but that +setup+ and +cleanup+ are only called once. + +If either your code, a +SAFE_*+ macro or a library function such as ++tst_syscall+ call +tst_brk+, then +run+ will exit immediately and the ++cleanup+ function is then called. Once 'cleanup' is completed, the test +executable will then exit altogether abandoning any remaining iterations of ++run+. + +For +statx+ we would like to create some files or file like objects which we +have control over. Deciding where to create the files is easy, we just create +it in the current working directory and let the LTP test harness handle where +that should be by setting +.needs_tmpdir = 1+. + +[source,c] +-------------------------------------------------------------------------------- +/* + * Test statx + * + * Check if statx exists and what error code it returns when we give it dodgy + * data. Then stat a file and check it returns success. + */ + +#include +#include "tst_test.h" +#include "lapi/syscalls.h" +#include "lapi/fcntl.h" + +#define FNAME "file_to_stat" +#define STATX_BASIC_STATS 0x000007ffU + +/*************** statx structure and wrapper goes here ! ***************/ + +... +-------------------------------------------------------------------------------- + +We have added an extra include +lapi/fcntl.h+ which wraps the system header by +the same name (+#include +). This header ensures we have definitions +for recently added macros such as +AT_FDCWD+ by providing fall backs if the +system header does not have them. The +lapi+ directory contains a number of +headers like this. + +At some point we may wish to add +lapi/stat.h+ to provide a fall back for +macros such as +STATX_BASIC_STATS+. However for the time being we have just +defined it in the test. + +[source,c] +-------------------------------------------------------------------------------- +... + +static void setup(void) +{ + SAFE_TOUCH(FNAME, 0777, NULL); +} + +static void run(void) +{ + struct statx statxbuf = { 0 }; + + TEST(sys_statx(0, NULL, 0, 0, &statxbuf)); + if (TEST_RETURN == 0) + tst_res(TFAIL, "statx thinks it can stat NULL"); + else if (TEST_ERRNO == EFAULT) + tst_res(TPASS, "statx set errno to EFAULT as expected"); + else + tst_res(TFAIL | TERRNO, "statx set errno to some unexpected value"); + + TEST(sys_statx(AT_FDCWD, FNAME, 0, STATX_BASIC_STATS, &statxbuf)); + if (TEST_RETURN == 0) + tst_res(TPASS, "It returned zero so it must have worked!"); + else + tst_res(TFAIL | TERRNO, "statx can not stat a basic file"); +} + +static struct tst_test test = { + .setup = setup, + .test_all = run, + .min_kver = "4.11", + .needs_tmpdir = 1 +}; +-------------------------------------------------------------------------------- + +The +setup+ callback uses one of the LTP's +SAFE+ functions to create an empty +file +file_to_stat+. Because we have set +.needs_tmpdir+, we can just create +this file in the present working directory. We don't need to create a ++cleanup+ callback yet because the LTP test harness will recursively delete +the temporary directory and its contents. + +The +run+ function can be called multiple times by the test harness, however ++setup+ and +cleanup+ callbacks will only be ran once. + +[WARNING] +By this point you may have begun to explore the LTP library headers or older +tests. In which case you will have come across functions from the old API such +as +tst_brkm+. The old API is being phased out, so you should not use these +functions. + +So far we haven't had to do any clean up. So our example doesn't answer the +question "what happens if part of the clean up fails?". To answer this we are +going to modify the test to ask the (highly contrived) question "What happens +if I create and open a file, then create a hard-link to it, then call open +again on the hard-link, then 'stat' the file". + +[source,c] +-------------------------------------------------------------------------------- +#define LNAME "file_to_stat_link" + +... + +static void setup(void) +{ + fd = SAFE_OPEN(FNAME, O_CREAT, 0777); + SAFE_LINK(FNAME, LNAME); + lfd = SAFE_OPEN(LNAME, 0); +} + +static void cleanup(void) +{ + if (lfd != 0) + SAFE_CLOSE(lfd); + + if (fd != 0) + SAFE_CLOSE(fd); +} + +static void run(void) +{ + ... + + TEST(sys_statx(AT_FDCWD, LNAME, 0, STATX_BASIC_STATS, &statxbuf)); + if (TEST_RETURN == 0) + tst_res(TPASS, "It returned zero so it must have worked!"); + else + tst_res(TFAIL | TERRNO, "statx can not stat a basic file"); +} + +static struct tst_test test = { + .setup = setup, + .cleanup = cleanup, + .test_all = run, + .tcnt = 2, + .min_kver = "4.11", + .needs_tmpdir = 1 +}; +-------------------------------------------------------------------------------- + +Because we are now opening a file, we need a +cleanup+ function to close the +file descriptors. We have to manually close the files to ensure the temporary +directory is deleted by the test harness (see the +https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines[test +writing guidelines] for details). + +As a matter of good practice, the file descriptors are closed in reverse +order. In some circumstances the order in which clean up is performed is +significant. In that case resources created towards the end of 'setup' are +dependent on ones near the beginning. So during 'cleanup' we remove the +dependants before their dependencies. + +If, for some reason, the file descriptor +lfd+ became invalid during the test, +but +fd+ was still open, we do not want +SAFE_CLOSE(lfd)+ to cause the ++cleanup+ function to exit prematurely. If it did, then +fd+ would remain open +which would cause problems on some file systems. + +Nor do we want to call +cleanup+ recursively. So during 'cleanup' +tst_brk+, +and consequently the +SAFE+ functions, do not cause the test to exit with ++TBROK+. Instead they just print an error message with +TWARN+. + +It is not entirely necessary to check if the file descriptors have a none zero +value before attempting to close them. However it avoids a bunch of spurious +warning messages if we fail to open +file_to_stat+. Test case failures can be +difficult to interpret at the best of times, so avoid filling the log with +noise. + +5.1 Check statx returns the correct number of hard links +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The field +statx.stx_nlink+ should be equal to 2, right? + +5.2 Git-branch +~~~~~~~~~~~~~~ + +We are about to make some organisational changes to the test, so now would be +a good time to branch. Then we can switch between the old and new versions, to +check the behavior has not been changed by accident. + +6. Split the test +----------------- + +In our current test, we have essentially rolled two different test cases into +one. Firstly we check if an error is returned when bad arguments are provided +and secondly we check what happens when we stat an actual file. Quite often it +makes sense to call +tst_res+ multiple times in a single test case because we +are checking different properties of the same result, but here we are clearly +testing two different scenarios. + +So we should split the test in two. One obvious way to do this is to create ++statx02.c+, but that seems like overkill in order to separate two simple test +cases. So, for now at least, we are going to do it a different way. + +[source,c] +-------------------------------------------------------------------------------- +... + +static void run_stat_null(void) +{ + struct statx statxbuf = { 0 }; + + TEST(sys_statx(0, NULL, 0, 0, &statxbuf)); + if (TEST_RETURN == 0) + tst_res(TFAIL, "statx thinks it can stat NULL"); + else if (TEST_ERRNO == EFAULT) + tst_res(TPASS, "statx set errno to EFAULT as expected"); + else + tst_res(TFAIL | TERRNO, "statx set errno to some unexpected value"); +} + +static void run_stat_symlink(void) +{ + struct statx statxbuf = { 0 }; + + TEST(sys_statx(AT_FDCWD, LNAME, 0, STATX_BASIC_STATS, &statxbuf)); + if (TEST_RETURN == 0) + tst_res(TPASS, "It returned zero so it must have worked!"); + else + tst_res(TFAIL | TERRNO, "statx can not stat a basic file"); +} + +static void run(unsigned int i) +{ + switch(i) { + case 0: run_stat_null(); + case 1: run_stat_symlink(); + } +} + +static struct tst_test test = { + .setup = setup, + .cleanup = cleanup, + .test = run, + .tcnt = 2, + .min_kver = "4.11", + .needs_tmpdir = 1 +}; +-------------------------------------------------------------------------------- + +So we have used an alternative form of the +test+ or +run+ callback which +accepts an index. Some tests use this index with an array of parameters and +expected return values. Others do something similar to the above. The index +can be used how you want so long as each iteration calls +tst_res+ in a +meaningful way. + +If an iteration fails to return a result (i.e. call +tst_res+ with a value +other than +TINFO+) then the test harness will report +TBROK+ and print the +iteration which failed. This prevents a scenario in your test from silently +failing due to some faulty logic. + +6.1 What is wrong with the switch statement? +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Were you paying attention? Also see the output of +checkpatch.pl+. + +6.2 Test a feature unique to statx +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +So far we have not tested anything which is unique to +statx+. So, for +example, you could check stx_btime is correct (possibly only to within a +margin of error) and that it differs from +stx_mtime+ after writing to the +file. + +Alternatively you could check that +stx_dev_major+ and +stx_dev_minor+ are set +correctly. Note that the LTP has helper functions for creating devices and +file systems (see +https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2214-testing-with-a-block-device[section +2.2.14] of the Test Writing Guidelines). + +This could be quite a challenging exercise. You may wish to tackle an +altogether different test scenario instead. If you get stuck just move onto +the next section and come back later. + +7. Submitting the test for review +--------------------------------- + +Ignoring the fact we should probably create +lapi/stat.h+ along with a bunch +of fallback logic in the build system. We can now get our test ready for +submission. + +The first thing you need to do before considering submitting your test is run ++scripts/checkpatch.pl --no-tree -f+ on +statx01.c+. Again, we use the kernel +style guidelines where possible. Next you should create a new branch, this +will allow you to reshape your commit history without fear. + +After that we have the pleasure of doing an interactive 'rebase' to clean up +our commit history. In its current form the test only really needs a single +commit, but if you have been using Git correctly then you should have +many. The main reason we want to compress it to a single commit, is to make +the LTP's Git-log readable. It also allows us to write a coherent description +of the work as a whole in retrospective. Although, when adding a new test, the +test description in the code will probably make the commit message redundant. + +Anyway, as an example, we shall look at my personal commit history from this +tutorial and 'rebase' it. You should try following along with your own +repository. First lets look at the commit history since we branched from +master. + +[source,shell] +-------------------------------------------------------------------------------- +$ git log -oneline master..HEAD +152d39fe7 (HEAD -> tutorial-rebase2, tutorial-rebase) tutorial: Start Submitting patch section +70f7ce7ce statx01: Stop checkpatch from complaining +bb0332bd7 tutorial: Fix review problems +6a87a084a statx01: Fix review problems +d784b1e85 test-writing-guidelines: Remove old API argument +c26e1be7a fixup! tutorial +1e24a5fb5 (me/tutorial-rebase) fixup! tutorial +568a3f7be fixup! tutorial +09dd2c829 statx: stage 6 +bfeef7902 statx: stage 5b +76e03d714 statx: stage 5a +98f5bc7ac statx: stage 4 +6f8c16438 statx: stage 3 (Add statx01) +5d93b84d8 Add statx and other syscall numbers +5ca627b78 tutorial: Add a step-by-step C test tutorial +-------------------------------------------------------------------------------- + +So we have told git to show all the commits which don't exist in 'master', but +are in +HEAD+, where +HEAD+ is the top of the current branch. The current +branch is +tutorial-rebase2+ which I just created. I have already done one +'rebase' and submitted a patch for review, so my original branch was just called ++tutorial+. + +As usual my commit history is starting to look like a bit of mess! There is +even a commit in there which should not be in the this branch (Remove old API +argument), however it can be ignored for now and 'cherry picked' into a new branch +later. + +For my patch I actually need at least two commits, one which contains the +tutorial text and one which contains the test and associated files. So first +of all I want to 'squash' (amalgamate) all the commits appended with ++tutorial:+ into the bottom commit. + +[source,shell] +-------------------------------------------------------------------------------- +$ git rebase -i 5ca627b78\^ +... +-------------------------------------------------------------------------------- + +This begins an interactive 'rebase' where commit 5ca6427b78 is the earliest +commit we want to edit. The +^+ symbol after the commit hash, specifies the +commit before this one. The interactive 'rebase' command takes the last commit +we want to keep unaltered as it's argument (in other words it takes a +non-inclusive range). + +Upon entering a similar command you will be presented with a text file +similar to the following. The file should be displayed in your text editor of +choice, if it doesn't, then you may change the editor variable in +.gitconfig+ +which was shown in section 3. + +[source,rebase] +-------------------------------------------------------------------------------- +pick 5ca627b78 tutorial: Add a step-by-step C test tutorial +pick 5d93b84d8 Add statx and other syscall numbers +pick 6f8c16438 statx: stage 3 (Add statx01) +pick 98f5bc7ac statx: stage 4 +pick 76e03d714 statx: stage 5a +pick bfeef7902 statx: stage 5b +pick 09dd2c829 statx: stage 6 +pick 568a3f7be fixup! tutorial +pick 1e24a5fb5 fixup! tutorial +pick c26e1be7a fixup! tutorial +pick d784b1e85 test-writing-guidelines: Remove old API argument +pick 6a87a084a statx01: Fix review problems +pick bb0332bd7 tutorial: Fix review problems +pick 70f7ce7ce statx01: Stop checkpatch from complaining +pick 152d39fe7 tutorial: Start Submitting patch section +-------------------------------------------------------------------------------- + +The last commit from Git-log is shown at the top. The left hand column +contains the commands we want to run on each commit. +pick+ just means we +re-apply the commit as-is. We can reorder the lines to apply the commits in a +different order, but we need to be careful when reordering commits to the same +file. If your 'rebase' results in a merge conflict, then you have probably +reordered some commits which contained changes to the same piece of code. + +Perhaps a better name for the interactive 'rebase' command would be 'replay'. As +we pick a point in the commit history, undo all those commits before that +point, then reapply them one at a time. During the replay we can reorder the +commits, drop, merge, split and edit them, creating a new history. + +The commands I am going to use are +reword+ and +fixup+. The +reword+ command +allows you to edit a single commit's message. The 'fixup' command 'squashes' a +commit into the commit above/preceding it, merging the two commits into +one. The commit which has +fixup+ applied has its commit message deleted. If +you think a commit might have something useful in its message then you can use ++squash+ instead. + +[source,rebase] +-------------------------------------------------------------------------------- +reword 5ca627b78 tutorial: Add a step-by-step C test tutorial +fixup 568a3f7be fixup! tutorial +fixup 1e24a5fb5 fixup! tutorial +fixup c26e1be7a fixup! tutorial +fixup bb0332bd7 tutorial: Fix review problems +fixup 152d39fe7 tutorial: Start Submitting patch section +fixup 276edecab tutorial: Save changes before rebase +pick 5d93b84d8 Add statx and other syscall numbers +pick 6f8c16438 statx: stage 3 (Add statx01) +pick 98f5bc7ac statx: stage 4 +pick 76e03d714 statx: stage 5a +pick bfeef7902 statx: stage 5b +pick 09dd2c829 statx: stage 6 +pick d784b1e85 test-writing-guidelines: Remove old API argument +pick 6a87a084a statx01: Fix review problems +-------------------------------------------------------------------------------- + +So all the commits marked with +fixup+ will be re-played by Git immediately +after 5ca62 at the top. A new commit will then be created with the amalgamated +changes of all the commits and 5ca62's log message. It turns out that I didn't +need to reword anything, but there is no harm in checking. It is easy to +forget the +Signed-off-by:+ line. + +I could now do the same for the commits to the +statx+ test, making the commit +message prefixes consistent. However I am not actually going to submit the +test (yet). + +I won't attempt to show you this, but if you need to do the opposite and split +apart a commit. It is also possible using Git-rebase by marking a line with ++edit+. This will pause Git just after replaying the marked commit. You can +then use a 'soft' Git-reset to bring the selected commit's changes back into +the 'index' where you are then able to un-stage some parts before +re-committing. + +You can also use +edit+ and +git commit --amend+ together to change a commit +deep in your history, but without reseting the 'index'. The 'index' contains +changes which you have staged with +git add+, but not yet committed. + +So now that the commit history has been cleaned up, we need to submit a patch +to the mailing list or make a pull request on GitHub. The mailing list is the +preferred place to make submissions and is more difficult for most people, so +I will only cover that method. + +Just before we create the patch, we need to check that our changes will still +apply to the master branch without problems. To do this we can use another +type of 'rebase' and then try rebuilding and running the test. + +[source,shell] +-------------------------------------------------------------------------------- +$ git checkout master +$ git pull origin +$ git checkout tutorial-rebase2 +$ git rebase master +-------------------------------------------------------------------------------- + +Above, I update the master branch and then replay our changes onto it using ++git rebase master+. You may find that after the rebase there is a merge +conflict. This will result in something which looks like the following (taken +from a Makefile conflict which was caused by reordering commits in a 'rebase'). + +[source,diff] +-------------------------------------------------------------------------------- +<<<<<<< HEAD +cve-2016-7117: LDFLAGS += -lpthread +======= +cve-2014-0196: LDFLAGS += -lpthread -lutil -lrt +cve-2016-7117: LDFLAGS += -lpthread -lrt +>>>>>>> 4dbfb8e79... Add -lrt +-------------------------------------------------------------------------------- + +The first line tells us this is the beginning of a conflict. The third line +separates the two conflicting pieces of content and the last line is the end +of the conflict. Usually, all you need to do is remove the lines you don't +want, stage the changes and continue the 'rebase' with +git rebase +--continue+. + +In order to create a patch e-mail we use +git format-patch+, we can then send +that e-mail using +git send-email+. It is also possible to import the patch +(+mbox+) file into a number of e-mail programs. + +[source,shell] +-------------------------------------------------------------------------------- +$ git format-patch -1 -v 2 -o output --to ltp@lists.linux.it fd3cc8596 +output/v2-0001-tutorial-Add-a-step-by-step-C-test-tutorial.patch +-------------------------------------------------------------------------------- + +The first argument +-1+ specifies we want one commit from fd3cc8596 +onwards. If we wanted this commit and the one after it we could specify +-2+ +instead. + +This is my second patch submission so I have used +-v 2+, which indicates this +is the second version of a patch set. The +-o+ option specifies the output +directory (literally called +output+). The +--to+ option adds the +To:+ e-mail +header, which I have set to the LTP mailing list. + +We can then send this patch with the following command sans +--dry-run+. + +[source,shell] +-------------------------------------------------------------------------------- +$ git send-email --dry-run output/v2-0001-tutorial-Add-a-step-by-step-C-test-tutorial.patch +-------------------------------------------------------------------------------- + +Git will ask some questions (which you an ignore) and then tell you what it +would do if this weren't a dry-run. In order for this to work you have to have +a valid SMTP server set in +.gitconfig+ and also be signed up to the LTP +mailing list under the same e-mail address you have configured in Git. You can +sign up at https://lists.linux.it/listinfo/ltp. + +8. Doing code review +-------------------- + +While waiting for your test to be reviewed, you are invited and encouraged to +review other contributors' code. This may seem bizarre when you are completely +new to the project, but there are two important ways in which you can +contribute here: + +A. Point out logical errors in the code. +B. Improve your own understanding + +It doesn't matter whether you know the canonical way of writing an LTP test in +C. An error of logic, when properly explained, is usually indisputable. These +are the most important errors to find as they always result in false test +results. Once someone points out such an error it is usually obvious to +everyone that it is a bug and needs to be fixed. + +Obviously testing the patch is one way of finding errors. You can apply +patches using +git am+. Then it is just a case of compiling and running the +tests. + +Finally, reading and attempting to comment on other peoples patches, gives +you a better understanding of the reviewers perspective. This is better for +the project and for you. + +Style and organisational issues are best left to after you have found logical +errors. + +9. Final notes +-------------- + +Hopefully you can now grasp the structure of an LTP test and have some idea of +what is available in the LTP test library. There are a vast number of library +functions available (mainly located in include and lib), some of which are +documented in the test writing guidelines and many of which are not. + +We have only scratched the surface of the immense technical complexity of +systems programming across multiple Kernel and C lib versions as well as +different hardware architectures. The important thing to take away from this +is that you have to be conscientious of what will happen on systems different +from yours. The LTP has a huge and varied user base, so situations you may +thing are unlikely can and do happen to somebody. + +Of course you don't want to spend time allowing for situations which may never +arise either, so you have to do your research and think about each situation +critically. The more systems you can test on before submitting your changes, +the better, although we understand not everyone has access to a lab. + +One important topic which has not been covered by this tutorial, is +multi-process or multi-threaded testing. The LTP library functions work inside +child processes and threads, but their semantics change slightly. There are +also various helper functions for synchronising and forking processes. For +more information see the Test Writing Guidelines (either at +https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines[the +Wiki] or in ./doc), in particular sections 2.2.7 to 2.2.10 and 2.2.13. + +When it comes time to submit a test, the preferred way to do it is on the +mailing list although you can also use GitHub. The LTP follows similar rules +to the kernel for formatting and submitting patches. Generally speaking the +review cycle is easier for small patches, so try to make small changes or +additions where possible. -- 2.13.3