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

os.lstat/os.stat don't set st_nlink on Windows #54236

Closed
briancurtin opened this issue Oct 5, 2010 · 19 comments
Closed

os.lstat/os.stat don't set st_nlink on Windows #54236

briancurtin opened this issue Oct 5, 2010 · 19 comments
Assignees
Labels
extension-modules C modules in the Modules dir OS-windows type-bug An unexpected behavior, bug, or error

Comments

@briancurtin
Copy link
Member

BPO 10027
Nosy @jaraco, @briancurtin
Files
  • st_nlink.diff
  • py3k_posixpath_v1.patch: replaced
  • py3k_posixpath_traverse_via_DeviceIoControl.patch: Same patch to r85789
  • 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 = 'https://github.com/briancurtin'
    closed_at = <Date 2011-06-04.03:00:56.953>
    created_at = <Date 2010-10-05.14:35:25.707>
    labels = ['extension-modules', 'type-bug', 'OS-windows']
    title = "os.lstat/os.stat don't set st_nlink on Windows"
    updated_at = <Date 2011-06-04.03:00:56.952>
    user = 'https://github.com/briancurtin'

    bugs.python.org fields:

    activity = <Date 2011-06-04.03:00:56.952>
    actor = 'brian.curtin'
    assignee = 'brian.curtin'
    closed = True
    closed_date = <Date 2011-06-04.03:00:56.953>
    closer = 'brian.curtin'
    components = ['Extension Modules', 'Windows']
    creation = <Date 2010-10-05.14:35:25.707>
    creator = 'brian.curtin'
    dependencies = []
    files = ['19134', '19290', '19335']
    hgrepos = []
    issue_num = 10027
    keywords = ['patch', 'needs review']
    message_count = 19.0
    messages = ['118012', '118029', '118064', '118066', '118720', '118724', '119108', '119110', '119111', '119130', '119182', '119380', '120521', '120675', '121667', '121977', '122275', '122324', '137624']
    nosy_count = 3.0
    nosy_names = ['jaraco', 'ocean-city', 'brian.curtin']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue10027'
    versions = ['Python 3.2']

    @briancurtin
    Copy link
    Member Author

    In msg117834 on bpo-8879 it was noticed that os.lstat and os.stat don't set st_nlink on Windows, which is causing the patch on that issue to fail test_tarfile.

    Attached is a stripped down version of the patch Hirokazu Yamamoto proposed on bpo-8879, containing just the os.*stat changes. As stated in that message, the patch is a quick hack to get test_os and test_tarfile working, so it could use review.

    @briancurtin briancurtin added extension-modules C modules in the Modules dir OS-windows type-bug An unexpected behavior, bug, or error labels Oct 5, 2010
    @briancurtin
    Copy link
    Member Author

    Overall I think this looks like a reasonable restructuring, and it works in a few manual tests of existing hardlinks on my system. Until bpo-8879 goes in, we can't really add tests for this.

    Hirokazu, do you want to commit this since you came up with it?

    @ocean-city
    Copy link
    Mannequin

    ocean-city mannequin commented Oct 6, 2010

    There are some questions.

    1. About my patch, I noticed it removed following code.
      Isn't this needed? I like clean code, but I don't want to
      break anything.
        /* Get WIN32_FIND_DATA structure for the path to determine if
           it is a symlink */
        if(info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) {
            find_data_handle = FindFirstFileA(path, &find_data);
            if(find_data_handle != INVALID_HANDLE_VALUE) {
                if(find_data.dwReserved0 == IO_REPARSE_TAG_SYMLINK) {
                    /* first clear the S_IFMT bits */
                    result->st_mode ^= (result->st_mode & 0170000);
                    /* now set the bits that make this a symlink */
                    result->st_mode |= 0120000;
                }
                FindClose(find_data_handle);
            }
        }
    
    
        /* Set S_IFEXEC if it is an .exe, .bat, ... */
        dot = strrchr(path, '.');
        if (dot) {
            if (stricmp(dot, ".bat") == 0 || stricmp(dot, ".cmd") == 0 ||
                stricmp(dot, ".exe") == 0 || stricmp(dot, ".com") == 0)
                result->st_mode |= 0111;
        }
    1. About current behavior. when os.stat() is used for junction
      (reparse point, but not simlink), returned information is
      about junction on XP or earlier, but about target folder on
      Vista or above. Is this intended bahavior?

    Thank you.

    @ocean-city
    Copy link
    Mannequin

    ocean-city mannequin commented Oct 6, 2010

    P.S. Thank you for acceptance. I really wanted to commit
    that code!

    @briancurtin
    Copy link
    Member Author

    Jason, any idea on #2?

    @jaraco
    Copy link
    Member

    jaraco commented Oct 14, 2010

    I'm unsure. I think when I addressed the issue, I was only concerned with symlinks. The Vista behavior sounds correct to me, so it may be that XP was left with the legacy behavior as it doesn't have native symlink support. It sounds as if the behavior on XP should be modified to follow the same technique as in Vista. I don't know when I'll have a moment to look at it in depth, but I'll try to in the next week or two.

    @ocean-city
    Copy link
    Mannequin

    ocean-city mannequin commented Oct 19, 2010

    How about this patch?

    • st_nlink support for stat()/lstat().
    • lstat() for junction read attributes of junction not target.
    • stat() for symlink of system locked file (ie: c:/pagefile.sys)
      should read attributes from target locked file not of symlink
      even via FindFirstFile. (I cannot test this, sorry)

    I hope other behavior was not changed by this patch.

    @ocean-city
    Copy link
    Mannequin

    ocean-city mannequin commented Oct 19, 2010

    ... I noticed I can test this via buildbot...
    Probably I'll try this.

    @briancurtin
    Copy link
    Member Author

    I'll also give it a run on my Windows machines but won't be able to until tomorrow morning (~1300 UTC).

    @ocean-city
    Copy link
    Mannequin

    ocean-city mannequin commented Oct 19, 2010

    I noticed stat() won't set S_IEXEC in stat()
    with my patch (py3k_posixpath_v1.patch). :-(

    @ocean-city
    Copy link
    Mannequin

    ocean-city mannequin commented Oct 20, 2010

    I replaced my patch (almost reverted stat() part)
    I think st_nlink will be set via stat()/lstat(), at least.

    @ocean-city
    Copy link
    Mannequin

    ocean-city mannequin commented Oct 22, 2010

    I created test branch "branches/py3k-stat-on-windows" and
    committed the new patch in r85789. This achieves msg119108.
    I tested this on Windows7 buildbot where symlink support
    exists, it seems working correct.
    http://www.python.org/dev/buildbot/all/builders/x86%20Windows7%203.x/builds/1840

    @briancurtin
    Copy link
    Member Author

    Works for me. I think it should be ok to commit.

    @ocean-city
    Copy link
    Mannequin

    ocean-city mannequin commented Nov 7, 2010

    I found Win32 FileID API. With this library, it seems
    we can use GetFileInformationByHandleEx before Vista.
    We can get real file name and hard link count from handle.

    http://www.microsoft.com/downloads/en/details.aspx?FamilyID=1DECC547-AB00-4963-A360-E4130EC079B8&amp;displaylang=en

    I don't have a patch, but maybe it is worth to implement it.

    @briancurtin
    Copy link
    Member Author

    I'm not sure how that would work in terms of redistributing, and how we'd handle it within our own build process. This close to the beta I'm -1 on adding that API.

    @ocean-city
    Copy link
    Mannequin

    ocean-city mannequin commented Nov 21, 2010

    Thank you for reply. Could you commit my last patch
    instead of me? I cannot commit files for a while.

    @briancurtin
    Copy link
    Member Author

    Committed to py3k in r86727.

    I think this should be backported to the maintenance branches, but not until after the upcoming point releases. Although those branches won't have the ability to create hard links, they should have the ability to view information about existing links on the system (or if they create them via ctypes/pywin32). Since those branches won't be able to explicitly test this, I think it makes sense to let this bake for a while on py3k.

    @ocean-city
    Copy link
    Mannequin

    ocean-city mannequin commented Nov 25, 2010

    Thank you for commit!

    @briancurtin
    Copy link
    Member Author

    This code has changed a lot since originally being committed, so I'll handle backporting in bpo-12084 which has the latest changes.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    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 OS-windows type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants