bpo-32557: allow shutil.disk_usage to take a file path on Windows also #9372
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
|
I've signed the CLA, but hadn't updated my b.p.o. account with my GitHub handle. That should be fixed now :) |
Modules/posixmodule.c
Outdated
| assert(path->length + 1 < MAX_PATH); | ||
| wcscpy_s(wpath, path->length + 1, path->wide); | ||
|
|
||
| if (win32_stat(wpath, &stat)) |
There was a problem hiding this comment.
win32_stat requires multiple system calls, so it's relatively expensive. I think only the uncommon case should be forced to pay this penalty, i.e. retry on the parent directory if GetDiskFreeSpaceExW fails with GetLastError() == ERROR_DIRECTORY. If you prefer to check first, call GetFileAttributesW instead to check for FILE_ATTRIBUTE_DIRECTORY, which is a relatively cheap call.
There was a problem hiding this comment.
Wow - thanks for taking a look so quickly, and thanks for the advice! I updated the diff to use a GetFileAttributesW check first, since it keeps the error checking logic simple. If you'd still rather I only do the fix if GetDisFreeSpaceExW fails, I'll be happy to update the diff again to do so.
…locate memory for the new path rather than allocating MAX_PATH characters on the stack
|
Ok - I've updated the PR to move the path truncation logic to the error case, and dynamically allocate space for the Path, per Steve Dower's request. |
Modules/posixmodule.c
Outdated
| return PyErr_NoMemory(); | ||
|
|
||
| wcscpy_s(wpath, dir_path.length + 1, path->wide); | ||
| _dirnameW(wpath); |
There was a problem hiding this comment.
_dirnameW is limited to MAX_PATH. Longer paths fail with -1 as the return code.
There was a problem hiding this comment.
Cool - I'll add the check, or do you mean I should not use _dirnameW, since in this case the path length can potentially exceed MAX_PATH characters? (I guess I'm a bit confused about Steve's comment, since parts of posixmodule.c still assume an upper bound of MAX_PATH, while others don't.)
Modules/posixmodule.c
Outdated
| _dirnameW(wpath); | ||
| dir_path.wide = wpath; | ||
|
|
||
| result = os__getdiskusage_impl(module, &dir_path); |
There was a problem hiding this comment.
I'd call GetDiskFreeSpaceExW inline on the parent directory and add a goto success statement. I dont see the need for a loop or recursion.
…os__getdiskusage_impl
There was a problem hiding this comment.
You're right that these changes need to be made throughout this module. It just hasn't been done yet (and until recently it wasn't an issue, since most of these functions didn't support >260 chars).
If you spot more, feel free to send a PR without an issue and @ mention python/windows-team for a review.
Modules/posixmodule.c
Outdated
|
|
||
| dir_path = PyMem_New(wchar_t, path->length + 1); | ||
| if (dir_path == NULL) | ||
| return PyErr_NoMemory(); |
There was a problem hiding this comment.
PEP 7 now requires braces even for single line blocks (which I'm sad about, but it's our style guide now)
There was a problem hiding this comment.
Cool - I was just following the convention I saw in the file, but I'm happy to add the braces.
Modules/posixmodule.c
Outdated
| PyMem_Free(dir_path); | ||
| if (retval) | ||
| goto success; | ||
| } | ||
| return PyErr_SetFromWindowsErr(0); |
There was a problem hiding this comment.
This is really obscure, but since we're here, PyMem_Free may overwrite the error code that is picked up by this function. We should capture GetLastError() immediately after the call and then pass it to this function.
There was a problem hiding this comment.
Got it - I'll make that change.
| @@ -343,8 +343,7 @@ Directory and files operations | |||
|
|
|||
| Return disk usage statistics about the given path as a :term:`named tuple` | |||
| with the attributes *total*, *used* and *free*, which are the amount of | |||
| total, used and free space, in bytes. On Windows, *path* must be a | |||
| directory; on Unix, it can be a file or directory. | |||
There was a problem hiding this comment.
Nit: I'd keep the "it can be a file or directory." part and add a versionchanged marker to explain that it now can accept file and directory on Windows. Example:
.. versionchanged:: 3.8
Explain the behaivor change here.There was a problem hiding this comment.
Ok - cool - I wasn't sure what the convention was. I'll update.
* Capture and propagate the last error before calling PyMem_Free. * Per PEP 7, wrap single-line conditional bodies in braces. * Revert the deoc string for disk_usage, and instead document my changes in a versionchanged block.
|
I fixed up the last two items (doc clarification and the NEWS file), so once the CI completes this can merge. Thanks, Joe! |
|
@jopamer: Status check is done, and it's a success ✅ . |
|
Thanks to you all! That was a lot of fun, and I really appreciate your guidance and support :) |
|
The new test is unstable, I wrote PR #11111 to fix the test. |
https://bugs.python.org/issue32557