Skip to content

bpo-34953: Implement mmap.mmap.__repr__ #9891

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.

Sign up for GitHub

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

Merged
zhangyangyu merged 17 commits into from
Oct 17, 2019
Merged

bpo-34953: Implement mmap.mmap.__repr__ #9891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
75856fb
bpo-34953: Implement `mmap.mmap.__repr__`
thautwarm Oct 13, 2018
718a6b0
done
thautwarm Oct 14, 2018
c6900ee
follow PEP7
thautwarm Oct 15, 2018
d9d8611
follow PEP7
thautwarm Oct 15, 2018
8f1546a
remove redundant line wraps and compat MS_WINDOWS
thautwarm Apr 19, 2019
9d87c5c
fix accessing data from m_obj
thautwarm Apr 19, 2019
9c77de3
add tests
thautwarm Apr 19, 2019
3226a35
compat windows and add exhaustive tests
thautwarm Apr 19, 2019
e4c2440
follow Zhang's guides to refine codes
thautwarm Apr 26, 2019
ff18f7d
fix tests
thautwarm Apr 26, 2019
3b1d544
avoid showing entire_contents according to core devs' points of view
thautwarm Apr 29, 2019
d5ffb47
test data with long int size & refine repr format args & remove redun…
thautwarm Apr 29, 2019
09b9554
invalid access now causes Py_UNREACHABLE
thautwarm Apr 29, 2019
a1b589f
remove 'fileno' due to code review and discussion & refine wording(cl…
thautwarm Apr 30, 2019
e5f3237
Merge branch 'master' into bpo-34953
zhangyangyu Oct 13, 2019
39d84a1
adjusts
zhangyangyu Oct 13, 2019
bf68fc7
replace PAGESIZE with ALLOCATIONGRANULARITY
zhangyangyu Oct 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions Lib/test/test_mmap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,42 @@ def test_flush_return_value(self):
# See bpo-34754 for details.
self.assertRaises(OSError, mm.flush, 1, len(b'python'))

def test_repr(self):
open_mmap_repr_pat = re.compile(
r"<mmap.mmap closed=False, "
r"access=(?P<access>\S+), "
r"length=(?P<length>\d+), "
r"pos=(?P<pos>\d+), "
r"offset=(?P<offset>\d+)>")
closed_mmap_repr_pat = re.compile(r"<mmap.mmap closed=True>")
mapsizes = (50, 100, 1_000, 1_000_000, 10_000_000)
offsets = tuple((mapsize // 2 // mmap.ALLOCATIONGRANULARITY)
* mmap.ALLOCATIONGRANULARITY for mapsize in mapsizes)
for offset, mapsize in zip(offsets, mapsizes):
data = b'a' * mapsize
length = mapsize - offset
accesses = ('ACCESS_DEFAULT', 'ACCESS_READ',
'ACCESS_COPY', 'ACCESS_WRITE')
positions = (0, length//10, length//5, length//4)
with open(TESTFN, "wb+") as fp:
fp.write(data)
fp.flush()
for access, pos in itertools.product(accesses, positions):
accint = getattr(mmap, access)
with mmap.mmap(fp.fileno(),
length,
access=accint,
offset=offset) as mm:
mm.seek(pos)
match = open_mmap_repr_pat.match(repr(mm))
self.assertIsNotNone(match)
self.assertEqual(match.group('access'), access)
self.assertEqual(match.group('length'), str(length))
self.assertEqual(match.group('pos'), str(pos))
self.assertEqual(match.group('offset'), str(offset))
match = closed_mmap_repr_pat.match(repr(mm))
self.assertIsNotNone(match)

@unittest.skipUnless(hasattr(mmap.mmap, 'madvise'), 'needs madvise')
def test_madvise(self):
size = 2 * PAGESIZE
Expand Down
69 changes: 57 additions & 12 deletions Modules/mmapmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,51 @@ mmap__exit__method(PyObject *self, PyObject *args)
return _PyObject_CallMethodIdNoArgs(self, &PyId_close);
}

static PyObject *
mmap__repr__method(PyObject *self)
{
mmap_object *mobj = (mmap_object *)self;

#ifdef MS_WINDOWS
#define _Py_FORMAT_OFFSET "lld"
if (mobj->map_handle == NULL)
#elif defined(UNIX)
# ifdef HAVE_LARGEFILE_SUPPORT
# define _Py_FORMAT_OFFSET "lld"
# else
# define _Py_FORMAT_OFFSET "ld"
# endif
if (mobj->data == NULL)
#endif
{
return PyUnicode_FromFormat("<%s closed=True>", Py_TYPE(self)->tp_name);
} else {
const char *access_str;

switch (mobj->access) {
case ACCESS_DEFAULT:
access_str = "ACCESS_DEFAULT";
break;
case ACCESS_READ:
access_str = "ACCESS_READ";
break;
case ACCESS_WRITE:
access_str = "ACCESS_WRITE";
break;
case ACCESS_COPY:
access_str = "ACCESS_COPY";
break;
default:
Py_UNREACHABLE();
}

@zhangyangyu zhangyangyu Apr 29, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, no difference between these. I'd suggest make closed a local variable and the logic could be clearer.

@thautwarm thautwarm Apr 29, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a difference, that when data is not available, getting offset could be insecure.

@zhangyangyu zhangyangyu Apr 29, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't get it. Why could it be insecure? After new_mmap_object they are all initialized right? What I am missing here? And if they are secure, I don't think we need to distingiush them between closed and open status, just show whatever they are. One more thing, why leave out the actual offset property and make pos the offset? It's confusing.

@thautwarm thautwarm Apr 29, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, I'd say sorry for offset should be pos.

At the first beginning I mean if mmap gets closed, the data of mmap_object has already been NULL. In this situation why should we allow uers to get a few outdated members like pos or size? I know people might feel like to ask a closed mmap for the historical records so I think you're right in that case, but in fact, the tell method and size method just raise exception PyErr_SetString(PyExc_ValueError, "mmap closed or invalid"):

So there's no way to get size and pos from a mmap object when it has been closed, why should we break this in just repr method? That's also what now motivates me to still distinguish closed status from the open one.

@thautwarm thautwarm Apr 29, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I know why I once used offset now..
It's the argument of mmap constructor: https://github.com/python/cpython/blob/master/Modules/mmapmodule.c#L1058

Also, length is used there instead of size.

I think we should do more to make it consistent in both low level(C data structures) and high level(interfaces for Python).
It might be better to change size to length and consist using offset via

The wording seems to be a thing, e.t.c, in previous codes there're already phrases like length of mmap, file size or memory offset:
https://github.com/python/cpython/blob/master/Modules/mmapmodule.c#L1133-L1145

@zhangyangyu zhangyangyu Apr 30, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Some properties are meaningless when closed, we don't need them, I mean pos. For others, I'd suggest just using the constructor arguments, length is length, pos is pos, offset is offset. Don't use size please since it's confusing, it's stored internally as a caculated field but unforunately there is also a size() method returns something different. As for fileno, I am okay to omit it for now. Actually no pos is also okay for me.


return PyUnicode_FromFormat("<%s closed=False, access=%s, length=%zd, "
"pos=%zd, offset=%" _Py_FORMAT_OFFSET ">",
Py_TYPE(self)->tp_name, access_str,
mobj->size, mobj->pos, mobj->offset);
}
}

#ifdef MS_WINDOWS
static PyObject *
mmap__sizeof__method(mmap_object *self, void *unused)
Expand Down Expand Up @@ -1044,23 +1089,23 @@ static PyTypeObject mmap_object_type = {
sizeof(mmap_object), /* tp_basicsize */
0, /* tp_itemsize */
/* methods */
(destructor) mmap_object_dealloc, /* tp_dealloc */
(destructor)mmap_object_dealloc, /* tp_dealloc */
0, /* tp_vectorcall_offset */
0, /* tp_getattr */
0, /* tp_setattr */
0, /* tp_as_async */
0, /* tp_repr */
(reprfunc)mmap__repr__method, /* tp_repr */
0, /* tp_as_number */
&mmap_as_sequence, /*tp_as_sequence*/
&mmap_as_mapping, /*tp_as_mapping*/
0, /*tp_hash*/
0, /*tp_call*/
0, /*tp_str*/
PyObject_GenericGetAttr, /*tp_getattro*/
0, /*tp_setattro*/
&mmap_as_buffer, /*tp_as_buffer*/
Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /*tp_flags*/
mmap_doc, /*tp_doc*/
&mmap_as_sequence, /* tp_as_sequence */
&mmap_as_mapping, /* tp_as_mapping */
0, /* tp_hash */
0, /* tp_call */
0, /* tp_str */
PyObject_GenericGetAttr, /* tp_getattro */
0, /* tp_setattro */
&mmap_as_buffer, /* tp_as_buffer */
Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /* tp_flags */
mmap_doc, /* tp_doc */
0, /* tp_traverse */
0, /* tp_clear */
0, /* tp_richcompare */
Expand Down