Thread: Re: [COMMITTERS] pgsql: Use symbolic names not octal constants for file permission flags

This broke the buildfarm on Windows, and I'm not sure of the best way to fix it.

We currently define read and write permissions in port/win32.h
specifically for windows. A quick-fix to just add these new ones as
aliases won't work, because they are used in both open and umask
calls.

Since umask() "turns off" permissions, they can't be defined to the
same, or all files are created readonly. For umask, it would work to
define them to 0. But for open() and other such calls that "turn on"
permissions", it needs to be defined to the same value as "read
permissions for user".

Not sure of the best way to fix this. Perhaps we need to invent
PG_UMASK_xyz macros?

//Magnus

On Fri, Dec 10, 2010 at 23:35, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Use symbolic names not octal constants for file permission flags.
>
> Purely cosmetic patch to make our coding standards more consistent ---
> we were doing symbolic some places and octal other places.  This patch
> fixes all C-coded uses of mkdir, chmod, and umask.  There might be some
> other calls I missed.  Inconsistency noted while researching tablespace
> directory permissions issue.
>
> Branch
> ------
> master
>
> Details
> -------
> http://git.postgresql.org/gitweb?p=postgresql.git;a=commitdiff;h=04f4e10cfc158239ca00a6ed6a84428c7acb1e6d
>
> Modified Files
> --------------
> src/backend/access/transam/xlog.c   |    2 +-
> src/backend/commands/copy.c         |    2 +-
> src/backend/commands/tablespace.c   |    2 +-
> src/backend/libpq/be-fsstubs.c      |    7 ++++---
> src/backend/postmaster/postmaster.c |    4 ++--
> src/backend/postmaster/syslogger.c  |    6 +++---
> src/backend/storage/file/copydir.c  |    2 +-
> src/backend/storage/ipc/ipc.c       |    4 ++--
> src/bin/initdb/initdb.c             |   16 ++++++++--------
> src/bin/pg_ctl/pg_ctl.c             |    2 +-
> 10 files changed, 24 insertions(+), 23 deletions(-)
>
>
> --
> Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-committers
>



--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:
> This broke the buildfarm on Windows, and I'm not sure of the best way to fix it.
> We currently define read and write permissions in port/win32.h
> specifically for windows. A quick-fix to just add these new ones as
> aliases won't work, because they are used in both open and umask
> calls.

Hm, those symbols are already in use elsewhere in the code; I would
assume it's just a matter of missing #includes in these particular
files.  Where does Windows define 'em?
        regards, tom lane

I wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> This broke the buildfarm on Windows, and I'm not sure of the best way to fix it.

> Hm, those symbols are already in use elsewhere in the code; I would
> assume it's just a matter of missing #includes in these particular
> files.  Where does Windows define 'em?

Ah, I have a theory: <fcntl.h>.  Seems that ancient Unix specs say
S_IRUSR etc should be defined there, rather than <sys/stat.h> which
is the modern expectation.  Count on M$ to find creative ways of being
incompatible ...
        regards, tom lane

On Sat, Dec 11, 2010 at 17:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Magnus Hagander <magnus@hagander.net> writes:
>>> This broke the buildfarm on Windows, and I'm not sure of the best way to fix it.
>
>> Hm, those symbols are already in use elsewhere in the code; I would
>> assume it's just a matter of missing #includes in these particular
>> files.  Where does Windows define 'em?
>
> Ah, I have a theory: <fcntl.h>.  Seems that ancient Unix specs say
> S_IRUSR etc should be defined there, rather than <sys/stat.h> which
> is the modern expectation.  Count on M$ to find creative ways of being
> incompatible ...

Nope, not there. I can't find S_IWGRP in any of the files.

But that symbol, OTOH, is *not* in use anywhere else in the code.
(only in zic.c, but it's ifdef'ed out on win32)

I guess I need to go look for each individual one that breaks and
split this into sub-problems.
--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:
> On Sat, Dec 11, 2010 at 17:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Ah, I have a theory: <fcntl.h>.

> Nope, not there. I can't find S_IWGRP in any of the files.

Well, I notice that copydir.c compiled before, and still compiles after,
despite this change:

-   if (mkdir(todir, S_IRUSR | S_IWUSR | S_IXUSR) != 0)
+   if (mkdir(todir, S_IRWXU) != 0)

I think the reason it's not failing is that it includes <fcntl.h>.
        regards, tom lane

On Sat, Dec 11, 2010 at 17:23, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Sat, Dec 11, 2010 at 17:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Ah, I have a theory: <fcntl.h>.
>
>> Nope, not there. I can't find S_IWGRP in any of the files.
>
> Well, I notice that copydir.c compiled before, and still compiles after,
> despite this change:
>
> -   if (mkdir(todir, S_IRUSR | S_IWUSR | S_IXUSR) != 0)
> +   if (mkdir(todir, S_IRWXU) != 0)
>
> I think the reason it's not failing is that it includes <fcntl.h>.

S_IRWXU is defined in port/win32.h...

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:
> On Sat, Dec 11, 2010 at 17:23, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think the reason it's not failing is that it includes <fcntl.h>.

> S_IRWXU is defined in port/win32.h...

No, it isn't.  There's an apparently-useless definition of _S_IRWXU
there, but no S_IRWXU.
        regards, tom lane

On Sat, Dec 11, 2010 at 17:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Sat, Dec 11, 2010 at 17:23, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I think the reason it's not failing is that it includes <fcntl.h>.
>
>> S_IRWXU is defined in port/win32.h...
>
> No, it isn't.  There's an apparently-useless definition of _S_IRWXU
> there, but no S_IRWXU.

Hmm. You're right, of course.

A search on my windows box finds the text string S_IRWXU in the
following "*.h" files across the whole filesystem:
c:\perl\lib\CORE\perl.h
c:\perl64\lib\CORE\perl.h
c:\pgsql\src\include\pg_config_os.h
c:\pgsql\src\include\port\win32.h

that's it.



--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:
> On Sat, Dec 11, 2010 at 17:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> No, it isn't. �There's an apparently-useless definition of _S_IRWXU
>> there, but no S_IRWXU.

> Hmm. You're right, of course.

> A search on my windows box finds the text string S_IRWXU in the
> following "*.h" files across the whole filesystem:
> c:\perl\lib\CORE\perl.h
> c:\perl64\lib\CORE\perl.h
> c:\pgsql\src\include\pg_config_os.h
> c:\pgsql\src\include\port\win32.h

> that's it.

OK, now I'm really confused.  We have at least two questions:

1. How did all those pre-existing references to S_IRXWU compile?

2. Why didn't the previously hard-wired constants passed to chmod
and umask fail on Windows?  The M$ documentation I can find at the
moment suggests that *only* _S_IREAD and _S_IWRITE bits are allowed
in the inputs to those functions, which apparently is untrue or none
of this code would have executed successfully.
        regards, tom lane

On Sat, Dec 11, 2010 at 17:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Sat, Dec 11, 2010 at 17:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> No, it isn't.  There's an apparently-useless definition of _S_IRWXU
>>> there, but no S_IRWXU.
>
>> Hmm. You're right, of course.
>
>> A search on my windows box finds the text string S_IRWXU in the
>> following "*.h" files across the whole filesystem:
>> c:\perl\lib\CORE\perl.h
>> c:\perl64\lib\CORE\perl.h
>> c:\pgsql\src\include\pg_config_os.h
>> c:\pgsql\src\include\port\win32.h
>
>> that's it.
>
> OK, now I'm really confused.  We have at least two questions:
>
> 1. How did all those pre-existing references to S_IRXWU compile?

Yeah, that's weird. IIRC (I stopped looking for the moment, need a
step back) some of them were protected by #ifndef WIN32, but not all
of them..


> 2. Why didn't the previously hard-wired constants passed to chmod
> and umask fail on Windows?  The M$ documentation I can find at the
> moment suggests that *only* _S_IREAD and _S_IWRITE bits are allowed
> in the inputs to those functions, which apparently is untrue or none
> of this code would have executed successfully.

Probably it ignores any flags it doesn't know about?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:
> On Sat, Dec 11, 2010 at 17:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 2. Why didn't the previously hard-wired constants passed to chmod
>> and umask fail on Windows? �The M$ documentation I can find at the
>> moment suggests that *only* _S_IREAD and _S_IWRITE bits are allowed
>> in the inputs to those functions, which apparently is untrue or none
>> of this code would have executed successfully.

> Probably it ignores any flags it doesn't know about?

Maybe, but unless _S_IREAD and _S_IWRITE are defined as 0400 and 0200,
the code would have been doing the wrong thing altogether.

I'm not sure why you think the group/other macros couldn't be #define'd
as 0?  But in any case there's still the question of where S_IRWXU is
coming from, since it clearly does work in several places.
        regards, tom lane

Magnus Hagander <magnus@hagander.net> writes:
> On Sat, Dec 11, 2010 at 17:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 1. How did all those pre-existing references to S_IRXWU compile?

> Yeah, that's weird. IIRC (I stopped looking for the moment, need a
> step back) some of them were protected by #ifndef WIN32, but not all
> of them..

The lightbulb just went on: in win32.h,

#define mkdir(a,b)    mkdir(a)

I didn't go through in complete detail, but I'll bet all the "working"
instances are in mkdir calls, or else inside #ifndef WIN32.

I think we can just #define the other cases as zeroes.  I'm not sure why
you think that's an issue for open --- the privileges don't exist.
        regards, tom lane

On Sat, Dec 11, 2010 at 18:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Sat, Dec 11, 2010 at 17:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> 1. How did all those pre-existing references to S_IRXWU compile?
>
>> Yeah, that's weird. IIRC (I stopped looking for the moment, need a
>> step back) some of them were protected by #ifndef WIN32, but not all
>> of them..
>
> The lightbulb just went on: in win32.h,
>
> #define mkdir(a,b)      mkdir(a)
>
> I didn't go through in complete detail, but I'll bet all the "working"
> instances are in mkdir calls, or else inside #ifndef WIN32.

Ah, that certainly looks like a smoking gun.


> I think we can just #define the other cases as zeroes.  I'm not sure why
> you think that's an issue for open --- the privileges don't exist.

Hmm. I was/am worried about any case that specifies *just* one of the
permissions that don't exist. That'll leave it at zero, whereas the
correct one might be the user-only version of whatever (read/write)
was given.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:
> On Sat, Dec 11, 2010 at 18:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think we can just #define the other cases as zeroes. �I'm not sure why
>> you think that's an issue for open --- the privileges don't exist.

> Hmm. I was/am worried about any case that specifies *just* one of the
> permissions that don't exist. That'll leave it at zero, whereas the
> correct one might be the user-only version of whatever (read/write)
> was given.

If we didn't specify the "user" read or write privilege, we shouldn't
get it.

What are the values of _S_IREAD and _S_IWRITE, anyway?  I'm still
wondering how come the previous coding with hardwired constants
behaved correctly.
        regards, tom lane

I wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Sat, Dec 11, 2010 at 18:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I think we can just #define the other cases as zeroes. �I'm not sure why
>>> you think that's an issue for open --- the privileges don't exist.

>> Hmm. I was/am worried about any case that specifies *just* one of the
>> permissions that don't exist. That'll leave it at zero, whereas the
>> correct one might be the user-only version of whatever (read/write)
>> was given.

> If we didn't specify the "user" read or write privilege, we shouldn't
> get it.

I put in #define's for these, and it seems to have fixed the MSVC
buildfarm members, but cygwin is still broken.  How come ... doesn't
that port use port/win32.h?

> What are the values of _S_IREAD and _S_IWRITE, anyway?  I'm still
> wondering how come the previous coding with hardwired constants
> behaved correctly.

Still curious about this.
        regards, tom lane

On 12/12/2010 11:16 AM, Tom Lane wrote:
>
> I put in #define's for these, and it seems to have fixed the MSVC
> buildfarm members, but cygwin is still broken.  How come ... doesn't
> that port use port/win32.h?
>

ITYM Mingw. And yes, it does use port/win32.h; narwhal's log says:
   config.status: linking src/include/port/win32.h to src/include/pg_config_os.h


cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 12/12/2010 11:16 AM, Tom Lane wrote:
>> I put in #define's for these, and it seems to have fixed the MSVC
>> buildfarm members, but cygwin is still broken.  How come ... doesn't
>> that port use port/win32.h?

> ITYM Mingw. And yes, it does use port/win32.h; narwhal's log says:
>     config.status: linking src/include/port/win32.h to src/include/pg_config_os.h

Oh, I guess the point is that WIN32_ONLY_COMPILER doesn't get defined,
and that block of file-permission-bit #defines is nested inside
#ifdef WIN32_ONLY_COMPILER.

So apparently the issue is that the mingw headers provide some but not
all of those symbols.  Which have they got, and how are they defined?
        regards, tom lane

On 12/13/2010 12:54 AM, Glen Knowles wrote:
> On Sun, Dec 12, 2010 at 8:16 AM, Tom Lane <tgl@sss.pgh.pa.us 
> <mailto:tgl@sss.pgh.pa.us>> wrote:
>
>     > What are the values of _S_IREAD and _S_IWRITE, anyway?  I'm still
>     > wondering how come the previous coding with hardwired constants
>     > behaved correctly.
>
>     Still curious about this.
>
> FWIW, _S_IREAD and _S_IWRITE are defined by Visual Studio C++ 2008 in 
> sys/stat.h as 0x0100 and 0x0080 respectively.
>
>

Mingw values are the same.

cheers

andrew