Re: [PATCH 2/2] kernel32: Implement a few more classes in GetFileInformationByHandleEx

[Martin Storsjö]

On Thu, 7 May 2015, Martin Storsjo wrote:
> ---
> dlls/kernel32/file.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/kernel32/file.c b/dlls/kernel32/file.c
> index 006db1c..9bf318e 100644
> --- a/dlls/kernel32/file.c
> +++ b/dlls/kernel32/file.c
> @@ -23,6 +23,7 @@
> #include "config.h"
> #include "wine/port.h"
>
> +#include <assert.h>
> #include <stdarg.h>
> #include <stdio.h>
> #include <errno.h>
> @@ -894,11 +895,10 @@ BOOL WINAPI GetFileInformationByHandleEx( HANDLE handle, FILE_INFO_BY_HANDLE_CLA
> {
>     NTSTATUS status;
>     IO_STATUS_BLOCK io;
> +    FILE_INFORMATION_CLASS nt_class;
>
> switch (class)
>     {
> -    case FileBasicInfo:
> -    case FileStandardInfo:
>     case FileRenameInfo:
>     case FileDispositionInfo:
>     case FileAllocationInfo:
> @@ -919,8 +919,24 @@ BOOL WINAPI GetFileInformationByHandleEx( HANDLE handle, FILE_INFO_BY_HANDLE_CLA
>         SetLastError( ERROR_CALL_NOT_IMPLEMENTED );
>         return FALSE;
>
> +    case FileBasicInfo:
> +    case FileStandardInfo:
>     case FileNameInfo:
> -        status = NtQueryInformationFile( handle, &io, info, size, FileNameInformation );
> +        switch (class) {
> +        case FileBasicInfo:
> +            nt_class = FileBasicInformation;
> +            break;
> +        case FileStandardInfo:
> +            nt_class = FileStandardInformation;
> +            break;
> +        case FileNameInfo:
> +            nt_class = FileNameInformation;
> +            break;
> +        default:
> +            assert(0);
> +            break;
> +        }
> +        status = NtQueryInformationFile( handle, &io, info, size, nt_class );
>         if (status != STATUS_SUCCESS)
>         {
>             SetLastError( RtlNtStatusToDosError( status ) );
> -- 
> 1.8.1.2

Is there anything concrete that needs to be fixed in this (and the other 
kernel32 patch in the same thread) to be acceptable?

// Martin

[PATCH 2/2] kernel32: Implement a few more classes in GetFileInformationByHandleEx

[Andrew Eikum]

On Tue, May 12, 2015 at 05:17:44PM +0300, Martin Storsjö wrote:
> > @@ -919,8 +919,24 @@ BOOL WINAPI GetFileInformationByHandleEx( HANDLE handle, FILE_INFO_BY_HANDLE_CLA
> >        SetLastError( ERROR_CALL_NOT_IMPLEMENTED );
> >        return FALSE;
> >
> > +    case FileBasicInfo:
> > +    case FileStandardInfo:
> >    case FileNameInfo:
> > -        status = NtQueryInformationFile( handle, &io, info, size, FileNameInformation );
> > +        switch (class) {
> > +        case FileBasicInfo:
> > +            nt_class = FileBasicInformation;
> > +            break;
> > +        case FileStandardInfo:
> > +            nt_class = FileStandardInformation;
> > +            break;
> > +        case FileNameInfo:
> > +            nt_class = FileNameInformation;
> > +            break;
> > +        default:
> > +            assert(0);
> > +            break;

The default case seems unnecessary, since it's impossible.

But, this whole double-switch thing strikes me as ugly. Another option
would be to call NtQueryInformationFile with the correct argument in
each case, then break and do the 'if(status!=SUCCESS)' stuff outside
of the switch statement.  That would eliminate the code duplication
with the NtQueryDirectoryFile error checking, too.
> Is there anything concrete that needs to be fixed in this (and the
> other kernel32 patch in the same thread) to be acceptable?

Tests would help for both patches.

Andrew

[PATCH 2/2] kernel32: Implement a few more classes in GetFileInformationByHandleEx

[Martin Storsjö]

On Tue, 12 May 2015, Andrew Eikum wrote:
> On Tue, May 12, 2015 at 05:17:44PM +0300, Martin Storsjö wrote:
> > > @@ -919,8 +919,24 @@ BOOL WINAPI GetFileInformationByHandleEx( HANDLE handle, FILE_INFO_BY_HANDLE_CLA
> > >        SetLastError( ERROR_CALL_NOT_IMPLEMENTED );
> > >        return FALSE;
> > >
> > > +    case FileBasicInfo:
> > > +    case FileStandardInfo:
> > >    case FileNameInfo:
> > > -        status = NtQueryInformationFile( handle, &io, info, size, FileNameInformation );
> > > +        switch (class) {
> > > +        case FileBasicInfo:
> > > +            nt_class = FileBasicInformation;
> > > +            break;
> > > +        case FileStandardInfo:
> > > +            nt_class = FileStandardInformation;
> > > +            break;
> > > +        case FileNameInfo:
> > > +            nt_class = FileNameInformation;
> > > +            break;
> > > +        default:
> > > +            assert(0);
> > > +            break;
>
> The default case seems unnecessary, since it's impossible.

At least my gcc (4.7) doesn't realize it, and complains about all the 
unhandled cases in the switch.
> But, this whole double-switch thing strikes me as ugly. Another option
> would be to call NtQueryInformationFile with the correct argument in
> each case, then break and do the 'if(status!=SUCCESS)' stuff outside
> of the switch statement.  That would eliminate the code duplication
> with the NtQueryDirectoryFile error checking, too.

That sounds like a good idea.
> > Is there anything concrete that needs to be fixed in this (and the
> > other kernel32 patch in the same thread) to be acceptable?
>
> Tests would help for both patches.

Yep, although I don't off-hand have any good idea of a meaningful test for 
GetSystemTimePreciseAsFileTime.

// Martin