pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Fix headerscheck failure in replication/worker_internal.h
10+ messages / 3 participants
[nested] [flat]
* pgsql: Fix headerscheck failure in replication/worker_internal.h
@ 2021-11-16 16:33 Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 10+ messages in thread
From: Alvaro Herrera @ 2021-11-16 16:33 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Fix headerscheck failure in replication/worker_internal.h
Broken by 31c389d8de91
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/ad26ee28250c4cd357a7420161a2be321c3dd536
Modified Files
--------------
src/include/replication/worker_internal.h | 1 +
1 file changed, 1 insertion(+)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix headerscheck failure in replication/worker_internal.h
@ 2021-11-16 17:09 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 10+ messages in thread
From: Alvaro Herrera @ 2021-11-16 17:09 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
On 2021-Nov-16, Alvaro Herrera wrote:
> Fix headerscheck failure in replication/worker_internal.h
The other failure is in src/include/libpq/be-gssapi-common.h:
In file included from /tmp/headerscheck.a6f0um/test.c:2:
./src/include/libpq/be-gssapi-common.h:20:10: fatal error: gssapi/gssapi.h: No existe el fichero o el directorio
20 | #include <gssapi/gssapi.h>
| ^~~~~~~~~~~~~~~~~
compilation terminated.
One possible solution for this one is to add an exclusion in
headerscheck (and cpluspluscheck?); the other possible solution seems to
be to wrap the whole contents of the file in "#ifdef ENABLE_GSS". I
think the latter is roughly the approach used for OpenSSL inclusions.
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix headerscheck failure in replication/worker_internal.h
@ 2021-11-16 17:14 Stephen Frost <sfrost@snowman.net>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 10+ messages in thread
From: Stephen Frost @ 2021-11-16 17:14 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-hackers@lists.postgresql.org
Greetings,
* Alvaro Herrera (alvherre@alvh.no-ip.org) wrote:
> On 2021-Nov-16, Alvaro Herrera wrote:
>
> > Fix headerscheck failure in replication/worker_internal.h
>
> The other failure is in src/include/libpq/be-gssapi-common.h:
>
> In file included from /tmp/headerscheck.a6f0um/test.c:2:
> ./src/include/libpq/be-gssapi-common.h:20:10: fatal error: gssapi/gssapi.h: No existe el fichero o el directorio
> 20 | #include <gssapi/gssapi.h>
> | ^~~~~~~~~~~~~~~~~
> compilation terminated.
>
> One possible solution for this one is to add an exclusion in
> headerscheck (and cpluspluscheck?); the other possible solution seems to
> be to wrap the whole contents of the file in "#ifdef ENABLE_GSS". I
> think the latter is roughly the approach used for OpenSSL inclusions.
Short answer is yes, inclusion of be-gssapi-common.h is typically
wrapped in a #ifdef, see src/backend/libpq/auth.c
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (818B, ../../20211116171430.GI26257@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix headerscheck failure in replication/worker_internal.h
@ 2021-11-16 17:33 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 1 reply; 10+ messages in thread
From: Alvaro Herrera @ 2021-11-16 17:33 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: pgsql-hackers@lists.postgresql.org
On 2021-Nov-16, Stephen Frost wrote:
> Short answer is yes, inclusion of be-gssapi-common.h is typically
> wrapped in a #ifdef, see src/backend/libpq/auth.c
It'd be as in the attached, then.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Attachments:
[text/x-diff] 0001-Harden-gssapi.h-inclusion-for-headerscheck.patch (0B, ../../202111161733.y6r2myylxrj5@alvherre.pgsql/2-0001-Harden-gssapi.h-inclusion-for-headerscheck.patch)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix headerscheck failure in replication/worker_internal.h
@ 2021-11-16 17:47 Stephen Frost <sfrost@snowman.net>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 2 replies; 10+ messages in thread
From: Stephen Frost @ 2021-11-16 17:47 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-hackers@lists.postgresql.org
Greetings,
On Tue, Nov 16, 2021 at 12:33 Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:
> On 2021-Nov-16, Stephen Frost wrote:
>
> > Short answer is yes, inclusion of be-gssapi-common.h is typically
> > wrapped in a #ifdef, see src/backend/libpq/auth.c
>
> It'd be as in the attached, then.
I’ve not looked at this very closely but no, normally it’s the inclusion of
be-gssapi-common.h that’s wrapped in the ifdef, not the contents of it,
again, see auth.c
Not against possibly changing that but I don’t get the point of including
be-gssapi-common.h if it’s not enabled in the build and typically if GSSAPI
is possible and the reason for including be-gssapi-common.h then there’s
other things that need to be under a ifdef, again, as in auth.c
If there isn’t any need to be-gssapi-common.h then maybe just drop that
include instead of adding an ifdef..?
Thanks,
Stephen
>
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix headerscheck failure in replication/worker_internal.h
@ 2021-11-16 18:01 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 0 replies; 10+ messages in thread
From: Alvaro Herrera @ 2021-11-16 18:01 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: pgsql-hackers@lists.postgresql.org
On 2021-Nov-16, Stephen Frost wrote:
> I’ve not looked at this very closely but no, normally it’s the inclusion of
> be-gssapi-common.h that’s wrapped in the ifdef, not the contents of it,
> again, see auth.c
I don't think you read my original post very carefully. I'm talking
about sanitizing the output of the headerscheck script.
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
"Java is clearly an example of money oriented programming" (A. Stepanov)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix headerscheck failure in replication/worker_internal.h
@ 2021-11-16 18:16 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 2 replies; 10+ messages in thread
From: Alvaro Herrera @ 2021-11-16 18:16 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: pgsql-hackers@lists.postgresql.org
On 2021-Nov-16, Stephen Frost wrote:
> Not against possibly changing that but I don’t get the point of including
> be-gssapi-common.h if it’s not enabled in the build and typically if GSSAPI
> is possible and the reason for including be-gssapi-common.h then there’s
> other things that need to be under a ifdef, again, as in auth.c
BTW, this is exactly why my first suggestion was to add an exclusion
rule to headerscheck so that be-gssapi-common.h is not verified by that
script. After re-reading your response, that looks like a reasonable
answer too.
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix headerscheck failure in replication/worker_internal.h
@ 2021-11-16 18:57 Stephen Frost <sfrost@snowman.net>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 0 replies; 10+ messages in thread
From: Stephen Frost @ 2021-11-16 18:57 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-hackers@lists.postgresql.org
Greetings,
On Tue, Nov 16, 2021 at 13:16 Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:
> On 2021-Nov-16, Stephen Frost wrote:
>
> > Not against possibly changing that but I don’t get the point of including
> > be-gssapi-common.h if it’s not enabled in the build and typically if
> GSSAPI
> > is possible and the reason for including be-gssapi-common.h then there’s
> > other things that need to be under a ifdef, again, as in auth.c
>
> BTW, this is exactly why my first suggestion was to add an exclusion
> rule to headerscheck so that be-gssapi-common.h is not verified by that
> script. After re-reading your response, that looks like a reasonable
> answer too.
Yeah, that seems better to me, though I’ve not yet had time to look deeply
into any of this.
Thanks,
Stephen
>
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix headerscheck failure in replication/worker_internal.h
@ 2021-11-16 20:12 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 1 reply; 10+ messages in thread
From: Tom Lane @ 2021-11-16 20:12 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Stephen Frost <sfrost@snowman.net>; pgsql-hackers@lists.postgresql.org
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Nov-16, Stephen Frost wrote:
>> Not against possibly changing that but I don’t get the point of including
>> be-gssapi-common.h if it’s not enabled in the build and typically if GSSAPI
>> is possible and the reason for including be-gssapi-common.h then there’s
>> other things that need to be under a ifdef, again, as in auth.c
> BTW, this is exactly why my first suggestion was to add an exclusion
> rule to headerscheck so that be-gssapi-common.h is not verified by that
> script. After re-reading your response, that looks like a reasonable
> answer too.
I think adding #ifdef ENABLE_GSS as per your prior message is better.
Headers have little business making assumptions about the context in
which they're included --- which is exactly why headerscheck exists ---
so I disagree with Stephen's argument. In any case I am not in favor of
making random exclusions from that script's testing.
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix headerscheck failure in replication/worker_internal.h
@ 2021-11-16 20:16 Stephen Frost <sfrost@snowman.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 10+ messages in thread
From: Stephen Frost @ 2021-11-16 20:16 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-hackers@lists.postgresql.org
Greetings,
On Tue, Nov 16, 2021 at 15:12 Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > On 2021-Nov-16, Stephen Frost wrote:
> >> Not against possibly changing that but I don’t get the point of
> including
> >> be-gssapi-common.h if it’s not enabled in the build and typically if
> GSSAPI
> >> is possible and the reason for including be-gssapi-common.h then there’s
> >> other things that need to be under a ifdef, again, as in auth.c
>
> > BTW, this is exactly why my first suggestion was to add an exclusion
> > rule to headerscheck so that be-gssapi-common.h is not verified by that
> > script. After re-reading your response, that looks like a reasonable
> > answer too.
>
> I think adding #ifdef ENABLE_GSS as per your prior message is better.
> Headers have little business making assumptions about the context in
> which they're included --- which is exactly why headerscheck exists ---
> so I disagree with Stephen's argument. In any case I am not in favor of
> making random exclusions from that script's testing.
I don’t feel all that strongly either way, so if you’d rather have it that
way then that’s fine. Will still need the other ifdefs too anyway though,
but I guess it isn’t that big of a deal.
Thanks,
Stephen
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2021-11-16 20:16 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-11-16 16:33 pgsql: Fix headerscheck failure in replication/worker_internal.h Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-11-16 17:09 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-11-16 17:14 ` Stephen Frost <sfrost@snowman.net>
2021-11-16 17:33 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-11-16 17:47 ` Stephen Frost <sfrost@snowman.net>
2021-11-16 18:01 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-11-16 18:16 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-11-16 18:57 ` Stephen Frost <sfrost@snowman.net>
2021-11-16 20:12 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-11-16 20:16 ` Stephen Frost <sfrost@snowman.net>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox