Skip to content

Complete O_flags used for file open #1853

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 27, 2024
Merged

Complete O_flags used for file open #1853

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c360095
Complete O_flags used for file open
BCSharp Dec 26, 2024
265b163
Update after review
BCSharp Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 24 additions & 11 deletions Src/IronPython/Modules/_fileio.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,11 @@ public FileIO(CodeContext/*!*/ context, string name, string mode = "r", bool clo
}

if (!_context.FileManager.TryGetStreams(fd, out _streams)) {
// TODO: This is not necessarily an error on Posix.
// The descriptor could have been opened by a different means than os.open.
// In such case:
// _streams = new(new UnixStream(fd, ownsHandle: true))
// _context.FileManager.Add(fd, _streams);
throw PythonOps.OSError(PythonFileManager.EBADF, "Bad file descriptor");
}
} else {
Expand All @@ -163,34 +168,40 @@ public FileIO(CodeContext/*!*/ context, string name, string mode = "r", bool clo
}

private static string NormalizeMode(string mode, out int flags) {
flags = 0;
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
flags |= O_NOINHERIT | O_BINARY;
} else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
flags |= O_CLOEXEC;
}
switch (StandardizeMode(mode)) {
case "r":
flags = O_RDONLY;
flags |= O_RDONLY;
return "rb";
case "w":
flags = O_CREAT | O_TRUNC | O_WRONLY;
flags |= O_CREAT | O_TRUNC | O_WRONLY;
return "wb";
case "a":
flags = O_APPEND | O_CREAT;
flags |= O_APPEND | O_CREAT | O_WRONLY;
return "ab";
case "x":
flags = O_CREAT | O_EXCL;
flags |= O_CREAT | O_EXCL | O_WRONLY;
return "xb";
case "r+":
case "+r":
flags = O_RDWR;
flags |= O_RDWR;
return "rb+";
case "w+":
case "+w":
flags = O_CREAT | O_TRUNC | O_RDWR;
flags |= O_CREAT | O_TRUNC | O_RDWR;
return "wb+";
case "a+":
case "+a":
flags = O_APPEND | O_CREAT | O_RDWR;
flags |= O_APPEND | O_CREAT | O_RDWR;
return "ab+";
case "x+":
case "+x":
flags = O_CREAT | O_RDWR | O_EXCL;
flags |= O_CREAT | O_EXCL | O_RDWR;
return "xb+";
default:
throw BadMode(mode);
Expand Down Expand Up @@ -225,14 +236,14 @@ static Exception BadMode(string mode) {
case 'a':
case 'x':
if (foundMode) {
return PythonOps.ValueError("Must have exactly one of create/read/write/append mode and at most one plus");
return BadModeException();
} else {
foundMode = true;
continue;
}
case '+':
if (foundPlus) {
return PythonOps.ValueError("Must have exactly one of create/read/write/append mode and at most one plus");
return BadModeException();
} else {
foundPlus = true;
continue;
Expand All @@ -245,7 +256,9 @@ static Exception BadMode(string mode) {
}
}

return PythonOps.ValueError("Must have exactly one of create/read/write/append mode and at most one plus");
return BadModeException();

static Exception BadModeException() => PythonOps.ValueError("Must have exactly one of create/read/write/append mode and at most one plus");
}
}

Expand Down
16 changes: 15 additions & 1 deletion Src/IronPython/Modules/_io.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Licensed to the .NET Foundation under one or more agreements.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

Expand All @@ -12,6 +12,7 @@
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Runtime.Versioning;
using System.Text;

using Microsoft.Scripting.Runtime;
Expand Down Expand Up @@ -54,6 +55,19 @@ public static partial class PythonIOModule {

private static int O_EXCL => RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? 0x400 : RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? 0x800 : 0x80;

[PythonHidden(PlatformsAttribute.PlatformFamily.Windows)]
Copy link
Contributor

@slozier slozier Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having PythonHidden on a private seems kind of odd but I guess it doesn't hurt...

Copy link
Member Author

@BCSharp BCSharp Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it doesn't hurt, but the less verbosity the better. I assume the same applies to internal, though I see there are a few PythonHidden that are internal already in the codebase.

Copy link
Contributor

@slozier slozier Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, I wonder why we'd have them on internal members. Maybe something that was public at some point?

Copy link
Member Author

@BCSharp BCSharp Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look for example at ByteArray.UnsafeByteList. It was established 5 years ago as internal, but 4 years ago you added PythonHidden in #1000. Do you remember why?

Copy link
Contributor

@slozier slozier Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't say I remember, but looking at #693 I changed Bytes.GetUnsafeByteArray (which was public) to a public property and then in a follow up commit made it internal. I guess I forgot to remove the PythonHidden when I did this. ByteArray.UnsafeByteList is probably a copy/paste...

[SupportedOSPlatform("linux")]
[SupportedOSPlatform("macos")]
private static int O_CLOEXEC => RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? 0x1000000 : 0x80000;

[PythonHidden(PlatformsAttribute.PlatformFamily.Unix)]
[SupportedOSPlatform("windows")]
private static int O_BINARY => 0x8000;

[PythonHidden(PlatformsAttribute.PlatformFamily.Unix)]
[SupportedOSPlatform("windows")]
private static int O_NOINHERIT => 0x80;

// *** END GENERATED CODE ***

#endregion
Expand Down
2 changes: 1 addition & 1 deletion Src/Scripts/generate_os_codes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ def generate_O_flags(cw, flagvalues, access):
def generate_all_O_flags(cw):
generate_O_flags(cw, O_flagvalues, 'public')

common_O_flags = ['O_RDONLY', 'O_WRONLY', 'O_RDWR', 'O_APPEND', 'O_CREAT', 'O_TRUNC', 'O_EXCL']
common_O_flags = ['O_RDONLY', 'O_WRONLY', 'O_RDWR', 'O_APPEND', 'O_CREAT', 'O_TRUNC', 'O_EXCL', 'O_CLOEXEC', 'O_BINARY', 'O_NOINHERIT']

def generate_common_O_flags(cw):
generate_O_flags(cw, OrderedDict((f, O_flagvalues[f]) for f in common_O_flags), 'private')
Expand Down
43 changes: 38 additions & 5 deletions Tests/test_file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

CP16623_LOCK = _thread.allocate_lock()

from iptest import IronPythonTestCase, is_cli, is_cpython, is_netcoreapp, is_posix, run_test, skipUnlessIronPython
from iptest import IronPythonTestCase, is_cli, is_cpython, is_netcoreapp, is_posix, is_linux, is_osx, is_windows, run_test, skipUnlessIronPython

class FileTest(IronPythonTestCase):

Expand Down Expand Up @@ -505,12 +505,12 @@ def test_file_manager_leak(self):
# the number of iterations should be larger than Microsoft.Scripting.Utils.HybridMapping.SIZE (currently 4K)
N = 5000
for i in range(N):
fd = os.open(self.temp_file, os.O_WRONLY | os.O_CREAT)
fd = os.open(self.temp_file, os.O_WRONLY | os.O_CREAT | os.O_TRUNC)
f = os.fdopen(fd, 'w', closefd=True)
f.close()

for i in range(N):
fd = os.open(self.temp_file, os.O_WRONLY | os.O_CREAT)
fd = os.open(self.temp_file, os.O_WRONLY | os.O_CREAT | os.O_TRUNC)
f = os.fdopen(fd, 'w', closefd=False)
g = os.fdopen(f.fileno(), 'w', closefd=True)
g.close()
Expand Down Expand Up @@ -624,7 +624,7 @@ def test_modes(self):
self.assertRaisesMessage(ValueError, "invalid mode: 'p'", open, 'abc', 'p')

# allow anything w/ U but r and w
err_msg = "mode U cannot be combined with 'x', 'w', 'a', or '+'" if is_cli or sys.version_info >= (3,7) else "mode U cannot be combined with x', 'w', 'a', or '+'" if sys.version_info >= (3,6) else "can't use U and writing mode at once"
err_msg = "mode U cannot be combined with 'x', 'w', 'a', or '+'" if is_cli or sys.version_info >= (3,7,4) else "mode U cannot be combined with x', 'w', 'a', or '+'" if sys.version_info >= (3,6) else "can't use U and writing mode at once"
self.assertRaisesMessage(ValueError, err_msg, open, 'abc', 'Uw')
self.assertRaisesMessage(ValueError, err_msg, open, 'abc', 'Ua')
self.assertRaisesMessage(ValueError, err_msg, open, 'abc', 'Uw+')
Expand Down Expand Up @@ -701,7 +701,7 @@ def test_errors(self):

with self.assertRaises(OSError) as cm:
open('path_too_long' * 100)
self.assertEqual(cm.exception.errno, (36 if is_posix else 22) if is_netcoreapp and not is_posix or sys.version_info >= (3,6) else 2)
self.assertEqual(cm.exception.errno, (63 if is_osx else 36 if is_linux else 22) if is_netcoreapp and not is_posix or sys.version_info >= (3,6) else 2)
Copy link
Contributor

@slozier slozier Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I mind the explicit number check, but wondering if we should be using the errno module values instead to make things easier to decipher.

Copy link
Member Author

@BCSharp BCSharp Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, assuming that errno is not buggy 😄


def test_write_bytes(self):
fname = self.temp_file
Expand Down Expand Up @@ -756,6 +756,39 @@ def test_open_with_BOM(self):
with open(fileName, "rb") as f:
self.assertEqual(f.read(), b"\xef\xbb\xbf\x42\xc3\x93\x4d\x0d\x0a")


def test_open_flags(self):
test_data = {
'rb': os.O_RDONLY,
'wb': os.O_WRONLY | os.O_CREAT | os.O_TRUNC,
'ab': os.O_WRONLY | os.O_CREAT | os.O_APPEND,
'xb': os.O_WRONLY | os.O_CREAT | os.O_EXCL,
'rb+': os.O_RDWR,
'wb+': os.O_RDWR | os.O_CREAT | os.O_TRUNC,
'ab+': os.O_RDWR | os.O_CREAT | os.O_APPEND,
'xb+': os.O_RDWR | os.O_CREAT | os.O_EXCL,
}
extra_flags = 0
if is_posix:
extra_flags |= os.O_CLOEXEC
elif is_windows:
extra_flags |= os.O_NOINHERIT | os.O_BINARY
test_data = {k: v | extra_flags for k, v in test_data.items()}

flags_received = None
def test_open(name, flags):
nonlocal flags_received
flags_received = flags
if mode[0] == 'x':
os.unlink(name)
return os.open(name, flags)

for mode in sorted(test_data):
with self.subTest(mode=mode):
with open(self.temp_file, mode, opener=test_open): pass
self.assertEqual(flags_received, test_data[mode])


def test_opener(self):
data = "test message\n"
with open(self.temp_file, "w", opener=os.open) as f:
Expand Down
Loading