I agree with all the code cleanups suggested and have made then in the attached patch, to save the committer some time.
Also in this patch, I changed sgml para to
<para>
Changing table-level options requires superuser privileges, for security
reasons: only a superuser should be able to determine which file is read
or which program is run. In principle non-superusers could be allowed to
change the other options, but that's not supported at present.
</para>
"Determine" is an odd word in this context. I understand it to mean "set/change", but I can see where a less familiar reader would take the meaning to be "has permission to see the value already set". Either way, it now mentions program as an option in addition to filename.
On 2016/09/11 8:04, Corey Huinker wrote: > V2 of this patch: > > Changes: > * rebased to most recent master > * removed non-tap test that assumed the existence of Unix sed program > * added non-tap test that assumes the existence of perl > * switched from filename/program to filename/is_program to more closely > follow patterns in copy.c > * slight wording change in C comments
This version looks mostly good to me. Except some whitespace noise in some hunks:
/* * Only superusers are allowed to set options of a file_fdw foreign table. - * This is because the filename is one of those options, and we don't want - * non-superusers to be able to determine which file gets read. + * The reason for this is to prevent non-superusers from changing the
Space after "the"
- if (stat(filename, &stat_buf) == 0) + if ((! is_program) && (stat(filename, &stat_buf) == 0)))
Space between ! and is_program.
- if (strcmp(def->defname, "filename") == 0) + if ((strcmp(def->defname, "filename") == 0) || (strcmp(def->defname, "program") == 0))
I think the usual style would be to split the if statement into two lines as follows to keep within 80 characters per line [1]:
By the way, doesn't the following paragraph in file-fdw.sgml need an update?
<para> Changing table-level options requires superuser privileges, for security reasons: only a superuser should be able to determine which file is read. In principle non-superusers could be allowed to change the other options, but that's not supported at present. </para>
I would like to mark this now as "Ready for Committer".