Skip to content

Reduce syscalls for posixpath.ismount #117394

Closed

Description

@nineteendo

Feature or enhancement

Proposal:

Currently posixpath.ismount uses realpath(join(path, '..')) to get the parent of the mount. This is absolutely overkill and should only be used when join(path, '..') isn't sufficient.

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

Linked PRs

Activity

  1. barneygale commented on Apr 1, 2024

    @barneygale
    Contributor

    Related to #96328

  2. serhiy-storchaka commented on Apr 1, 2024

    @serhiy-storchaka
    Member

    abspath is broken by design.

    Let /mnt/storage/link be a link to /mnt, /mnt/storage is a mount point, and the original path is /mnt/storage/link/../mnt/storage. abspath() will return /mnt/storage/mnt/storage. dirname() will then return /mnt/storage/mnt which is on the same device as the original path, therefore ismount() will return False. But the real path is /mnt/storage, and it is a mount point.

  3. nineteendo commented on Apr 1, 2024

    @nineteendo
    ContributorAuthor

    Are you sure? ntpath.realpath() doesn't handle symlinks that way:

    >>> import ntpath
    >>> ntpath.realpath(r"C:\Users\wanne\path-picker\link")
    'C:\\Users\\wanne'
    >>> ntpath.realpath(r"C:\Users\wanne\path-picker\link\..")
    'C:\\Users\\wanne\\path-picker'

    Does that work the same on Linux?

  4. barneygale commented on Apr 1, 2024

    @barneygale
    Contributor

    It works differently on Linux - .. entries point to the real parent of a directory.

  5. added a commit that references this issue on Apr 2, 2024
  6. serhiy-storchaka commented on Apr 2, 2024

    @serhiy-storchaka
    Member

    #117447 is an alternate PR which achieves the same or better speed up.

  7. nineteendo commented on Apr 2, 2024

    @nineteendo
    ContributorAuthor

    Let /mnt/storage/link be a link to /mnt, /mnt/storage is a mount point, and the original path is /mnt/storage/link/../mnt/storage.

    Actually, you can work around this: ismount(realpath('/mnt/storage/link/../mnt/storage')).
    You couldn't remove realpath from the old implemenation, because that would raise a PermissionError.
    If documented, I feel like this is a cleaner solution, what do you think?

  8. serhiy-storchaka commented on Apr 2, 2024

    @serhiy-storchaka
    Member

    Currently this workaround is built in ismount() (see bpo-2466). Your solution replaces realpath() with abspath(), but the latter is not suitable for this. realpath() is relatively expensive, so my solution only calls it if it is needed.

  9. added a commit that references this issue on Apr 17, 2024
  10. added a commit that references this issue on Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    3.13only security fixesperformancePerformance or resource usagetype-featureA feature request or enhancement

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions