On 11/25/19 5:08 AM, Andrew Dunstan wrote:
On 11/11/19 4:28 PM, Mark Dilger wrote:
On further consideration, I'm wondering why we don't just
unconditionally use a closed input pty for all these functions (except
run_log). None of them use any input, and making the client worry
about
whether or not to close it seems something of an abstraction break.
There would be no API change at all involved in this case, just a
bit of
extra cleanliness. Would need testing on Windows, I'll go and do that
now.
Thoughts?
That sounds a lot better than your previous patch.
PostgresNode.pm and TestLib.pm both invoke IPC::Run::run. If you
change all the invocations in TestLib to close input pty, should you
do the same for PostgresNode? I don't have a strong argument for
doing so, but it seems cleaner to have both libraries invoking
commands under identical conditions, such that if commands were
borrowed from one library and called from the other they would behave
the same.
PostgresNode already uses TestLib, so perhaps setting up the
environment can be abstracted into something, perhaps TestLib::run,
and then used everywhere that IPC::Run::run currently is used.
I don't think we need to do that. In the case of the PostgresNode.pm
uses we know what the executable is, unlike the the TestLib.pm cases.
They are our own executables and we don't expect them to be doing
anything funky with /dev/tty.
Ok. I think your proposal sounds fine.
Here's a patch for that. The pty stuff crashes and burns on my Windows
test box, so there I just set stdin to an empty string via the usual
pipe mechanism.
Ok, I've reviewed and tested this. It works fine for me on Linux. I
am not set up to test it on Windows. I think it is ready to commit.
I have one remaining comment about the code, and this is just FYI. I
won't quibble with you committing your patch as it currently stands.
You might consider changing the '\x04' literal to use a named control
character, both for readability and portability, as here:
+ use charnames ':full';
+ @no_stdin = ('<pty<', \"\N{END OF TRANSMISSION}");
The only character set I can find where this matters is EBCDIC, in
which the EOT character is 55 rather than 4. Since EBCDIC does not
occur in the list of supported character sets for postgres, per the
docs section 23.3.1, I don't suppose it matters too much. Nor can
I test how this works on EBCDIC, so I'm mostly guessing that perl
would do the right thing there. But, at least to my eyes, it is
more immediately clear what the code is doing when the control
character name is spelled out.
--
Mark Dilger
