Lists: | pgsql-hackers |
---|
From: | Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> |
---|---|
To: | Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: log files and permissions |
Date: | 2010-07-12 05:57:45 |
Message-ID: | AANLkTimeEc6TCk8EJDJazNmweYBs8YXvORknptWksive@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
I checked "log_file_mode GUC" patch, and found a couple of Windows-specific
and translation issues.
* fchmod() is not available on some platforms, including Windows.
fh = fopen(filename, mode);
setvbuf(fh, NULL, LBF_MODE, 0);
fchmod(fileno(fh), Log_file_mode);
I think umask()->fopen() is better rather than fopen()->chmod().
See codes in DoCopyTo() at commands/copy.c.
* How does the file mode work on Windows?
If it doesn't work, we should explain it in docs.
Description for .pgpass for Windows might be a help.
| http://developer.postgresql.org/pgdocs/postgres/libpq-pgpass.html
| On Microsoft Windows, ... no special permissions check is made.
* This message format is hard to translate.
ereport(am_rotating ? LOG : FATAL,
(errcode_for_file_access(),
(errmsg("could not create%slog file \"%s\": %m",
am_rotating ? " new " : " ", filename))));
It might look a duplication of codes, but I think this form is better
because we can reuse the existing translation catalogs.
if (am_rotating)
ereport(FATAL, ... "could not create log file ...);
else
ereport(LOG, ... "could not open new log file ...);
--
Itagaki Takahiro
From: | Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> |
---|---|
To: | Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: log files and permissions |
Date: | 2010-07-12 10:36:37 |
Message-ID: | 4C3AF035.4090603@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Itagaki Takahiro wrote:
> I checked "log_file_mode GUC" patch, and found a couple of Windows-specific
> and translation issues.
Thank you for the review. Attached patch attempts to fix these issues.
> * fchmod() is not available on some platforms, including Windows.
> fh = fopen(filename, mode);
> setvbuf(fh, NULL, LBF_MODE, 0);
> fchmod(fileno(fh), Log_file_mode);
>
I've changed that to using chmod(), that should be available everywhere and
is already used in several places in Postgres code.
> * How does the file mode work on Windows?
> If it doesn't work, we should explain it in docs.
Indeed it seems that chmod() doesn't actually do anything useful on Windows.
So I've added a documentation note about it and put an #ifndef WIN32 around
the chmod() call.
> It might look a duplication of codes, but I think this form is better
> because we can reuse the existing translation catalogs.
> if (am_rotating)
> ereport(FATAL, ... "could not create log file ...);
> else
> ereport(LOG, ... "could not open new log file ...);
>
Fixed.
regards,
Martin
Attachment | Content-Type | Size |
---|---|---|
log-file-mode.patch | text/x-diff | 9.4 KB |
From: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
---|---|
To: | Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> |
Cc: | Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: log files and permissions |
Date: | 2010-07-13 01:48:59 |
Message-ID: | AANLkTim4ljryk4wsylNlF8PAuM5SaRynKUSvFACHgJt1@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On Mon, Jul 12, 2010 at 7:36 PM, Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> wrote:
> Itagaki Takahiro wrote:
>> I checked "log_file_mode GUC" patch, and found a couple of Windows-specific
>> and translation issues.
>
> Thank you for the review. Attached patch attempts to fix these issues.
> + if (!*value || *endptr || file_mode < 0 || file_mode > 0777)
> + {
> + ereport(GUC_complaint_elevel(source),
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("invalid value for parameter \"log_file_mode\"")));
The sticky bit cannot be set via log_file_mode. Is this intentional?
> +#ifndef WIN32
> + chmod(filename, Log_file_mode);
> +#endif
Don't we need to check the return value of chmod()?
> +const char *assign_log_file_mode(const char *value,
> + bool doit, GucSource source);
> +const char *show_log_file_mode(void);
You forgot to write the show_log_file_mode()? I was not able to find that
in the patch.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
From: | Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> |
---|---|
To: | Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: log files and permissions |
Date: | 2010-07-13 05:18:00 |
Message-ID: | AANLkTikiiKM2_Pz66U9rarkD4MJdDBnpL1O3K-i5fIiE@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
I think the patch is almost ready for committer except the following
three issues:
2010/7/13 Fujii Masao <masao(dot)fujii(at)gmail(dot)com>:
>> + if (!*value || *endptr || file_mode < 0 || file_mode > 0777)
> The sticky bit cannot be set via log_file_mode. Is this intentional?
We should also check the value not to be something like 0699.
How about checking it with (file_mode & ~0666) != 0 ?
>> +#ifndef WIN32
>> + chmod(filename, Log_file_mode);
>> +#endif
> Don't we need to check the return value of chmod()?
I prefer umask() rather than chmod() here.
>> +const char *show_log_file_mode(void);
> You forgot to write the show_log_file_mode()? I was not able to find that
> in the patch.
I want show_log_file_mode to print the setting value in octal format.
--
Itagaki Takahiro
From: | Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> |
---|---|
To: | Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> |
Cc: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: log files and permissions |
Date: | 2010-07-13 12:16:08 |
Message-ID: | 4C3C5908.2010909@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Itagaki Takahiro wrote:
> I think the patch is almost ready for committer except the following
> three issues:
>
> 2010/7/13 Fujii Masao <masao(dot)fujii(at)gmail(dot)com>:
>>> + if (!*value || *endptr || file_mode < 0 || file_mode > 0777)
>> The sticky bit cannot be set via log_file_mode. Is this intentional?
Yes -- I don't think there is a use case for sticky or setuid bits on log
files, even allowing execute is questionable.
> We should also check the value not to be something like 0699.
> How about checking it with (file_mode & ~0666) != 0 ?
Aha, that would ensure that the execute bit is not specified. Works for me.
The 0699 and other invalid octal values are caught by strtol()
>>> +#ifndef WIN32
>>> + chmod(filename, Log_file_mode);
>>> +#endif
>> Don't we need to check the return value of chmod()?
>
> I prefer umask() rather than chmod() here.
>
Converted to umask()
>>> +const char *show_log_file_mode(void);
>> You forgot to write the show_log_file_mode()? I was not able to find that
>> in the patch.
>
> I want show_log_file_mode to print the setting value in octal format.
>
I've now (re)added the show_log_file_mode(). It used to be there, but then
at some point I decided to display the value "as-is".
While going through it, I moved the _setmode() call for win32 to logfile_open().
regards,
Martin
Attachment | Content-Type | Size |
---|---|---|
log-file-mode.patch | text/x-diff | 10.2 KB |
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> |
Cc: | Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: log files and permissions |
Date: | 2010-07-13 14:31:25 |
Message-ID: | 28347.1279031485@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> ...
> We should also check the value not to be something like 0699.
> How about checking it with (file_mode & ~0666) != 0 ?
> ...
> I want show_log_file_mode to print the setting value in octal format.
It seems like a whole lot of lily-gilding is going on here. Just make
it work like unix_socket_permissions already does. That's been there
for years and nobody has complained about it.
regards, tom lane
From: | Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: log files and permissions |
Date: | 2010-07-16 10:15:41 |
Message-ID: | 4C40314D.8020601@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Tom Lane wrote:
> Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
>> ...
>> We should also check the value not to be something like 0699.
>> How about checking it with (file_mode & ~0666) != 0 ?
>> ...
>> I want show_log_file_mode to print the setting value in octal format.
>
> It seems like a whole lot of lily-gilding is going on here. Just make
> it work like unix_socket_permissions already does. That's been there
> for years and nobody has complained about it.
>
Thanks, somehow I missed that we can already specify octal integers
as GUC-s. I now converted the log_file_mode to integer and dropped
the assign_log_file_mode function.
regards,
Martin
Attachment | Content-Type | Size |
---|---|---|
log-file-mode.patch | text/x-diff | 9.3 KB |
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> |
Cc: | Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: log files and permissions |
Date: | 2010-07-16 22:34:59 |
Message-ID: | 24717.1279319699@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> writes:
> Thanks, somehow I missed that we can already specify octal integers
> as GUC-s. I now converted the log_file_mode to integer and dropped
> the assign_log_file_mode function.
Applied with a few corrections. The noncosmetic changes were:
* prevent Log_file_mode from disabling S_IWUSR permissions --- we had
better be able to write the files no matter what.
* save and restore errno across ereport() call; needed since some
callers look at errno after a failure.
* make unix_socket_permissions print its value in octal, for consistency
with log_file_mode.
BTW, I'm not 100% convinced that having the octal show-functions is
a good idea, mainly because they aren't consistent with the other
columns in pg_settings:
regression=# select * from pg_settings where name = 'log_file_mode';
name | setting | unit | category |
short_desc |
extra_desc
| co
ntext | vartype | source | min_val | max_val | enumvals | boot_val | reset_val
| sourcefile | sourceline
---------------+---------+------+--------------------------------------+--------
----------------------------------+---------------------------------------------
--------------------------------------------------------------------------------
----------------------------------------------------------------------------+---
------+---------+---------+---------+---------+----------+----------+-----------
+------------+------------
log_file_mode | 0600 | | Reporting and Logging / Where to Log | Sets th
e file permissions for log files. | The parameter value is expected to be a nume
ric mode specification in the form accepted by the chmod and umask system calls.
(To use the customary octal format the number must start with a 0 (zero).) | si
ghup | integer | default | 0 | 511 | | 384 | 384
| |
(1 row)
I guess this is not strictly incorrect, as long as you understand what
the leading '0' means per C conventions, but it looks a bit weird.
However, we're not going to be able to improve on this without a lot more
hackery than I think it's worth.
regards, tom lane