[PATCH 2/4] ntdll: Fix read_directory_getdirentries().
Matteo Bruni
matteo.mystral at gmail.com
Fri Aug 28 14:56:22 CDT 2015
2015-08-28 2:40 GMT+02:00 Ken Thomases <ken at codeweavers.com>:
> On Aug 27, 2015, at 5:05 PM, Matteo Bruni <matteo.mystral at gmail.com> wrote:
>> A "safer" option would be to store the position in the fd unchanged
>> but reserve two values (-1 and -2?) for the '.' and '..'. I'm not sure
>> that's entirely safe but I don't see anything better.
>
> Negative values are rejected by lseek() with EINVAL. (Didn't actually try it, but the man page says so and the kernel code seemed to agree. ;)
Empirical tests agree too, lseek(fd, -1, SEEK_SET) returns -1 i.e.
error and a subsequent lseek(SEEK_CUR) gives a different value ;)
> We would need to treat offset 0 as indicating that the next entry to be read is ".". OFF_MAX - 1 would indicate that the next entry to be read is "..". OFF_MAX - 2 would indicate that we should seek to 0 and read real entries. Or something like that.
>
> I think we should not use OFF_MAX itself as a magic value. In my testing, the NFS driver sets the position to that at EOF. Of course, that raises the possibility that there are other special values that we'll collide with.
Yeah, that's a concern. Actually there is another possibility though.
We could swap the "." and ".." entries with the first two entries
returned from getdirentries(), in the same vein as
read_directory_getdents(). That should work but I'm not sure it's much
of an improvement overall. Also it looks like HFS+ at least lists
directory entries in-order and it would be somewhat unfortunate to
lose that "feature".
I'm attaching a v2 of the patch with your points addressed. I think
I'll also try to come up with an alternative version following the
aforementioned approach and see how ugly it looks like compared to
this...
-------------- next part --------------
commit 07c343ebd82178b716266a529a12cfc7fd0c1a1c
Author: Matteo Bruni <mbruni at codeweavers.com>
Date: Mon Jul 20 15:38:26 2015 +0100
ntdll: Fix read_directory_getdirentries(). (v2)
---
v2: This still abuses the fd position but arguably in a significantly
less nasty way. I fixed the other points reported by Ken too.
diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c
index f9562d2..2c5d940 100644
--- a/dlls/ntdll/directory.c
+++ b/dlls/ntdll/directory.c
@@ -1820,133 +1820,118 @@ static int read_directory_getdirentries( int fd, IO_STATUS_BLOCK *io, void *buff
BOOLEAN restart_scan, FILE_INFORMATION_CLASS class )
{
long restart_pos;
- ULONG_PTR restart_info_pos = 0;
- size_t size, initial_size = length;
- int res, fake_dot_dot = 1;
- char *data, local_buffer[8192];
+ int bytes_read, bytes_left, prev_bytes_left;
+ off_t i;
+ char data[8192];
struct dirent *de;
- union file_directory_info *info, *last_info = NULL, *restart_last_info = NULL;
+ union file_directory_info *info, *last_info = NULL;
- size = initial_size;
- data = local_buffer;
- if (size > sizeof(local_buffer) && !(data = RtlAllocateHeap( GetProcessHeap(), 0, size )))
+ io->u.Status = STATUS_SUCCESS;
+
+ if (restart_scan)
{
- io->u.Status = STATUS_NO_MEMORY;
- return io->u.Status;
+ lseek( fd, 0, SEEK_SET );
+ i = 0;
+ }
+ else
+ {
+ i = lseek( fd, 0, SEEK_CUR );
+ if (i == OFF_MAX - 2 || i == OFF_MAX - 1)
+ lseek( fd, 0, SEEK_SET );
}
- if (restart_scan) lseek( fd, 0, SEEK_SET );
-
- io->u.Status = STATUS_SUCCESS;
-
- /* FIXME: should make sure size is larger than filesystem block size */
- res = wine_getdirentries( fd, data, size, &restart_pos );
- if (res == -1)
+ bytes_read = bytes_left = wine_getdirentries( fd, data, sizeof(data), &restart_pos );
+ if (bytes_read == -1)
{
io->u.Status = FILE_GetNtStatus();
- res = 0;
goto done;
}
de = (struct dirent *)data;
- if (restart_scan)
+ for (;;)
{
- /* check if we got . and .. from getdirentries */
- if (res > 0)
+ if (i == 0)
{
- if (!strcmp( de->d_name, "." ) && res > dir_reclen(de))
- {
- struct dirent *next_de = (struct dirent *)(data + dir_reclen(de));
- if (!strcmp( next_de->d_name, ".." )) fake_dot_dot = 0;
- }
+ info = append_entry( buffer, io, length, ".", NULL, mask, class );
}
- /* make sure we have enough room for both entries */
- if (fake_dot_dot)
+ else if (i == OFF_MAX - 2)
{
- const ULONG min_info_size = dir_info_size( class, 1 ) + dir_info_size( class, 2 );
- if (length < min_info_size || single_entry)
- {
- FIXME( "not enough room %u/%u for fake . and .. entries\n", length, single_entry );
- fake_dot_dot = 0;
- }
+ info = append_entry( buffer, io, length, "..", NULL, mask, class );
}
-
- if (fake_dot_dot)
+ else if (bytes_left > 0)
{
- if ((info = append_entry( buffer, io, length, ".", NULL, mask, class )))
- last_info = info;
- if ((info = append_entry( buffer, io, length, "..", NULL, mask, class )))
- last_info = info;
+ prev_bytes_left = bytes_left;
+ bytes_left -= dir_reclen(de);
+ if (de->d_fileno && strcmp( de->d_name, "." ) && strcmp( de->d_name, ".." ))
+ info = append_entry( buffer, io, length, de->d_name, NULL, mask, class );
+ else
+ info = NULL;
+ de = (struct dirent *)((char *)de + dir_reclen(de));
+ }
+ else
+ {
+ info = NULL;
+ }
- restart_last_info = last_info;
- restart_info_pos = io->Information;
+ /* We count like 0, OFF_MAX - 2, OFF_MAX - 1, 1, 2, ... */
+ if (i == 0)
+ i = OFF_MAX - 2;
+ else
+ i++;
+ if (i == OFF_MAX)
+ i = 1;
+
+ if (info)
+ {
+ last_info = info;
+ if (single_entry)
+ break;
+ }
+ else if (io->u.Status == STATUS_BUFFER_OVERFLOW)
+ {
+ i--;
+ if (i == 0)
+ i = OFF_MAX - 1;
+ else if (i == OFF_MAX - 3)
+ i = 0;
+ bytes_left = prev_bytes_left;
+ if (last_info)
+ io->u.Status = STATUS_SUCCESS;
+ break;
}
- }
- while (res > 0)
- {
- res -= dir_reclen(de);
- if (de->d_fileno &&
- !(fake_dot_dot && (!strcmp( de->d_name, "." ) || !strcmp( de->d_name, ".." ))))
+ if (!bytes_left)
{
- if ((info = append_entry( buffer, io, length, de->d_name, NULL, mask, class )))
+ if (bytes_read >= sizeof(struct dirent))
{
- last_info = info;
- if (!has_wildcard( mask )) break;
- /* if we have to return but the buffer contains more data, restart with a smaller size */
- if (res > 0 && (single_entry || io->Information + max_dir_info_size(class) > length))
- {
- lseek( fd, (unsigned long)restart_pos, SEEK_SET );
- size = (char *)de + dir_reclen(de) - data;
- io->Information = restart_info_pos;
- last_info = restart_last_info;
- goto restart;
- }
+ bytes_read = bytes_left = wine_getdirentries( fd, data, sizeof(data), &restart_pos );
+ de = (struct dirent *)data;
}
else
- {
- if (io->u.Status == STATUS_BUFFER_OVERFLOW)
- {
- lseek( fd, (unsigned long)restart_pos, SEEK_SET );
- if (restart_info_pos) /* if we have a complete read already, return it */
- {
- io->u.Status = STATUS_SUCCESS;
- io->Information = restart_info_pos;
- last_info = restart_last_info;
- break;
- }
- /* otherwise restart from the start with a smaller size */
- size = (char *)de - data;
- if (!size) break;
- io->Information = 0;
- last_info = NULL;
- goto restart;
- }
- }
- }
- /* move on to the next entry */
- if (res > 0)
- {
- de = (struct dirent *)((char *)de + dir_reclen(de));
- continue;
+ break;
}
- if (size < initial_size) break; /* already restarted once, give up now */
- restart_last_info = last_info;
- restart_info_pos = io->Information;
- restart:
- res = wine_getdirentries( fd, data, size, &restart_pos );
- de = (struct dirent *)data;
+ }
+
+ /* Store the updated position in the fd. */
+ if (i == OFF_MAX - 2 || i == OFF_MAX - 1)
+ {
+ i = lseek( fd, i, SEEK_SET );
+ }
+ else
+ {
+ lseek( fd, restart_pos, SEEK_SET );
+ if (bytes_left != bytes_read)
+ wine_getdirentries( fd, data, bytes_read - bytes_left, &restart_pos );
}
if (last_info)
last_info->next = 0;
else if (io->u.Status != STATUS_BUFFER_OVERFLOW)
io->u.Status = restart_scan ? STATUS_NO_SUCH_FILE : STATUS_NO_MORE_FILES;
- res = 0;
+
done:
- if (data != local_buffer) RtlFreeHeap( GetProcessHeap(), 0, data );
- return res;
+ return 0;
}
#ifdef _DARWIN_FEATURE_64_BIT_INODE
More information about the wine-devel
mailing list