Re: kernel32: Don't set unconditionally SECTION_QUERY flag in OpenFileMapping.

[Dmitry Timoshkov]

Thanks, I have only style related comments, and since I'm an author of
existing tests I'd prefer to keep an original style if possible.

Jacek Caban  wrote:
> --- a/dlls/advapi32/tests/security.c
> +++ b/dlls/advapi32/tests/security.c
> @@ -5560,17 +5560,23 @@ static void test_filemap_security(void)
>      char temp_path[MAX_PATH];
>      char file_name[MAX_PATH];
>      DWORD ret, i, access;
> - HANDLE file, mapping, dup;
> - HANDLE file, mapping, dup, created_mapping;
> static const struct
> {
> -
>    int generic, mapped;
> -
>    DWORD generic, mapped;

Please leave 'int' type, it changes nothing.
> -
>    BOOL open_only;
> } map[] =
>    {
>        { 0, 0 },
>        { GENERIC_READ, STANDARD_RIGHTS_READ | SECTION_QUERY | SECTION_MAP_READ },
>        { GENERIC_WRITE, STANDARD_RIGHTS_WRITE | SECTION_MAP_WRITE },
>        { GENERIC_EXECUTE, STANDARD_RIGHTS_EXECUTE | SECTION_MAP_EXECUTE },
> -
>    { GENERIC_ALL, STANDARD_RIGHTS_REQUIRED | SECTION_ALL_ACCESS }
> -
>    { GENERIC_ALL, STANDARD_RIGHTS_REQUIRED | SECTION_ALL_ACCESS },
> -
>    { SECTION_MAP_READ | SECTION_MAP_WRITE, SECTION_MAP_READ | SECTION_MAP_WRITE},
> -
>    { SECTION_MAP_WRITE, SECTION_MAP_WRITE},
> -
>    { SECTION_MAP_READ | SECTION_QUERY, SECTION_MAP_READ | SECTION_QUERY},
> -
>    { SECTION_QUERY, SECTION_MAP_READ, TRUE },
> -
>    { SECTION_QUERY | SECTION_MAP_READ, SECTION_QUERY | SECTION_MAP_READ }
> };
>    static const struct
>    {
>
> @@ -5599,6 +5605,8 @@ static void test_filemap_security(void)
>  for (i = 0; i < sizeof(prot_map)/sizeof(prot_map[0]); i++)
>  {
> -
>    if (map[i].open_only) continue;
> -
>    SetLastError(0xdeadbeef);
>    mapping = CreateFileMappingW(file, NULL, prot_map[i].prot, 0, 4096, NULL);
>    if (prot_map[i].mapped)
>
> @@ -5645,6 +5653,8 @@ static void test_filemap_security(void)
>  for (i = 0; i < sizeof(map)/sizeof(map[0]); i++)
>  {
> -
>    if (map[i].open_only) continue;
> -
>    SetLastError( 0xdeadbeef );
>    ret = DuplicateHandle(GetCurrentProcess(), mapping, GetCurrentProcess(), &dup,
>                          map[i].generic, FALSE, 0);
>
> @@ -5659,6 +5669,24 @@ static void test_filemap_security(void)
>      CloseHandle(mapping);
>      CloseHandle(file);
>      DeleteFileA(file_name);
> -
> - created_mapping = CreateFileMappingA( INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE, 0, 0x1000,
> -
>                                     "Wine Test Open Mapping");
> - ok( created_mapping != NULL, "CreateFileMapping failed with error %u\n", GetLastError() );
> -
> - for(i=0; i < sizeof(map)/sizeof(*map); i++)

Please add spaces around '=' and use sizeof(map[0]) for consistency with existing
code.
> - {
> -
>    if (!map[i].generic) continue;
> -
> -
>    mapping = OpenFileMappingA( map[i].generic, FALSE, "Wine Test Open Mapping");
> -
>    ok( mapping != NULL, "OpenFileMapping failed with error %d\n", GetLastError());
> -
>    access = get_obj_access( mapping );
> -
>    ok( access == map[i].mapped, "[%d] unexpected access flags %x, expected %x\n",
> -
>        i, access, map[i].mapped );

Please use "%d:" instead of [%d] and %#x instead of %x for consistency
with existing code.

P.S.
That's perfectly fine to completely ignore my comments, just wanted to show
how it looks like for comparison with d3d-like acceptance policy :)

kernel32: Don't set unconditionally SECTION_QUERY flag in OpenFileMapping.

[Jacek Caban]

On 03/11/16 14:44, Dmitry Timoshkov wrote:
> Thanks, I have only style related comments, and since I'm an author of
> existing tests I'd prefer to keep an original style if possible.
>
> Jacek Caban  wrote:
> > --- a/dlls/advapi32/tests/security.c
> > +++ b/dlls/advapi32/tests/security.c
> > @@ -5560,17 +5560,23 @@ static void test_filemap_security(void)
> >      char temp_path[MAX_PATH];
> >      char file_name[MAX_PATH];
> >      DWORD ret, i, access;
> > - HANDLE file, mapping, dup;
> > - HANDLE file, mapping, dup, created_mapping;
> > static const struct
> > {
> > -
> >    int generic, mapped;
> > -
> >    DWORD generic, mapped;
>
> Please leave 'int' type, it changes nothing.
> > -
> >    BOOL open_only;
> > } map[] =
> >    {
> >        { 0, 0 },
> >        { GENERIC_READ, STANDARD_RIGHTS_READ | SECTION_QUERY | SECTION_MAP_READ },
> >        { GENERIC_WRITE, STANDARD_RIGHTS_WRITE | SECTION_MAP_WRITE },
> >        { GENERIC_EXECUTE, STANDARD_RIGHTS_EXECUTE | SECTION_MAP_EXECUTE },
> > -
> >    { GENERIC_ALL, STANDARD_RIGHTS_REQUIRED | SECTION_ALL_ACCESS }
> > -
> >    { GENERIC_ALL, STANDARD_RIGHTS_REQUIRED | SECTION_ALL_ACCESS },
> > -
> >    { SECTION_MAP_READ | SECTION_MAP_WRITE, SECTION_MAP_READ | SECTION_MAP_WRITE},
> > -
> >    { SECTION_MAP_WRITE, SECTION_MAP_WRITE},
> > -
> >    { SECTION_MAP_READ | SECTION_QUERY, SECTION_MAP_READ | SECTION_QUERY},
> > -
> >    { SECTION_QUERY, SECTION_MAP_READ, TRUE },
> > -
> >    { SECTION_QUERY | SECTION_MAP_READ, SECTION_QUERY | SECTION_MAP_READ }

You missed missing space before '}' in your review.
> >  };
> >  static const struct
> >  {
> >
> > @@ -5599,6 +5605,8 @@ static void test_filemap_security(void)
> >  for (i = 0; i < sizeof(prot_map)/sizeof(prot_map[0]); i++)
> >  {
> > -
> >    if (map[i].open_only) continue;
> > -
> >    SetLastError(0xdeadbeef);
> >    mapping = CreateFileMappingW(file, NULL, prot_map[i].prot, 0, 4096, NULL);
> >    if (prot_map[i].mapped)
> >
> > @@ -5645,6 +5653,8 @@ static void test_filemap_security(void)
> >  for (i = 0; i < sizeof(map)/sizeof(map[0]); i++)
> >  {
> > -
> >    if (map[i].open_only) continue;
> > -
> >    SetLastError( 0xdeadbeef );
> >    ret = DuplicateHandle(GetCurrentProcess(), mapping, GetCurrentProcess(), &dup,
> >                          map[i].generic, FALSE, 0);
> >
> > @@ -5659,6 +5669,24 @@ static void test_filemap_security(void)
> >      CloseHandle(mapping);
> >      CloseHandle(file);
> >      DeleteFileA(file_name);
> > -
> > - created_mapping = CreateFileMappingA( INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE, 0, 0x1000,
> > -
> >                                     "Wine Test Open Mapping");
> > - ok( created_mapping != NULL, "CreateFileMapping failed with error %u\n", GetLastError() );
> > -
> > - for(i=0; i < sizeof(map)/sizeof(*map); i++)
>
> Please add spaces around '=' and use sizeof(map[0]) for consistency with existing
> code.
> > - {
> > -
> >    if (!map[i].generic) continue;
> > -
> > -
> >    mapping = OpenFileMappingA( map[i].generic, FALSE, "Wine Test Open Mapping");
> > -
> >    ok( mapping != NULL, "OpenFileMapping failed with error %d\n", GetLastError());
> > -
> >    access = get_obj_access( mapping );
> > -
> >    ok( access == map[i].mapped, "[%d] unexpected access flags %x, expected %x\n",
> > -
> >        i, access, map[i].mapped );
>
> Please use "%d:" instead of [%d] and %#x instead of %x for consistency
> with existing code.
>
> P.S.
> That's perfectly fine to completely ignore my comments, just wanted to show
> how it looks like for comparison with d3d-like acceptance policy :)

As far as I'm concerned such reviews are waste of both your and my time,
no matter whom they are from and right now it's you who gave me such
review. I don't know what you wanted to show me, I don't recall myself
giving such a review ever. I will send a "fixed" version with hope that
I can finally go back to fixing real problems.

Cheers,
Jacek

kernel32: Don't set unconditionally SECTION_QUERY flag in OpenFileMapping.

[Dmitry Timoshkov]

Jacek Caban  wrote:
> > P.S.
> > That's perfectly fine to completely ignore my comments, just wanted to show
> > how it looks like for comparison with d3d-like acceptance policy :)
>
> As far as I'm concerned such reviews are waste of both your and my time,
> no matter whom they are from and right now it's you who gave me such
> review. I don't know what you wanted to show me, I don't recall myself
> giving such a review ever. I will send a "fixed" version with hope that
> I can finally go back to fixing real problems.

That wasn't my intent to force you to send an "improved" version, quite
contrary I tried to show how for an ordinal developer a so called "review"
process suggested by the d3d team looks like. Yes, I agree with you - that's
a major waste of time, and sorry for the false feeling of blaming you for
something you never did.

kernel32: Don't set unconditionally SECTION_QUERY flag in OpenFileMapping.

[Andrew Eikum]

On Fri, Mar 11, 2016 at 10:54:33PM +0800, Dmitry Timoshkov wrote:
> > > That's perfectly fine to completely ignore my comments, just wanted to show
> > > how it looks like for comparison with d3d-like acceptance policy :)
> >
> > As far as I'm concerned such reviews are waste of both your and my time,
> > no matter whom they are from and right now it's you who gave me such
> > review. I don't know what you wanted to show me, I don't recall myself
> > giving such a review ever. I will send a "fixed" version with hope that
> > I can finally go back to fixing real problems.
>
> That wasn't my intent to force you to send an "improved" version, quite
> contrary I tried to show how for an ordinal developer a so called "review"
> process suggested by the d3d team looks like. Yes, I agree with you - that's
> a major waste of time, and sorry for the false feeling of blaming you for
> something you never did.

Actually you seem to be a pretty big fan of making obtuse statements
that waste peoples' time.

https://bugs.winehq.org/show_bug.cgi?id=25625#c11

Andrew

kernel32: Don't set unconditionally SECTION_QUERY flag in OpenFileMapping.

[Dmitry Timoshkov]

Andrew Eikum  wrote:
> On Fri, Mar 11, 2016 at 10:54:33PM +0800, Dmitry Timoshkov wrote:
> > > > That's perfectly fine to completely ignore my comments, just wanted to show
> > > > how it looks like for comparison with d3d-like acceptance policy :)
> > >
> > > As far as I'm concerned such reviews are waste of both your and my time,
> > > no matter whom they are from and right now it's you who gave me such
> > > review. I don't know what you wanted to show me, I don't recall myself
> > > giving such a review ever. I will send a "fixed" version with hope that
> > > I can finally go back to fixing real problems.
> >
> > That wasn't my intent to force you to send an "improved" version, quite
> > contrary I tried to show how for an ordinal developer a so called "review"
> > process suggested by the d3d team looks like. Yes, I agree with you - that's
> > a major waste of time, and sorry for the false feeling of blaming you for
> > something you never did.
>
> Actually you seem to be a pretty big fan of making obtuse statements
> that waste peoples' time.
>
> https://bugs.winehq.org/show_bug.cgi?id=25625#c11

It looks like it worked, and forced you to waste a lot of time writing
another obtuse reply instead of trying to understand what you were actually
pointed out to.