Hi,
On 2021-10-03 10:18:31 -0700, Andres Freund wrote:
> As you can see in the test output, every mismatch prints the whole file,
> despite only intending to show the tail. Which appears to be because the
> windows portion of 3c5b0685b921 doesn't actually work. The reason for that in
> turn is that afaict the setFilePointer doesn't change the file position in a
> way that affects perl.
>
> Consequently, if I force the !win32 path, the tests pass.
>
> At first I assumed the cause of this is that while the setFilePointer()
> modifies the
> state of the underlying handle, it doesn't actually let perl know about
> that. Due to buffering etc perl likely has its own bookeeping about the
> position in the file. There's some pretty clear hints in
> https://perldoc.perl.org/functions/seek
>
> But the problem turns out to be that it's bogus to pass $fh to
> setFilePointer(). That's a perl handle, not an win32 handle. Fixing that seems
> to make the tests pass.
It does (I only let it run to the ssl test, then pushed a newer revision):
https://cirrus-ci.com/task/5345293928497152?logs=ssl#L5
> Why did 3c5b0685b921 choose to use setFilePointer() in the first place? At
> this point it's a perl filehandle, so we should just use perl seek?
>
>
> Leaving the concrete breakage aside, I'm somewhat unhappy that there's not a
> single comment explaining why TestLib.pm is trying to use native windows
> APIs.
>
> Isn't the code as-is also "leaking" an open IO::Handle? There's a
> CloseHandle($fHandle), but nothing is done to $fh. But perhaps there's some
> perl magic cleaning things up? Even if so, loks like just closing $fh will
> close the handle as well...
I think something roughly like the attached might be a good idea. Runs locally
on linux, and hopefully still on windows
https://cirrus-ci.com/build/4857291573821440
Greetings,
Andres Freund
>From e84841e58acb20ba0f17ed88d6455deefb265a57 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Sun, 3 Oct 2021 10:07:42 -0700
Subject: [PATCH] WIP: Fix TestLib::slurp_file() with offset on windows.
---
src/test/perl/TestLib.pm | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 15f4e6f56e3..2171e93d8ff 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -492,33 +492,33 @@ sub slurp_file
my ($filename, $offset) = @_;
local $/;
my $contents;
+ my $fh;
+
+ # On windows open file using win32 APIs, to allow us to set the
+ # FILE_SHARE_DELETE flag ("d" below), otherwise other accesses to the file
+ # may fail.
if ($Config{osname} ne 'MSWin32')
{
- open(my $in, '<', $filename)
+ open($fh, '<', $filename)
or croak "could not read \"$filename\": $!";
- if (defined($offset))
- {
- seek($in, $offset, SEEK_SET)
- or croak "could not seek \"$filename\": $!";
- }
- $contents = <$in>;
- close $in;
}
else
{
my $fHandle = createFile($filename, "r", "rwd")
or croak "could not open \"$filename\": $^E";
- OsFHandleOpen(my $fh = IO::Handle->new(), $fHandle, 'r')
+ OsFHandleOpen($fh = IO::Handle->new(), $fHandle, 'r')
or croak "could not read \"$filename\": $^E\n";
- if (defined($offset))
- {
- setFilePointer($fh, $offset, qw(FILE_BEGIN))
- or croak "could not seek \"$filename\": $^E\n";
- }
- $contents = <$fh>;
- CloseHandle($fHandle)
- or croak "could not close \"$filename\": $^E\n";
}
+
+ if (defined($offset))
+ {
+ seek($fh, $offset, SEEK_SET)
+ or croak "could not seek \"$filename\": $!";
+ }
+
+ $contents = <$fh>;
+ close $fh;
+
$contents =~ s/\r\n/\n/g if $Config{osname} eq 'msys';
return $contents;
}
--
2.32.0.rc2
