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.
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list