Re: [PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.

[Nikolay Sivov]

On 7/24/2010 18:51, Max TenEyck Woodbury wrote:
> dlls/ntdll/file.c |    5 +++++
>   1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c
> index 0a6ee55..86c200f 100644
> --- a/dlls/ntdll/file.c
> +++ b/dlls/ntdll/file.c
> @@ -2148,6 +2148,11 @@ NTSTATUS WINAPI NtSetInformationFile(HANDLE handle, PIO_STATUS_BLOCK io,
>               io->u.Status = STATUS_INVALID_PARAMETER_3;
>           break;
> - /* Invalid requests - do not need 'fixing'. */
> - case FileAllInformation:
> -
>    io->u.Status = STATUS_NOT_IMPLEMENTED;
> -
>    break;
> -
> default:
>     FIXME("Unsupported class (%d)\n", class);
>     io->u.Status = STATUS_NOT_IMPLEMENTED;

Add a test please, and a comment won't be needed with a test too.

[PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.

[Max TenEyck Woodbury]

On 07/24/2010 10:58 AM, Nikolay Sivov wrote:
> On 7/24/2010 18:51, Max TenEyck Woodbury wrote:
> > dlls/ntdll/file.c | 5 +++++
> > 1 files changed, 5 insertions(+), 0 deletions(-)
> >
> > diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c
> > index 0a6ee55..86c200f 100644
> > --- a/dlls/ntdll/file.c
> > +++ b/dlls/ntdll/file.c
> > @@ -2148,6 +2148,11 @@ NTSTATUS WINAPI NtSetInformationFile(HANDLE
> > handle, PIO_STATUS_BLOCK io,
> > io->u.Status = STATUS_INVALID_PARAMETER_3;
> > break;
> > - /* Invalid requests - do not need 'fixing'. */
> > - case FileAllInformation:
> > - io->u.Status = STATUS_NOT_IMPLEMENTED;
> > - break;
> > -
> >
> > default:
> > FIXME("Unsupported class (%d)\n", class);
> > io->u.Status = STATUS_NOT_IMPLEMENTED;
>
> Add a test please, and a comment won't be needed with a test too.

There already is a test in dlls/ntdll/test/file.c. It produces a
'fixme:' when it should not. This fixes that.

[PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.

[Dmitry Timoshkov]

Max TenEyck Woodbury  wrote:
> > > - /* Invalid requests - do not need 'fixing'. */
> > > - case FileAllInformation:
> > > - io->u.Status = STATUS_NOT_IMPLEMENTED;
> > > - break;
> > > -
> > >
> > > default:
> > > FIXME("Unsupported class (%d)\n", class);
> > > io->u.Status = STATUS_NOT_IMPLEMENTED;
> >
> > Add a test please, and a comment won't be needed with a test too.
>
> There already is a test in dlls/ntdll/test/file.c. It produces a
> 'fixme:' when it should not. This fixes that.

This is not a fix. A fix is a patch that makes a test pass that previously
did not. You simply silence a harmless fixme.

[PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.

[Max TenEyck Woodbury]

On 07/25/2010 12:50 AM, Dmitry Timoshkov wrote:
> Max TenEyck Woodbury  wrote:
> > > > - /* Invalid requests - do not need 'fixing'. */
> > > > - case FileAllInformation:
> > > > - io->u.Status = STATUS_NOT_IMPLEMENTED;
> > > > - break;
> > > > -
> > > >
> > > > default:
> > > > FIXME("Unsupported class (%d)\n", class);
> > > > io->u.Status = STATUS_NOT_IMPLEMENTED;
> > >
> > > Add a test please, and a comment won't be needed with a test too.
> >
> > There already is a test in dlls/ntdll/test/file.c. It produces a
> > 'fixme:' when it should not. This fixes that.
>
> This is not a fix. A fix is a patch that makes a test pass that previously
> did not. You simply silence a harmless fixme.

The 'fixme' is not 'harmless'. It gets counted. It claims that there is
something wrong that needs fixing. In fact, the proper implementation
for this particular request is to set an error code and ignore the
request. The test code checks for exactly that condition and does NOT 
report a test failure when that happens. It reports a failure if the 
error code does NOT have the specified value. In other words this patch
fixes an error in the 'work to be done count'.

Please stop the sniping. There is now more text disparaging the patch
than there is text in the patch itself.

-Max

[PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.

[Reece Dunn]

On 24 July 2010 16:10, Max TenEyck Woodbury  wrote:
> On 07/24/2010 10:58 AM, Nikolay Sivov wrote:
> > On 7/24/2010 18:51, Max TenEyck Woodbury wrote:
> > > dlls/ntdll/file.c | 5 +++++
> > > 1 files changed, 5 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c
> > > index 0a6ee55..86c200f 100644
> > > --- a/dlls/ntdll/file.c
> > > +++ b/dlls/ntdll/file.c
> > > @@ -2148,6 +2148,11 @@ NTSTATUS WINAPI NtSetInformationFile(HANDLE
> > > handle, PIO_STATUS_BLOCK io,
> > > io->u.Status = STATUS_INVALID_PARAMETER_3;
> > > break;
> > > - /* Invalid requests - do not need 'fixing'. */
> > > - case FileAllInformation:
> > > - io->u.Status = STATUS_NOT_IMPLEMENTED;
> > > - break;
> > > -
> > >
> > > default:
> > > FIXME("Unsupported class (%d)\n", class);
> > > io->u.Status = STATUS_NOT_IMPLEMENTED;
> >
> > Add a test please, and a comment won't be needed with a test too.
>
> There already is a test in dlls/ntdll/test/file.c. It produces a
> 'fixme:' when it should not. This fixes that.

The only test I can find is:

1110     res = pNtSetInformationFile(h, &io, &fai_buf.fai, sizeof
fai_buf, FileAllInformation);
1111     ok ( res == STATUS_INVALID_INFO_CLASS || res ==
STATUS_NOT_IMPLEMENTED, "shouldn't be able to set FileAllInformation,
res %x\n", res);

I would say that (based on this test), STATUS_NOT_IMPLEMENTED is the
wrong value to use -- it should really be STATUS_INVALID_INFO_CLASS.
The check should mark the STATUS_NOT_IMPLEMENTED value as a broken
value:

ok ( res == STATUS_INVALID_INFO_CLASS || broken(res ==
STATUS_NOT_IMPLEMENTED), "shouldn't be able to set FileAllInformation,
res %x\n", res);

This would fix the return value for this condition.

In addition to this, the Wine implementation is returning io->u.Status
in all these cases, but the tests for pNtSetInformationFile do not
also check they are the same. To avoid false positives or negatives,
io->u.Status needs to be set before each test. Thus, you have
something like:

io.u.Status = 0xdeadbeef;
    res = pNtSetInformationFile(...);
    ok (res == STATUS_SUCCESS, "Expected NtSetInformationFile to
return STATUS_SUCCESS, got %d\n", res);
    ok(res == U(io).Status, "Expected NtSetInformationFile return to
match io.u.Status, got res = %d, io.u.Status = %d\n", res,
U(io).Status);

or:

ok (res == STATUS_SUCCESS, "Expected NtSetInformationFile to
return STATUS_SUCCESS, got %d\n", res);
    ok(U(io).Status == STATUS_SUCCESS, "Expected io.u.Status to be
STATUS_SUCCESS, got %d\n", U(io).Status);

This way, the tests confirm that io.u.Status and the return value of
NtSetInformationFile are the same.

- Reece

[PATCH] dlls/ntdll/file.c: Setting FileAllInformation is not 'fixable'.

[Max TenEyck Woodbury]

On 07/25/2010 02:25 PM, Reece Dunn wrote:
> On 24 July 2010 16:10, Max TenEyck Woodbury  wrote:
> > On 07/24/2010 10:58 AM, Nikolay Sivov wrote:
> > > On 7/24/2010 18:51, Max TenEyck Woodbury wrote:
> > > > dlls/ntdll/file.c | 5 +++++
> > > > 1 files changed, 5 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c
> > > > index 0a6ee55..86c200f 100644
> > > > --- a/dlls/ntdll/file.c
> > > > +++ b/dlls/ntdll/file.c
> > > > @@ -2148,6 +2148,11 @@ NTSTATUS WINAPI NtSetInformationFile(HANDLE
> > > > handle, PIO_STATUS_BLOCK io,
> > > > io->u.Status = STATUS_INVALID_PARAMETER_3;
> > > > break;
> > > > - /* Invalid requests - do not need 'fixing'. */
> > > > - case FileAllInformation:
> > > > - io->u.Status = STATUS_NOT_IMPLEMENTED;
> > > > - break;
> > > > -
> > > >
> > > > default:
> > > > FIXME("Unsupported class (%d)\n", class);
> > > > io->u.Status = STATUS_NOT_IMPLEMENTED;
> > >
> > > Add a test please, and a comment won't be needed with a test too.
> >
> > There already is a test in dlls/ntdll/test/file.c. It produces a
> > 'fixme:' when it should not. This fixes that.
>
> The only test I can find is:
>
> 1110     res = pNtSetInformationFile(h,&io,&fai_buf.fai, sizeof
> fai_buf, FileAllInformation);
> 1111     ok ( res == STATUS_INVALID_INFO_CLASS || res ==
> STATUS_NOT_IMPLEMENTED, "shouldn't be able to set FileAllInformation,
> res %x\n", res);
>
> I would say that (based on this test), STATUS_NOT_IMPLEMENTED is the
> wrong value to use -- it should really be STATUS_INVALID_INFO_CLASS.
> The check should mark the STATUS_NOT_IMPLEMENTED value as a broken
> value:
>   ok ( res == STATUS_INVALID_INFO_CLASS || broken(res ==
>
> STATUS_NOT_IMPLEMENTED), "shouldn't be able to set FileAllInformation,
> res %x\n", res);
>
> This would fix the return value for this condition.
>
> In addition to this, the Wine implementation is returning io->u.Status
> in all these cases, but the tests for pNtSetInformationFile do not
> also check they are the same. To avoid false positives or negatives,
> io->u.Status needs to be set before each test. Thus, you have
> something like:
>  io.u.Status = 0xdeadbeef;
>  res = pNtSetInformationFile(...);
>  ok (res == STATUS_SUCCESS, "Expected NtSetInformationFile to
>
> return STATUS_SUCCESS, got %d\n", res);
>      ok(res == U(io).Status, "Expected NtSetInformationFile return to
> match io.u.Status, got res = %d, io.u.Status = %d\n", res,
> U(io).Status);
>
> or:
>  ok (res == STATUS_SUCCESS, "Expected NtSetInformationFile to
>
> return STATUS_SUCCESS, got %d\n", res);
>      ok(U(io).Status == STATUS_SUCCESS, "Expected io.u.Status to be
> STATUS_SUCCESS, got %d\n", U(io).Status);
>
> This way, the tests confirm that io.u.Status and the return value of
> NtSetInformationFile are the same.
> - Reece

Excellent critique. You are obviously better informed than I am. Please
submit a patch. I will assume your logic to be correct and will change
the status value generated.

- Max