Re: advapi32: Implement GetSecurityInfo. [take 2]

[Dmitry Timoshkov]

"Dan Hipschman"  wrote:
> +#include <assert.h>

...
> DWORD WINAPI GetSecurityInfo(
>     HANDLE hObject, SE_OBJECT_TYPE ObjectType,
> @@ -2718,8 +2735,44 @@ DWORD WINAPI GetSecurityInfo(
>     PSECURITY_DESCRIPTOR *ppSecurityDescriptor
> )
> {
> - FIXME("stub!\n");
> - return ERROR_BAD_PROVIDER;
> - PSECURITY_DESCRIPTOR sd;
> - NTSTATUS status;
> - ULONG n1, n2;
> - BOOL present, defaulted;
> -
> - status = NtQuerySecurityObject(hObject, SecurityInfo, NULL, 0, &n1);
> - assert(status != STATUS_SUCCESS);
> - if (status != STATUS_BUFFER_TOO_SMALL)
> -
>    return RtlNtStatusToDosError(status);

My previous comments regarding assert() still apply.

advapi32: Implement GetSecurityInfo. [take 2]

[Dan Hipschman]

On Wed, Aug 06, 2008 at 02:21:44PM +0900, Dmitry Timoshkov wrote:
> "Dan Hipschman"  wrote:
> > +#include <assert.h>
>
> ...
> > DWORD WINAPI GetSecurityInfo(
> >    HANDLE hObject, SE_OBJECT_TYPE ObjectType,
> > @@ -2718,8 +2735,44 @@ DWORD WINAPI GetSecurityInfo(
> >    PSECURITY_DESCRIPTOR *ppSecurityDescriptor
> > )
> > {
> > - FIXME("stub!\n");
> > - return ERROR_BAD_PROVIDER;
> > - PSECURITY_DESCRIPTOR sd;
> > - NTSTATUS status;
> > - ULONG n1, n2;
> > - BOOL present, defaulted;
> > -
> > - status = NtQuerySecurityObject(hObject, SecurityInfo, NULL, 0, &n1);
> > - assert(status != STATUS_SUCCESS);
> > - if (status != STATUS_BUFFER_TOO_SMALL)
> > -
> >    return RtlNtStatusToDosError(status);
>
> My previous comments regarding assert() still apply.

But I like the assert.  You haven't really given me a reason to take it
out.  Why is it inappropriate?  It's use here seems perfectly reasonable
to me.  I'm calling NtQuerySecurityObject in such a way that I'm
expecting it to fail, and if it doesn't fail it's certainly a bug.
Further, it had better fail because there's nothing reasonable to do if
it doesn't.  assert is the perfect way to document all these things.

advapi32: Implement GetSecurityInfo. [take 2]

[Dmitry Timoshkov]

"Dan Hipschman"  wrote:
> But I like the assert.  You haven't really given me a reason to take it
> out.  Why is it inappropriate?  It's use here seems perfectly reasonable
> to me.  I'm calling NtQuerySecurityObject in such a way that I'm
> expecting it to fail, and if it doesn't fail it's certainly a bug.
> Further, it had better fail because there's nothing reasonable to do if
> it doesn't.  assert is the perfect way to document all these things.

Any API may fail in some way, but that would be ridiculous to pollute
the whole Wine source tree with asserts. As I've said, IMO it's better
to have a test case for the API which will detect broken behaviour
instead.

advapi32: Implement GetSecurityInfo. [take 2]

[Juan Lang]

> Any API may fail in some way, but that would be ridiculous to pollute
> the whole Wine source tree with asserts. As I've said, IMO it's better
> to have a test case for the API which will detect broken behaviour
> instead.

I agree that we shouldn't have asserts across DLLs.  While we may
"know" that the behavior is consistent, we may change the
implementation of another DLL at some point, and it's more difficult
to discover cross-DLL dependencies like this than those within one
DLL.  I think Dmitry's right that test cases for the API are a better
way to ensure an API will behave in a particular way.  asserts are
better to ensure consistency within one module, IMO.
--Juan