be-secure-gssapi.c and auth.c with setenv() not compatible on Windows
Hi all,
Now that hamerkop has been fixed and that we have some coverage with
builds of GSSAPI on Windows thanks to 02511066, the buildfarm has been
complaining about a build failure on Windows for 12 and 13:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=hamerkop&dt=2021-05-28%2011%3A06%3A18&stg=make
The logs are hard to decrypt, but I guess that this is caused by the
use of setenv() in be-secure-gssapi.c and auth.c, as the tree has no
implementation that MSVC could feed on for those branches.
The recent commit 7ca37fb has changed things so as setenv() is used
instead of putenv(), and provides a fallback implementation, which
explains why the compilation of be-secure-gssapi.c and auth.c works
with MSVC, as reported by hamerkop.
We can do two things here:
1) Switch be-secure-gssapi.c and auth.c to use putenv().
2) Backport into 12 and 13 the fallback implementation of setenv
introduced in 7ca37fb, and keep be-secure-gssapi.c as they are now.
It is worth noting that 860fe27 mentions the use of setenv() in
be-secure-gssapi.c but has done nothing for it. I would choose 1), on
the ground that adding a new file on back-branches adds an additional
cost to Windows maintainers if they use their own MSVC scripts (I know
of one case here that would be impacted), and that does not seem
mandatory here as putenv() would just work.
Thoughts?
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
We can do two things here:
1) Switch be-secure-gssapi.c and auth.c to use putenv().
2) Backport into 12 and 13 the fallback implementation of setenv
introduced in 7ca37fb, and keep be-secure-gssapi.c as they are now.
There's a lot of value in keeping the branches looking alike.
On the other hand, 7ca37fb hasn't survived contact with the
public yet, so I'm a bit nervous about it.
It's not clear to me how much of 7ca37fb you're envisioning
back-patching in (2). I think it'd be best to back-patch
only the addition of pgwin32_setenv, and then let the gssapi
code use it. In that way, if there's anything wrong with
pgwin32_setenv, we're only breaking code that never worked
on Windows before anyway.
regards, tom lane
On Fri, May 28, 2021 at 11:37:22AM -0400, Tom Lane wrote:
There's a lot of value in keeping the branches looking alike.
On the other hand, 7ca37fb hasn't survived contact with the
public yet, so I'm a bit nervous about it.
I don't think this set of complications is worth the risk
destabilizing those stable branches.
It's not clear to me how much of 7ca37fb you're envisioning
back-patching in (2). I think it'd be best to back-patch
only the addition of pgwin32_setenv, and then let the gssapi
code use it. In that way, if there's anything wrong with
pgwin32_setenv, we're only breaking code that never worked
on Windows before anyway.
Just to be clear, for 2) I was thinking to pick up the minimal parts
you have changed in win32env.c and add src/port/setenv.c to add the
fallback implementation of setenv(), without changing anything else.
This also requires grabbing the small changes within pgwin32_putenv(),
visibly.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Fri, May 28, 2021 at 11:37:22AM -0400, Tom Lane wrote:
It's not clear to me how much of 7ca37fb you're envisioning
back-patching in (2). I think it'd be best to back-patch
only the addition of pgwin32_setenv, and then let the gssapi
code use it. In that way, if there's anything wrong with
pgwin32_setenv, we're only breaking code that never worked
on Windows before anyway.
Just to be clear, for 2) I was thinking to pick up the minimal parts
you have changed in win32env.c and add src/port/setenv.c to add the
fallback implementation of setenv(), without changing anything else.
This also requires grabbing the small changes within pgwin32_putenv(),
visibly.
What I had in mind was to *only* add pgwin32_setenv, not setenv.c.
There's no evidence that any other modern platform lacks setenv.
Moreover, there's no issue in these branches unless your platform
lacks setenv yet has GSS support.
regards, tom lane
On Sat, May 29, 2021 at 10:44:14AM -0400, Tom Lane wrote:
What I had in mind was to *only* add pgwin32_setenv, not setenv.c.
There's no evidence that any other modern platform lacks setenv.
Moreover, there's no issue in these branches unless your platform
lacks setenv yet has GSS support.
I have been finally able to poke at that, resulting in the attached.
You are right that adding only the fallback implementation for
setenv() seems to be enough. I cannot get my environment to complain,
and the code compiles.
--
Michael
Attachments:
0001-Add-fallback-implementation-for-setenv.patchtext/x-diff; charset=us-asciiDownload+81-4
On Mon, May 31, 2021 at 09:14:36AM +0900, Michael Paquier wrote:
I have been finally able to poke at that, resulting in the attached.
You are right that adding only the fallback implementation for
setenv() seems to be enough. I cannot get my environment to complain,
and the code compiles.
Okay, applied this stuff to 12 and 13 to take care of the build
failures with hamerkop. The ECPG tests should also turn back to green
there.
--
Michael
On Tue, Jun 01, 2021 at 10:14:49AM +0900, Michael Paquier wrote:
Okay, applied this stuff to 12 and 13 to take care of the build
failures with hamerkop. The ECPG tests should also turn back to green
there.
hamerkop has reported back, and things are now good on those
branches. Now for the remaining issue of HEAD..
--
Michael