Re: [libvirt] [PATCH v2 2/3] virtestmock: Print invalid file accesses into a file

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?

> >> + } >> + >> + 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.

>> + >> +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.