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.
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
gh-93180: Update documentation of os.copy_file_range
#93182
Conversation
This code is responsible for the documented features in the kernel: https://github.com/torvalds/linux/blob/1e57930e9f4083ad5854ab6eadffe790a8167fb4/fs/read_write.c#L1491-L1507
Doc/library/os.rst
Outdated
|
||
This function should not be used for copying a file from special | ||
filesystems like procfs and sysfs because of | ||
`a known issue <https://lore.kernel.org/linux-fsdevel/20210126135012.1.If45b7cdc3ff707bc1efa17f5366057d60603c45f@changeid/>`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link contains a fix. Was the fix merged into Linux?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it wasn't. I tested os.copy_file_range
for copying /proc/cpuinfo
on Linux 5.17, it returned 0 and the destination file was empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, some Python functions that operate files are prone to similar issues too :(. I'll create a few bug reports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not add a link to an email.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about such a note?
:func:
os.copy_file_range
should not be used for copying a range of a pseudo file from a special filesystem like procfs and sysfs. It will always copy no bytes and return 0 as if the file was empty because of a known Linux kernel issue.
Doc/library/os.rst
Outdated
some filesystems could implement extra optimizations, such as the use of | ||
reflinks (i.e., two or more inodes that share pointers to the same | ||
copy-on-write disk blocks; supported file systems include btrfs and XFS) | ||
and server-side copy (in the case of NFS). The copy is done as if | ||
both files are opened as binary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function arguments are file descriptors. What does "binary" mean for file descriptors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence is not new. I guess it means that bytes written to a destination file will be equal to ones read from the source file.
In contrary, if a file range is read in the text mode in Python and it is written in the text mode to a different file, bytes may differ. For example, line endings may be changed. See https://docs.python.org/3/tutorial/inputoutput.html#reading-and-writing-files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind to rephase the sentence to explain that? Explain that the copy does not change the line endings, for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 069651d.
BTW, shutil.copyfile
looks to behave the same, but shutil.copyfileobj
seems to respect the text mode.
Doc/library/os.rst
Outdated
|
||
This function should not be used for copying a file from special | ||
filesystems like procfs and sysfs because of | ||
`a known issue <https://lore.kernel.org/linux-fsdevel/20210126135012.1.If45b7cdc3ff707bc1efa17f5366057d60603c45f@changeid/>`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not add a link to an email.
Misc/NEWS.d/next/Documentation/2022-05-24-21-30-57.gh-issue-93180.V4yMpc.rst
Outdated
Show resolved
Hide resolved
Merged, thanks! |
Resolves #93180