[libvirt] [PATCH v2 2/3] virtestmock: Print invalid file accesses into a file
Michal Privoznik
mprivozn at redhat.com
Thu May 12 12:33:08 UTC 2016
On 11.05.2016 17:15, Peter Krempa wrote:
> On Tue, May 10, 2016 at 17:24:13 +0200, Michal Privoznik wrote:
>> All the accesses to files outside our build or source directories
>> are now identified and appended into a file for later processing.
>> The location of the file that contains all the records can be
>> controlled via VIR_TEST_FILE_ACCESS env variable and defaults to
>> abs_builddir "/test_file_access.txt".
>>
>> The script that will process the access file is to be added in
>> next commit.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> .gitignore | 1 +
>> HACKING | 11 ++++++
>> cfg.mk | 6 +--
>> docs/hacking.html.in | 15 ++++++++
>> tests/testutils.c | 27 +++++++++----
>> tests/virtestmock.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++--
>> 6 files changed, 151 insertions(+), 14 deletions(-)
>>
>
> [...]
>
>> diff --git a/HACKING b/HACKING
>> index e308568..63ad327 100644
>> --- a/HACKING
>> +++ b/HACKING
>> @@ -152,6 +152,17 @@ There is also a "./run" script at the top level, to make it easier to run
>> programs that have not yet been installed, as well as to wrap invocations of
>> various tests under gdb or Valgrind.
>>
>> +When running our test suite it may happen that nondeterministic result is
>> +produced because of the test suite relying on a particular file in the system
>
> it may happen that the test result is nondeterministic
>
>> +being accessible or having some specific value. This is a problem because if
>> +ran under different environment the test result may be different and a test
>
> This whole sentence seems redundant with the first one.
>
>> +can fail. To catch this kind of errors, the test suite has a module for that.
>
> has a module that prints any path touched
>
>> +The module prints any path touched that fulfils constraints described above
>> +into a file. Then "VIR_TEST_FILE_ACCESS" environment variable can alter
>> +location where the file is stored.
>> +
>> + VIR_TEST_FILE_ACCESS="/tmp/file_access.txt" ./qemuxml2argvtest
>> +
>>
>>
>> (9) The Valgrind test should produce similar output to "make check". If the output
>> diff --git a/cfg.mk b/cfg.mk
>> index c0aba57..1bf63ac 100644
>> --- a/cfg.mk
>> +++ b/cfg.mk
>> @@ -1181,10 +1181,10 @@ exclude_file_name_regexp--sc_prohibit_access_xok = \
>> ^(cfg\.mk|src/util/virutil\.c)$$
>>
>> exclude_file_name_regexp--sc_prohibit_asprintf = \
>> - ^(cfg\.mk|bootstrap.conf$$|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$)
>> + ^(cfg\.mk|bootstrap.conf$$|src/util/virstring\.[ch]$$|tests/vir(cgroup|test)mock\.c$$)
>>
>> exclude_file_name_regexp--sc_prohibit_strdup = \
>> - ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
>> + ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup|test)mock.c$$)
>>
>> exclude_file_name_regexp--sc_prohibit_close = \
>> (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c)$$)
>> @@ -1211,7 +1211,7 @@ exclude_file_name_regexp--sc_prohibit_select = \
>> ^cfg\.mk$$
>>
>> exclude_file_name_regexp--sc_prohibit_raw_allocation = \
>> - ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|vircgroupmock)\.c|tools/wireshark/src/packet-libvirt\.c)$$
>> + ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|vir(cgroup|test)mock)\.c|tools/wireshark/src/packet-libvirt\.c)$$
>>
>> exclude_file_name_regexp--sc_prohibit_readlink = \
>> ^src/(util/virutil|lxc/lxc_container)\.c$$
>
> If you statically link with our utils you could avoid any of those
> above. Is there a special reason to avoid our wrappers?
>
>> diff --git a/docs/hacking.html.in b/docs/hacking.html.in
>> index 5cd23a2..63d26bd 100644
>> --- a/docs/hacking.html.in
>> +++ b/docs/hacking.html.in
>> @@ -189,6 +189,21 @@
>> under gdb or Valgrind.
>> </p>
>>
>> + <p>When running our test suite it may happen that
>> + nondeterministic result is produced because of the test
>> + suite relying on a particular file in the system being
>> + accessible or having some specific value. This is a
>> + problem because if ran under different environment the
>> + test result may be different and a test can fail. To
>> + catch this kind of errors, the test suite has a module
>> + for that. The module prints any path touched that fulfils
>> + constraints described above into a file. Then
>> + <code>VIR_TEST_FILE_ACCESS</code> environment variable
>> + can alter location where the file is stored.</p>
>> +<pre>
>> + VIR_TEST_FILE_ACCESS="/tmp/file_access.txt" ./qemuxml2argvtest
>> +</pre>
>
> See comments above.
>
>> +
>> </li>
>> <li><p>The Valgrind test should produce similar output to
>> <code>make check</code>. If the output has traces within libvirt
>> diff --git a/tests/testutils.c b/tests/testutils.c
>> index 595b64d..725398c 100644
>> --- a/tests/testutils.c
>> +++ b/tests/testutils.c
>> @@ -156,6 +156,13 @@ virtTestRun(const char *title,
>> {
>> int ret = 0;
>>
>> + /* Some test are fragile about environ settings. If that's
>> + * the case, don't poison it. All this means is that we will
>
> You mean that the test cases actually purge the environment?
Yes. I don't recall the exact test off the top of my head, but there was
an issue that we dry ran a command and compared what it would be ran
like. Including all environ variables.
>
>> + * not see which test in particular is touching which file,
>> + * but we are still able to tell which binary is doing so. */
>
> I don't think this is accurate, since if you don't have _PROGNAME set,
> the logging code won't print anything.
Okay, I can remove it.
>
>> + if (getenv("VIR_TEST_MOCK_PROGNAME"))
>> + setenv("VIR_TEST_MOCK_TESTNAME", title, 1);
>
> [...]
>
>> diff --git a/tests/virtestmock.c b/tests/virtestmock.c
>> index f138e98..c47eb0c 100644
>> --- a/tests/virtestmock.c
>> +++ b/tests/virtestmock.c
>
> [...]
>
>> @@ -64,17 +68,112 @@ static void init_syms(void)
>> LOAD_SYM(access);
>> LOAD_SYM_ALT(stat, __xstat);
>> LOAD_SYM_ALT(lstat, __lxstat);
>> +
>> +}
>> +
>> +static void
>> +printFile(const char *output,
>> + const char *file)
>> +{
>> + FILE *fp;
>> + const char *testname = getenv("VIR_TEST_MOCK_TESTNAME");
>> +
>> + if (!progname) {
>> + progname = getenv("VIR_TEST_MOCK_PROGNAME");
>> +
>> + if (!progname)
>> + return;
>> + }
>> +
>> + if (!(fp = realfopen(output, "a"))) {
>> + fprintf(stderr, "Unable to open %s: %s\n", output, strerror(errno));
>> + abort();
>
> I'm a bit worried that if this wrapper fails the testsuite will abort.
> Couldn't we just skip creating/opening the file at this point? You can
> always error out in the phase where the file is checked.
But how would I know then that this has failed? Note that this should
also work in case a single test is ran. Why is aborting each test that
failed to open the file a bad thing anyway?
>
>> + }
>> +
>> + if (flock(fileno(fp), LOCK_EX) < 0) {
>> + fprintf(stderr, "Unable to lock %s: %s\n", output, strerror(errno));
>> + fclose(fp);
>> + abort();
>
> Won't there be a possibility of collision if two tests are running in
> parallel?
I don't know what you mean. That's why I lock the file here so that
there's no collision.
>
>> + }
>> +
>> + /* Now append the following line into the output file:
>> + * $file: $progname $testname */
>> +
>> + fprintf(fp, "%s: %s", file, progname);
>> + if (testname)
>> + fprintf(fp, ": %s", testname);
>> +
>> + fputc('\n', fp);
>> +
>> + flock(fileno(fp), LOCK_UN);
>> + fclose(fp);
>> +}
>> +
>> +
>> +#define VIR_FILE_ACCESS_DEFAULT abs_builddir "/test_file_access.txt"
>> +
>> +static void
>> +cutOffLastComponent(char *path)
>> +{
>> + char *base = path, *p = base;
>> +
>> + while (*p) {
>> + if (*p == '/')
>> + base = p;
>> + p++;
>> + }
>> +
>> + if (base != p)
>> + *base = '\0';
>
> We already have a function like this:
> virStorageFileRemoveLastPathComponent.
>
> Also 'strrchr' instead of the while loop?
Hm.. okay. I can expose the function. I've looked around virfile.c and
haven't find anything. I didn't check storage driver sources though.
>
>> }
>>
>> static void
>> checkPath(const char *path)
>> {
>> + char *fullPath = NULL;
>> + char *relPath = NULL;
>> + char *crippledPath = NULL;
>> +
>> + if (path[0] != '/' &&
>> + asprintf(&relPath, "./%s", path) < 0)
>> + goto error;
>> +
>> + /* Le sigh. Both canonicalize_file_name() and realpath()
>> + * expect @path to exist otherwise they return an error. So
>> + * if we are called over an non-existent file, this could
>> + * return an error. In that case do our best and hope we will
>> + * catch possible error. */
>
> You can always stat the file to check if it exists before trying to get
> it's canonical path.
Why? if stat() succeeds so does canonicalize_file_name() and vice versa.
Therefore I think calling stat() would be just redundant.
>
>> + if ((fullPath = canonicalize_file_name(relPath ? relPath : path))) {
>> + path = fullPath;
>> + } else {
>> + /* Yeah, our worst nightmares just became true. Path does
>> + * not exist. Cut off the last component and retry. */
>> + if (!(crippledPath = strdup(relPath ? relPath : path)))
>> + goto error;
>
> So if the parrent directory won't exist either you will attempt to do
> the checking in the non-canonical path?
>
> Maybe you could try to match the paths according to getcwd() to resolve
> relative paths at first or maybe do this in a loop until you find an
> existing object. (I'm mostly worried about paths like
> ../../../../../../something/that/does/not/exist where at least the root
> directory should be accessible)
I think this is overcomplicated approach.
>
>> +
>> + cutOffLastComponent(crippledPath);
>> +
>> + if ((fullPath = canonicalize_file_name(crippledPath)))
>> + path = fullPath;
>> + }
>> +
>> +
>> if (!STRPREFIX(path, abs_topsrcdir) &&
>> !STRPREFIX(path, abs_topbuilddir)) {
>> - /* Okay, this is a dummy check and spurious print.
>> - * But this is going to be replaced soon. */
>> - fprintf(stderr, "*** %s ***\n", path);
>> + const char *output = getenv("VIR_TEST_FILE_ACCESS");
>
> I think you could cache this too as you've did with 'progname'
Okay.
>
>> + if (!output)
>> + output = VIR_FILE_ACCESS_DEFAULT;
>> + printFile(output, path);
>> }
>> +
>> + free(crippledPath);
>> + free(relPath);
>> + free(fullPath);
>> +
>> + return;
>> + error:
>> + fprintf(stderr, "Out of memory\n");
>> + abort();
>> }
>
Michal
More information about the libvir-list
mailing list