Clean up path_converter in posixmodule.c

[serhiy-storchaka]

BPO	26671
Nosy	@vstinner, @larryhastings, @vadmium, @serhiy-storchaka
Files	path_converter_cleanup.patch path_converter_cleanup_2.patch path_converter_cleanup_3.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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-04-08.05:49:25.604>
created_at = <Date 2016-03-30.09:31:14.450>
labels = ['extension-modules', 'type-feature']
title = 'Clean up path_converter in posixmodule.c'
updated_at = <Date 2016-04-08.05:49:25.603>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2016-04-08.05:49:25.603>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-04-08.05:49:25.604>
closer = 'serhiy.storchaka'
components = ['Extension Modules']
creation = <Date 2016-03-30.09:31:14.450>
creator = 'serhiy.storchaka'
dependencies = []
files = ['42327', '42341', '42380']
hgrepos = []
issue_num = 26671
keywords = ['patch']
message_count = 13.0
messages = ['262657', '262695', '262734', '262950', '262953', '262954', '262955', '262956', '262959', '262961', '263005', '263007', '263008']
nosy_count = 5.0
nosy_names = ['vstinner', 'larry', 'python-dev', 'martin.panter', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue26671'
versions = ['Python 3.6']

[serhiy-storchaka]

path_converter in Modules/posixmodule.c sequentially tries to convert an argument to str, bytes, and int. If previous conversion is failed, it clears the error and tries with type. This can hide some errors (such as MemoryError) and even cause using unexpected conversion.

Proposed patch cleans up path_converter. In addition it avoids copying the content of instances of string subclass.

[larryhastings]

I approve in principle, but this patch isn't ready.

If we compile on Win32, and allow_fd is on, and they pass in an invalid fd, your patched code will reach line 914 "length = PyBytes_GET_SIZE(bytes);" but bytes will be uninitialized.

[serhiy-storchaka]

Good catch! Here is updated patch. It fixes also hiding exception in dir_fd converter.

[larryhastings]

Can you post the updated patch please?

[serhiy-storchaka]

Here it is.

[larryhastings]

LGTM.

[python-dev]

New changeset a866f5727b7f by Serhiy Storchaka in branch 'default':
Issue bpo-26671: Enhanced path_converter.
https://hg.python.org/cpython/rev/a866f5727b7f

[serhiy-storchaka]

Thank you for your review Larry.

[python-dev]

New changeset 8dc144e47252 by Serhiy Storchaka in branch 'default':
Issue bpo-26671: Fixed #ifdef indentation.
https://hg.python.org/cpython/rev/8dc144e47252

[python-dev]

New changeset 4acdb324a430 by Serhiy Storchaka in branch 'default':
Issue bpo-26671: Fixed #ifdef indentation.
https://hg.python.org/cpython/rev/4acdb324a430

[vadmium]

Looks like the tests may need updating for a changed exception message:

http://buildbot.python.org/all/builders/x86%20Ubuntu%20Shared%203.x/builds/12996/steps/test/logs/stdio
======================================================================
FAIL: test_stat (test.test_posix.PosixTester)
----------------------------------------------------------------------
TypeError: stat: path should be string, bytes or integer, not NoneType

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_posix.py", line 415, in test_stat
    posix.stat, None)
AssertionError: "can't specify None for path argument" does not match "stat: path should be string, bytes or integer, not NoneType"

======================================================================
FAIL: test_stat_dir_fd (test.test_posix.PosixTester)
----------------------------------------------------------------------
TypeError: argument should be integer or None, not str

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_posix.py", line 867, in test_stat_dir_fd
    posix.stat, support.TESTFN, dir_fd=posix.getcwd())
AssertionError: "should be integer, not" does not match "argument should be integer or None, not str"

[python-dev]

New changeset 633bb190fb76 by Serhiy Storchaka in branch 'default':
Issue bpo-26671: Fixed tests for changed error messages.
https://hg.python.org/cpython/rev/633bb190fb76

[serhiy-storchaka]

Thank you Martin.