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

Fix GreenFileIO.readall() for regular file #257

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@vstinner
Contributor

vstinner commented Oct 19, 2015

GreenFileIO.readall() calls _trampoline(self, read=True). Problem: on
regular file, epoll_ctl() fails with EPERM on Linux, because regular
files are not supported.

This change skips the call to _trampoline() on regular files. Add
also various unit tests on the various ways to read the whole content
of a file.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jan 4, 2016

Contributor

ping?

Contributor

vstinner commented Jan 4, 2016

ping?

@jstasiak

This comment has been minimized.

Show comment
Hide comment
@jstasiak

jstasiak Jan 7, 2016

Contributor

Hey @haypo, do you mind moving the code in regular_file_readall.py into a section guarded by if __name__ == '__main__' so that it's import-safe?

Contributor

jstasiak commented Jan 7, 2016

Hey @haypo, do you mind moving the code in regular_file_readall.py into a section guarded by if __name__ == '__main__' so that it's import-safe?

Fix GreenFileIO.readall() for regular file
GreenFileIO.readall() calls _trampoline(self, read=True). Problem: on
regular file, epoll_ctl() fails with EPERM on Linux, because regular
files are not supported.

This change skips the call to _trampoline() on regular files. Add
also various unit tests on the various ways to read the whole content
of a file.
@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jan 7, 2016

Contributor

Hey @haypo, do you mind moving the code in regular_file_readall.py into a section guarded by if name == 'main' so that it's import-safe?

Sure, it's done.

Contributor

vstinner commented Jan 7, 2016

Hey @haypo, do you mind moving the code in regular_file_readall.py into a section guarded by if name == 'main' so that it's import-safe?

Sure, it's done.

@@ -98,7 +101,8 @@ def readall(self):
except OSError as e:
if get_errno(e) not in SOCKET_BLOCKING:
raise IOError(*e.args)
self._trampoline(self, read=True)
if not self._is_regular:

This comment has been minimized.

@temoto

temoto Jun 2, 2016

Member

According to stat() documentation, file descriptor may be a regular file, directory, symlink, block or char device, socket and fifo. What are supported types? Is it possible to use white list instead? Check that fd is of supported type.

@temoto

temoto Jun 2, 2016

Member

According to stat() documentation, file descriptor may be a regular file, directory, symlink, block or char device, socket and fifo. What are supported types? Is it possible to use white list instead? Check that fd is of supported type.

This comment has been minimized.

@davidszotten

davidszotten Jun 2, 2016

Contributor

hm. looking at this again actually makes me wonder if #314 is a better fix. the problem is triggered when we call epoll.register on a regular file, but we should only be doing that when we catch something in SOCKET_BLOCKING. if i understand things correctly, this should never be raised from regular files.

@davidszotten

davidszotten Jun 2, 2016

Contributor

hm. looking at this again actually makes me wonder if #314 is a better fix. the problem is triggered when we call epoll.register on a regular file, but we should only be doing that when we catch something in SOCKET_BLOCKING. if i understand things correctly, this should never be raised from regular files.

@davidszotten

This comment has been minimized.

Show comment
Hide comment
@davidszotten

davidszotten Jul 1, 2016

Contributor

this should be fixed by #314

Contributor

davidszotten commented Jul 1, 2016

this should be fixed by #314

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Dec 5, 2016

Contributor

Sorry, I'm not interested to update this old pull request anymore, so I just close it.

Contributor

vstinner commented Dec 5, 2016

Sorry, I'm not interested to update this old pull request anymore, so I just close it.

@vstinner vstinner closed this Dec 5, 2016

@davidszotten

This comment has been minimized.

Show comment
Hide comment
@davidszotten

davidszotten Dec 5, 2016

Contributor

problem is already fixed by #314 i think

Contributor

davidszotten commented Dec 5, 2016

problem is already fixed by #314 i think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment