[issue12743] C API marshalling doc contains XXX
New submission from JJeffries <jamesjeffries1@gmail.com>: The Python C API manual page for data marshaling contains the following paragraph. XXX What about error detection? It appears that reading past the end of the file will always result in a negative numeric value (where that’s relevant), but it’s not clear that negative values won’t be handled properly when there’s no error. What’s the right way to tell? Should only non-negative values be written using these routines? I suggest that the XXX should be removed as it is unclear why it's there. Patch to follow in the next couple of days if others agree. ---------- assignee: docs@python components: Documentation messages: 141959 nosy: JJeffries, docs@python priority: normal severity: normal status: open title: C API marshalling doc contains XXX _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue12743> _______________________________________
Éric Araujo <merwok@netwok.org> added the comment: It should be removed if someone is confident that it’s a obsolete comment, or if tests get added to answer the questions in the note. ---------- nosy: +eric.araujo _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue12743> _______________________________________
Martin v. Löwis <martin@v.loewis.de> added the comment: Would you just remove the "XXX" string, or the entire comment? "XXX" is typically used to indicate that something needs to be done, and the comment makes a clear statement as to what it is that needs to be done. ---------- nosy: +loewis _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue12743> _______________________________________
Ronald Oussoren added the comment:
From reading the source of Python/marshal.c it seems that the read function's raise an exception on I/O errors, but don't return a specific value (that is, sentence starting with "It appears that" is wrong).
PyMarshal_ReadLongFromFile calls r_long, this calls r_string without checking for errors and calculates the return value from the buffer passed to r_string. On I/O errors the buffer may not have been filled at all and contains random data (whatever happened to be on the stack). Likewise for PyMarhal_ReadShortFromFile (through r_short instead of r_long). r_string does raise an exception on I/O errors or short reads, but reading from FILE* and Python objects. The most straightforward documentation update would be: * Remove the entire XXX paragraph * Add text to the documentation for PyMarshal_ReadLongFromFile and PyMarshal_ReadShortFromFile: On error sets the appopriate exception (:exc:`EOFError`), but does not return a specific value. Use :func:`PyErr_Occurred` to check for errors. ---------- nosy: +ronaldoussoren _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue12743> _______________________________________
Changes by Serhiy Storchaka <storchaka@gmail.com>: ---------- stage: -> needs patch type: -> behavior _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue12743> _______________________________________
Berker Peksag added the comment: Since 4059e871e74e, PyMarshal_ReadLongFromFile and PyMarshal_ReadShortFromFile can return -1 on error. Return values of those functions were already documented in acb4d43955f6. Some of the usages also check return value of PyErr_Occurred(): https://hg.python.org/cpython/rev/11958c69a4b2#l2.7 I removed the outdated paragraph and add a sentence about using PyErr_Occurred(). ---------- keywords: +patch nosy: +berker.peksag, haypo stage: needs patch -> patch review versions: +Python 3.5, Python 3.6 Added file: http://bugs.python.org/file42555/issue12743.diff _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue12743> _______________________________________
Changes by Berker Peksag <berker.peksag@gmail.com>: ---------- versions: +Python 3.7 Added file: http://bugs.python.org/file44871/issue12743_v2.diff _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue12743> _______________________________________
Change by Berker Peksag <berker.peksag@gmail.com>: ---------- pull_requests: +7981 _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
Berker Peksag <berker.peksag@gmail.com> added the comment: New changeset defcffdf86780e3a184ebb25dc9a7b807753d57a by Berker Peksag in branch 'master': bpo-12743: Delete comment from marshal.rst (GH-8457) https://github.com/python/cpython/commit/defcffdf86780e3a184ebb25dc9a7b80775... ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
Change by miss-islington <mariatta.wijaya+miss-islington@gmail.com>: ---------- pull_requests: +8010 _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
Change by miss-islington <mariatta.wijaya+miss-islington@gmail.com>: ---------- pull_requests: +8011 _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
miss-islington <mariatta.wijaya+miss-islington@gmail.com> added the comment: New changeset 21ed29ac290b10d31dcac947f9246ae4d8b94a86 by Miss Islington (bot) in branch '3.7': bpo-12743: Delete comment from marshal.rst (GH-8457) https://github.com/python/cpython/commit/21ed29ac290b10d31dcac947f9246ae4d8b... ---------- nosy: +miss-islington _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
miss-islington <mariatta.wijaya+miss-islington@gmail.com> added the comment: New changeset 146ba436cc0457b8ef7fea8b054b9ccb15e24748 by Miss Islington (bot) in branch '3.6': bpo-12743: Delete comment from marshal.rst (GH-8457) https://github.com/python/cpython/commit/146ba436cc0457b8ef7fea8b054b9ccb15e... ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
Change by Berker Peksag <berker.peksag@gmail.com>: ---------- resolution: -> fixed stage: patch review -> resolved status: open -> closed versions: +Python 3.8 -Python 3.5 _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
Éric Araujo <merwok@netwok.org> added the comment: It should be removed if someone is confident that it’s a obsolete comment, or if tests get added to answer the questions in the note. ---------- nosy: +eric.araujo _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue12743> _______________________________________
Martin v. Löwis <martin@v.loewis.de> added the comment: Would you just remove the "XXX" string, or the entire comment? "XXX" is typically used to indicate that something needs to be done, and the comment makes a clear statement as to what it is that needs to be done. ---------- nosy: +loewis _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue12743> _______________________________________
Ronald Oussoren added the comment:
From reading the source of Python/marshal.c it seems that the read function's raise an exception on I/O errors, but don't return a specific value (that is, sentence starting with "It appears that" is wrong).
PyMarshal_ReadLongFromFile calls r_long, this calls r_string without checking for errors and calculates the return value from the buffer passed to r_string. On I/O errors the buffer may not have been filled at all and contains random data (whatever happened to be on the stack). Likewise for PyMarhal_ReadShortFromFile (through r_short instead of r_long). r_string does raise an exception on I/O errors or short reads, but reading from FILE* and Python objects. The most straightforward documentation update would be: * Remove the entire XXX paragraph * Add text to the documentation for PyMarshal_ReadLongFromFile and PyMarshal_ReadShortFromFile: On error sets the appopriate exception (:exc:`EOFError`), but does not return a specific value. Use :func:`PyErr_Occurred` to check for errors. ---------- nosy: +ronaldoussoren _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue12743> _______________________________________
Changes by Serhiy Storchaka <storchaka@gmail.com>: ---------- stage: -> needs patch type: -> behavior _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue12743> _______________________________________
Berker Peksag added the comment: Since 4059e871e74e, PyMarshal_ReadLongFromFile and PyMarshal_ReadShortFromFile can return -1 on error. Return values of those functions were already documented in acb4d43955f6. Some of the usages also check return value of PyErr_Occurred(): https://hg.python.org/cpython/rev/11958c69a4b2#l2.7 I removed the outdated paragraph and add a sentence about using PyErr_Occurred(). ---------- keywords: +patch nosy: +berker.peksag, haypo stage: needs patch -> patch review versions: +Python 3.5, Python 3.6 Added file: http://bugs.python.org/file42555/issue12743.diff _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue12743> _______________________________________
Changes by Berker Peksag <berker.peksag@gmail.com>: ---------- versions: +Python 3.7 Added file: http://bugs.python.org/file44871/issue12743_v2.diff _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue12743> _______________________________________
Change by Berker Peksag <berker.peksag@gmail.com>: ---------- pull_requests: +7981 _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
Berker Peksag <berker.peksag@gmail.com> added the comment: New changeset defcffdf86780e3a184ebb25dc9a7b807753d57a by Berker Peksag in branch 'master': bpo-12743: Delete comment from marshal.rst (GH-8457) https://github.com/python/cpython/commit/defcffdf86780e3a184ebb25dc9a7b80775... ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
Change by miss-islington <mariatta.wijaya+miss-islington@gmail.com>: ---------- pull_requests: +8010 _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
Change by miss-islington <mariatta.wijaya+miss-islington@gmail.com>: ---------- pull_requests: +8011 _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
miss-islington <mariatta.wijaya+miss-islington@gmail.com> added the comment: New changeset 21ed29ac290b10d31dcac947f9246ae4d8b94a86 by Miss Islington (bot) in branch '3.7': bpo-12743: Delete comment from marshal.rst (GH-8457) https://github.com/python/cpython/commit/21ed29ac290b10d31dcac947f9246ae4d8b... ---------- nosy: +miss-islington _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
miss-islington <mariatta.wijaya+miss-islington@gmail.com> added the comment: New changeset 146ba436cc0457b8ef7fea8b054b9ccb15e24748 by Miss Islington (bot) in branch '3.6': bpo-12743: Delete comment from marshal.rst (GH-8457) https://github.com/python/cpython/commit/146ba436cc0457b8ef7fea8b054b9ccb15e... ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
Change by Berker Peksag <berker.peksag@gmail.com>: ---------- resolution: -> fixed stage: patch review -> resolved status: open -> closed versions: +Python 3.8 -Python 3.5 _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue12743> _______________________________________
participants (7)
-
Berker Peksag -
JJeffries
-
Martin v. Löwis
-
miss-islington
-
Ronald Oussoren
-
Serhiy Storchaka
-
Éric Araujo