Re: pg_upgrade's exec_prog() coding improvement

Hi,

I've been bitten twice by exec_prog()s API, so here's a patch to try to
make it a bit harder to misuse.

There are two main changes here; one is to put the logfile option as the
first argument; then comes a bool, then the format string. This means
you get a warning if you pass the wrong number of arguments before the
format; previously I mis-merged in the KEY SHARE patch so that I was
passing the log file as format, and the compiler didn't complain at all.

The other interesting change I did was move the responsibility of adding
SYSTEMQUOTE and the ">> %s 2>&1" redirections to exec_prog itself,
reducing clutter in the caller. This makes the callers a lot easier to
read.

One other minor change I did was split it in two versions: a simpler one
with less frammishes, that doesn't let you supply an alternative log
file and doesn't return a status value. This is used for all but one of
the callers; only that one caller was interested in the result value
anyway. And that caller is also the only one that passes an
opt_log_file other than NULL, so I removed that from the simple version.

Lastly, I removed the is_priv boolean, which seems pretty pointless;
just made all calls set the umask inconditionally. The only caller
doing this was the cp/xcopy call, and I don't see any reason for this to
be different from other callers.

This makes pg_upgrade a bit more readable.

If anyone can test this on Windows I would be appreciate it.

One problem with this is that I get this warning:

/pgsql/source/HEAD/contrib/pg_upgrade/exec.c: In function ‘s_exec_prog’:
/pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute]
/pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute]

I have no idea how to silence that. Ideas?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 23.08.2012 23:07, Alvaro Herrera wrote:
> One problem with this is that I get this warning:
>
> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c: In function ‘s_exec_prog’:
> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute]
> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute]
>
> I have no idea how to silence that. Ideas?

You can do what the warning suggests, and tell the compiler that
exec_prog takes printf-like arguments. See elog.h for an example of that:

extern int
errmsg(const char *fmt,...)
/* This extension allows gcc to check the format string for consistency with
the supplied arguments. */
__attribute__((format(PG_PRINTF_ATTRIBUTE, 1, 2)));

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> On 23.08.2012 23:07, Alvaro Herrera wrote:
>> One problem with this is that I get this warning:
>> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c: In function s_exec_prog:
>> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for gnu_printf format attribute [-Wmissing-format-attribute]
>> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for gnu_printf format attribute [-Wmissing-format-attribute]
>>
>> I have no idea how to silence that. Ideas?

> You can do what the warning suggests, and tell the compiler that
> exec_prog takes printf-like arguments.

exec_prog already has such decoration, and Alvaro's patch doesn't remove
it. So the question is, exactly what the heck does that warning mean?

regards, tom lane

On Fri, Aug 24, 2012 at 10:08:58AM -0400, Tom Lane wrote:
> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> > On 23.08.2012 23:07, Alvaro Herrera wrote:
> >> One problem with this is that I get this warning:
> >> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c: In function ‘s_exec_prog’:
> >> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute]
> >> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute]
> >>
> >> I have no idea how to silence that. Ideas?
>
> > You can do what the warning suggests, and tell the compiler that
> > exec_prog takes printf-like arguments.
>
> exec_prog already has such decoration, and Alvaro's patch doesn't remove
> it. So the question is, exactly what the heck does that warning mean?

It sounds like it is suggestioning to use more specific attribute
decoration. This might be relevant:

http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

-Wmissing-format-attribute
Warn about function pointers that might be candidates for format
attributes. Note these are only possible candidates, not absolute ones.
GCC guesses that function pointers with format attributes that are used
in assignment, initialization, parameter passing or return statements
should have a corresponding format attribute in the resulting type. I.e.
the left-hand side of the assignment or initialization, the type of the
parameter variable, or the return type of the containing function
respectively should also have a format attribute to avoid the warning.

GCC also warns about function definitions that might be candidates
for format attributes. Again, these are only possible candidates. GCC
guesses that format attributes might be appropriate for any function
that calls a function like vprintf or vscanf, but this might not always
be the case, and some functions for which format attributes are
appropriate may not be detected.

and I see this option enabled in configure.in.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> It sounds like it is suggestioning to use more specific attribute
> decoration. This might be relevant:

> http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

> -Wmissing-format-attribute
> Warn about function pointers that might be candidates for format
> attributes. Note these are only possible candidates, not absolute ones.
> GCC guesses that function pointers with format attributes that are used
> in assignment, initialization, parameter passing or return statements
> should have a corresponding format attribute in the resulting type. I.e.
> the left-hand side of the assignment or initialization, the type of the
> parameter variable, or the return type of the containing function
> respectively should also have a format attribute to avoid the warning.

> GCC also warns about function definitions that might be candidates
> for format attributes. Again, these are only possible candidates. GCC
> guesses that format attributes might be appropriate for any function
> that calls a function like vprintf or vscanf, but this might not always
> be the case, and some functions for which format attributes are
> appropriate may not be detected.

> and I see this option enabled in configure.in.

Hm. I'm a bit dubious about enabling warnings that are so admittedly
heuristic as this. They admit that the warnings might be wrong, and
yet document no way to shut them up. I think we might be better advised
to not enable this warning.

regards, tom lane

Actually it seems better to just get rid of the extra varargs function
and just have a single exec_prog. The extra NULL argument is not
troublesome enough, it seems. Updated version attached.

Again, win32 testing would be welcome. Sadly, buildfarm does not run
pg_upgrade's "make check".

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, 2012-08-24 at 10:08 -0400, Tom Lane wrote:
> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> > On 23.08.2012 23:07, Alvaro Herrera wrote:
> >> One problem with this is that I get this warning:
> >> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c: In function ‘s_exec_prog’:
> >> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute]
> >> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute]
> >>
> >> I have no idea how to silence that. Ideas?
>
> > You can do what the warning suggests, and tell the compiler that
> > exec_prog takes printf-like arguments.
>
> exec_prog already has such decoration, and Alvaro's patch doesn't remove
> it. So the question is, exactly what the heck does that warning mean?

The warning is about s_exec_prog, not exec_prog.

Excerpts from Alvaro Herrera's message of vie ago 24 11:44:33 -0400 2012:
> Actually it seems better to just get rid of the extra varargs function
> and just have a single exec_prog. The extra NULL argument is not
> troublesome enough, it seems. Updated version attached.

Applied (with some very minor changes).

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 08/24/2012 11:44 AM, Alvaro Herrera wrote:
>
> Again, win32 testing would be welcome. Sadly, buildfarm does not run
> pg_upgrade's "make check".

Yesterday I added a new module to the buildfarm client code to run this
(<https://github.com/PGBuildFarm/client-code/commit/ab812cb9920c65e39ec7358dc816371f1fef31eb>).
It required a couple of tweaks in the base code. This will be in a new
buildfarm client release fairly shortly. It's running on crake now, and
I will add it to pitta to get some Windows coverage.

It would be a lot nicer is the test were written in Perl, since we don't
necessarily have a Bourne shell available for MSVC builds, but we
definitely have Perl available.

None of this does what I think we really need, which is cross-release
pg_upgrade testing, which remains on my TODO list as a fairly high
priority item.

cheers

andrew

On 08/31/2012 10:52 AM, Andrew Dunstan wrote:
>
> On 08/24/2012 11:44 AM, Alvaro Herrera wrote:
>>
>> Again, win32 testing would be welcome. Sadly, buildfarm does not run
>> pg_upgrade's "make check".
>
>
>
> Yesterday I added a new module to the buildfarm client code to run
> this
> (<https://github.com/PGBuildFarm/client-code/commit/ab812cb9920c65e39ec7358dc816371f1fef31eb>).
> It required a couple of tweaks in the base code. This will be in a new
> buildfarm client release fairly shortly. It's running on crake now,
> and I will add it to pitta to get some Windows coverage.
>

but it's not working on pitta :-(

It will take me some time to work out why, and I have several more
urgent things on my plate. Meanwhile at least we have Linux coverage.

cheers

andrew