Thread: 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’ formatattribute [-Wmissing-format-attribute] /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ formatattribute [-Wmissing-format-attribute] I have no idea how to silence that. Ideas? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
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’ formatattribute [-Wmissing-format-attribute] > /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ formatattribute [-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.linnakangas@enterprisedb.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’
formatattribute [-Wmissing-format-attribute]
>> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’
formatattribute [-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.linnakangas@enterprisedb.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’ formatattribute [-Wmissing-format-attribute] > >> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ formatattribute [-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 mightbe candidates for formatattributes. Note these are only possible candidates, not absolute ones.GCC guesses that functionpointers with format attributes that are usedin assignment, initialization, parameter passing or return statementsshouldhave a corresponding format attribute in the resulting type. I.e.the left-hand side of the assignment orinitialization, the type of theparameter variable, or the return type of the containing functionrespectively should alsohave a format attribute to avoid the warning. GCC also warns about function definitions that might be candidatesforformat attributes. Again, these are only possible candidates. GCCguesses that format attributes might be appropriatefor any functionthat calls a function like vprintf or vscanf, but this might not alwaysbe the case, and some functionsfor which format attributes areappropriate may not be detected. and I see this option enabled in configure.in. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.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
Attachment
On Fri, 2012-08-24 at 10:08 -0400, Tom Lane wrote: > Heikki Linnakangas <heikki.linnakangas@enterprisedb.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’ formatattribute [-Wmissing-format-attribute] > >> /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ formatattribute [-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