Prev: [HACKERS] log files and permissions
Next: [HACKERS] hello
From: Martin Pihlak on 12 Jul 2010 06:36
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

From: Fujii Masao on 12 Jul 2010 21:48
On Mon, Jul 12, 2010 at 7:36 PM, Martin Pihlak <martin.pihlak(a)gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Itagaki Takahiro on 13 Jul 2010 01:18
I think the patch is almost ready for committer except the following
three issues:

2010/7/13 Fujii Masao <masao.fujii(a)gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on 13 Jul 2010 10:31
Itagaki Takahiro <itagaki.takahiro(a)gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Martin Pihlak on 13 Jul 2010 08:16
Itagaki Takahiro wrote:
> I think the patch is almost ready for committer except the following
> three issues:
>
> 2010/7/13 Fujii Masao <masao.fujii(a)gmail.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

First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: [HACKERS] log files and permissions
Next: [HACKERS] hello