Skip to content
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

Add some posix functions #55021

Closed
rosslagerwall mannequin opened this issue Jan 3, 2011 · 26 comments
Closed

Add some posix functions #55021

rosslagerwall mannequin opened this issue Jan 3, 2011 · 26 comments
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@rosslagerwall
Copy link
Mannequin

rosslagerwall mannequin commented Jan 3, 2011

BPO 10812
Nosy @loewis, @birkenfeld, @gpshead, @ronaldoussoren, @abalkin, @pitrou, @giampaolo, @ned-deily
Files
  • mpos.patch: Patch + test for issue
  • 10812_v2.patch: Updated patch
  • 10812_v3.patch: Updated patch
  • 10812_v4.patch: Updated patch, added sethostid, gethostname
  • 10812_v5.patch: Updated patch, removed get/sethostname
  • 10812_v6.patch: Fixes for OS X
  • 10812_v7.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2011-03-17.18:24:48.122>
    created_at = <Date 2011-01-03.12:45:50.277>
    labels = ['extension-modules', 'type-feature']
    title = 'Add some posix functions'
    updated_at = <Date 2012-07-31.17:55:28.929>
    user = 'https://bugs.python.org/rosslagerwall'

    bugs.python.org fields:

    activity = <Date 2012-07-31.17:55:28.929>
    actor = 'ronaldoussoren'
    assignee = 'rosslagerwall'
    closed = True
    closed_date = <Date 2011-03-17.18:24:48.122>
    closer = 'rosslagerwall'
    components = ['Extension Modules']
    creation = <Date 2011-01-03.12:45:50.277>
    creator = 'rosslagerwall'
    dependencies = []
    files = ['20239', '20273', '20292', '20305', '20311', '20461', '21250']
    hgrepos = []
    issue_num = 10812
    keywords = ['patch']
    message_count = 26.0
    messages = ['125162', '125174', '125183', '125187', '125414', '125454', '125555', '125604', '125634', '125690', '125729', '125730', '125749', '126598', '131088', '131090', '131111', '131168', '131169', '131275', '131282', '131398', '167012', '167015', '167016', '167021']
    nosy_count = 11.0
    nosy_names = ['loewis', 'georg.brandl', 'gregory.p.smith', 'ronaldoussoren', 'belopolsky', 'pitrou', 'giampaolo.rodola', 'ned.deily', 'Arfrever', 'rosslagerwall', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue10812'
    versions = ['Python 3.3']

    @rosslagerwall
    Copy link
    Mannequin Author

    rosslagerwall mannequin commented Jan 3, 2011

    Here's a patch that adds a bunch of posix functions that are missing from the posix module. Includes tests & documentation.

    Tested on Linux & FreeBSD.

    Specifically:
    futimes
    lutimes
    futimens
    fexecve
    gethostid
    sethostname
    waitid
    lockf
    readv
    pread
    writev
    pwrite
    truncate
    posix_fallocate
    posix_fadvise
    sync

    @rosslagerwall rosslagerwall mannequin added extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Jan 3, 2011
    @pitrou
    Copy link
    Member

    pitrou commented Jan 3, 2011

    First couple comments:

    • you don't have to modify Misc/NEWS yourself; it will probably make patch maintenance easier
    • it would seem more natural for readv() to take a sequence of writable buffers (such as bytearrays) instead; I don't think the current signature is very useful
    • readv() and writev() should support both lists and tuples, at the minimum (perhaps arbitrary iterables if you like to spend more time on it :-)): see the PySequence* API

    @pitrou
    Copy link
    Member

    pitrou commented Jan 3, 2011

    For the record, I get the following failures under OpenSolaris:

    ======================================================================
    ERROR: test_lutimes (test.test_posix.PosixTester)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/home/antoine/py3k/cc/Lib/test/test_posix.py", line 265, in test_lutimes
        posix.lutimes(support.TESTFN, None)
    AttributeError: 'module' object has no attribute 'lutimes'

    ======================================================================
    ERROR: test_posix_fallocate (test.test_posix.PosixTester)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/home/antoine/py3k/cc/Lib/test/test_posix.py", line 236, in test_posix_fallocate
        posix.posix_fallocate(fd, 0, 10)
    OSError: [Errno 22] Invalid argument

    @pitrou
    Copy link
    Member

    pitrou commented Jan 3, 2011

    According to the posix_fallocate() man page under OpenSolaris:

     EINVAL    The len argument is less than or equal to zero, or
               the  offset  argument  is  less  than zero, or the
               underlying  file  system  does  not  support  this
               operation.
    

    I would go for the third (last) interpretation: the filesystem (ZFS here) doesn't support it.

    @rosslagerwall
    Copy link
    Mannequin Author

    rosslagerwall mannequin commented Jan 5, 2011

    This patch:

    Fixes test_lutimes(),
    Ignores a posix_fallocate() failure on Solaris due to ZFS,
    Changes readv() & writev() signatures such that they take sequences instead of tuples,
    Changes readv() so that it takes a number of writable buffers, fills them and returns the total number of bytes read.

    @abalkin
    Copy link
    Member

    abalkin commented Jan 5, 2011

    The patch contains a lot of repeated boilerplate code. I wonder if some of it can be factored out and reused. For example iov buffer allocation code appears to be identical in writev and readv.

    @rosslagerwall
    Copy link
    Mannequin Author

    rosslagerwall mannequin commented Jan 6, 2011

    This new patch reuses iov allocation code between readv & writev.
    It reuses code between exec, execve & fexecve.
    It reuses code for parsing off_t types.

    I've tried where possible to reuse code but I think though that the code seems pretty standard for the posix module. Each function pretty much parses args, calls posix function, checks for error and returns result.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jan 6, 2011

    Patch looks mostly good. Some comments:

    • sethostname supports names with embedded null bytes, so
      wrapper should not use strlen
    • it's a bit asymmetric that gethostname is in the socket
      module and supports Windows, and sethostname is in the POSIX
      module. It would be useful to have a gethostname in the POSIX
      module also which is a) POSIX only and b) supports embedded
      NUL bytes.
    • I think get/sethostname should use the FSDefault encoding.
    • gethostid should check for POSIX errors.
    • it checks for sethostid but then doesn't implement it.
    • parsing id_t with "l" is incorrect, methinks
    • siginfo_t should map to a structsequence
    • according to my copy of waitid(2), not all of the fields you
      are returning from siginfo_t are actually filled.
    • instead of PARSE_OFF_T, I rather recommend to use an O& parser.

    @rosslagerwall
    Copy link
    Mannequin Author

    rosslagerwall mannequin commented Jan 7, 2011

    it's a bit asymmetric that gethostname is in the socket
    module and supports Windows, and sethostname is in the POSIX
    module. It would be useful to have a gethostname in the POSIX
    module also which is a) POSIX only and b) supports embedded
    NUL bytes.

    According to the spec for gethostname(), the hostname that it returns is null-terminated so it won't support embedded NUL bytes. Should we still add it anyway?

    @rosslagerwall
    Copy link
    Mannequin Author

    rosslagerwall mannequin commented Jan 7, 2011

    Thanks for the comments.

    I implemented sethostid() - its not actually in the posix spec, but linux/*bsd
    have it although the signature for FreeBSD is a bit different.

    I implemented gethostname(). Both get/sethostname() now use FSDefault encoding.
    gethostname() is null-terminated so it doesn't have to deal with embedded NULs.
    sethostname() works with embedded NULs.

    Since id_t can contain a pid_t, I now parse id_t with _Py_PARSE_PID.
    waitid() also now returns a structsequence of the correct fields.

    According to the spec, gethostid does not set errno - it now checks anyway.

    Instead of PARSE_OFF_T, I use a O& parser.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jan 7, 2011

    According to the spec for gethostname(), the hostname that it returns
    is null-terminated so it won't support embedded NUL bytes. Should we
    still add it anyway?

    Oops, I misread the spec. No, gethostname should then not be duplicated.
    Putting sethostname into the socket module might be worth considering,
    though; I find the proposed split confusing.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jan 7, 2011

    According to the spec, gethostid does not set errno - it now checks anyway.

    Sorry, I misread that also. Leaving the check is fine; reverting it to
    the previous code would be fine as well.

    @rosslagerwall
    Copy link
    Mannequin Author

    rosslagerwall mannequin commented Jan 8, 2011

    This patch takes out sethostname() and gethostname(). I'll open up a new issue to add sethostname() to the socket module.

    @rosslagerwall
    Copy link
    Mannequin Author

    rosslagerwall mannequin commented Jan 20, 2011

    A few small fixes for OS X:
    It has no return value for sethostid() and sets different errno if permission denied,
    waitid() is broken - so its disabled,
    the timeval struct used in futimes and lutimes is defined slightly differently.

    @gpshead
    Copy link
    Member

    gpshead commented Mar 16, 2011

    I added some comments on the review for 10812_v5.patch. not sure why v6 doesn't have a review link. Overall, very nice after addressing the few comments I had.

    btw, can you sync this up with the hg tip (3.3) now while addressing the above?

    I'm excited to see readv/writev/fadvise added. Extremely useful.

    @rosslagerwall
    Copy link
    Mannequin Author

    rosslagerwall mannequin commented Mar 16, 2011

    PyParse_off_t was already added when sendfile() was added as was the iovec_setup stuff.

    I'll upload a patch soon which updates it to take this and the other comments into account.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Mar 16, 2011

    v6 was created against an unknown mercurial repository (most likely code.python.org). It actually doesn't apply cleanly anymore so it would be best to regenerate it it (or go on with any v7 that may result out of the review).

    @rosslagerwall
    Copy link
    Mannequin Author

    rosslagerwall mannequin commented Mar 16, 2011

    This new patch, updated against the tip for 3.3:

    Shares the iov_setup and iov_cleanup code, py_parse_off_t with sendfile().

    Removes gethostid and sethostid since they're deprecated.

    I think I was correct with referring to utime() since I was referring to the python function, os.utime().

    @rosslagerwall
    Copy link
    Mannequin Author

    rosslagerwall mannequin commented Mar 16, 2011

    Forgot to attach the patch

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 17, 2011

    New changeset 525320df8afb by Ross Lagerwall in branch 'default':
    Issue bpo-10812: Add some extra posix functions to the os module.
    http://hg.python.org/cpython/rev/525320df8afb

    @rosslagerwall rosslagerwall mannequin closed this as completed Mar 17, 2011
    @rosslagerwall rosslagerwall mannequin self-assigned this Mar 17, 2011
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 17, 2011

    New changeset 8945e087a5a6 by Ross Lagerwall in branch 'default':
    Issue bpo-10812: Revert os.lseek change.
    http://hg.python.org/cpython/rev/8945e087a5a6

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 19, 2011

    New changeset 96e09d039433 by Ross Lagerwall in branch 'default':
    Fix refleak introduced by bpo-10812.
    http://hg.python.org/cpython/rev/96e09d039433

    @ronaldoussoren
    Copy link
    Contributor

    msg126598 says that waitid is broken on MacOSX. In what way it is broken?

    The code is currently disabled for OSX without any indication why this is so, that makes it hard to know how to test if new OSX releases (like the just released OSX 10.8) fix waitid.

    @ronaldoussoren
    Copy link
    Contributor

    To elaborate: when I remove the "&& !defined(APPLE)" tests from posixmodule.c test_posix passes on OSX 10.8, including test_waitid.

    I have no idea if this means that waitid is no longer "broken" in OSX 10.8 or that test_posix doesn't trigger the issue.

    @rosslagerwall
    Copy link
    Mannequin Author

    rosslagerwall mannequin commented Jul 31, 2012

    I can't actually remember why I disabled waitid for OS X - that was message was rather a long time ago :-(

    Unfortunately, I don't currently have access to an OS X machine to test it.

    A google search shows the following comment in the v8 javascript engine:
    // We're disabling usage of waitid in Mac OS X because it doens't work for us:
    // a parent process hangs on waiting while a child process is already a zombie.
    // See http://code.google.com/p/v8/issues/detail?id=401.

    Do you have access to test an older release like Leopard?

    @ronaldoussoren
    Copy link
    Contributor

    I have VMs with earlier versions of OSX (at least 10.5 and 10.6). I cannot run those right now because they use enough resources to interfere with other activities. I'll try to test later this week.

    BTW. I'd prefer to make waitid available on OSX unless it is totally non-functional. I'm not convinced that the Google problem is a good enough reason to avoid exposing the function. If it is disabled there should be a clear description of why that is, preferable in the form of a (configure) test.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants