On Thu, May 12, 2016 at 14:33:08 +0200, Michal Privoznik wrote:
On 11.05.2016 17:15, Peter Krempa wrote:
On Tue, May 10, 2016 at 17:24:13 +0200, Michal Privoznik wrote:
[...]
+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?
In such case it's more than likely that the file will fail to be opened even for the analysis.
+ } + + 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.
Never mind. I re-read the man page for flock and noticed that LOCK_EX whithout LOCK_NB will actually block until the lock is released. [...]
+ +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.
src/util/virstoragefile.c AFAIK
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.
Hmm, fair enough.
+ 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.
Hmm, let's see if it will bite in the future then.