__quickstat() fails to properly recognize directories

[czietz]

As mentioned in #37: There is an issue with __quickstat() not properly recognizing directories -- tested under TOS. __quickstat() is called from multiple functions in MiNTLib and is supposed to be a "quick stat version that gets everything but the timestamps."

Testcase: Create a directory \DIR and a file \FILE and run the following program:

#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
#include <stdio.h>

int __quickstat(const char *_path, struct stat *st, int lflag);

/* Create a directory 'DIR' and a regular file 'FILE' before running this. */
int main(void)
{
	struct stat st;
	int rv;
	rv = stat("\\FILE", &st);
	printf("stat(FILE) returns %d, mode is %o, S_ISDIR is %d\n", rv, st.st_mode, S_ISDIR(st.st_mode));
	rv = stat("\\DIR", &st);
	printf("stat(DIR) returns %d, mode is %o, S_ISDIR is %d\n", rv, st.st_mode, S_ISDIR(st.st_mode));
	rv = stat("\\NOTFOUND", &st);
	printf("stat(NOTFOUND) returns %d, mode is %o, S_ISDIR is %d\n", rv, st.st_mode, S_ISDIR(st.st_mode));

	rv = __quickstat("\\FILE", &st, 0);
	printf("__quickstat(FILE) returns %d, mode is %o, S_ISDIR is %d\n", rv, st.st_mode, S_ISDIR(st.st_mode));
	rv = __quickstat("\\DIR", &st, 0);
	printf("__quickstat(DIR) returns %d, mode is %o, S_ISDIR is %d\n", rv, st.st_mode, S_ISDIR(st.st_mode));
	rv = __quickstat("\\NOTFOUND", &st, 0);
	printf("__quickstat(NOTFOUND) returns %d, mode is %o, S_ISDIR is %d\n", rv, st.st_mode, S_ISDIR(st.st_mode));

	return 0;
}

You can see that stat() correctly reports directories, __quickstat() doesn't: st_mode is always 0100644 = regular file. It seems as if __quickstat() determines whether something is a directory or a file solely based on some simple check of the name and not on the information returned by Fsfirst:

mintlib/mintlib/quickstat.c

Lines 100 to 101 in f5c219b

	if (((drv = path[0]) != 0) && path[1] == ':' &&
	(path[2] == 0 || (path[2] == '\\' && path[3] == 0)) ) {

I also find this section of the code very strange:

mintlib/mintlib/quickstat.c

Lines 205 to 212 in f5c219b

	st->st_ino = __inode++;
	st->st_flags = 0;
	st->st_mode = 0644 | (isdir ? S_IFDIR | 0111 : S_IFREG);
	if (st->st_flags & FA_RDONLY)
	st->st_mode &= ~0222; /* no write permission */
	if (st->st_flags & FA_HIDDEN)
	st->st_mode &= ~0444; /* no read permission */

It sets st_mode only based on isdir. However when isdir is set, execution directly goto-es to fill_dir, skipping the block of code entirely. Also, st_flags is set to 0, only to be checked immediately afterwards for non-zero values.

[th-otto]

The goto fill_dir seems to be ok, since the other fields (st_mode etc) are set just before it.

But you are right, that whole code looks really messy, and i wonder whether we need that function at all. If you are using that function on mint with e.g. an ext2 filesystem, Fsfirst will actually do a Fstat64() anyway. On plain TOS, Fsfirst will also return the timestamps, so you don't need an extra call for it. So what the hell is saved by using that function instead of stat()?

Edit: eventually we should do some tests with eg. Kobold in gemdos mode (or just cp -r). One time with the current mintlib, one time with a library where quickstat() is replaced by stat(). Then copy some directories around, with a lot of small files. That should give a hint whether it really makes a difference.

[czietz]

What I was trying to say with respect to the goto: Whenever isdir is set to 1, line 207...

mintlib/mintlib/quickstat.c

Line 207 in f5c219b

st->st_mode = 0644 | (isdir ? S_IFDIR | 0111 : S_IFREG);

... is never reached. Therefore, checking isdir there is superfluous and possibly not what was intended.

[mikrosk]

@th-otto do you plan to work on this further? I guess it would be nice to have this 100% fixed before release.

[th-otto]

No time currently :( It would need some thorough tests.