classification
Title: C API marshalling doc contains XXX
Type: behavior Stage: resolved
Components: Documentation Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: JJeffries, berker.peksag, docs@python, eric.araujo, loewis, miss-islington, ronaldoussoren, vstinner
Priority: normal Keywords: patch

Created on 2011-08-12 14:43 by JJeffries, last changed 2018-07-27 04:47 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
issue12743.diff berker.peksag, 2016-04-21 13:49 review
issue12743_v2.diff berker.peksag, 2016-09-28 16:29 review
Pull Requests
URL Status Linked Edit
PR 8457 merged berker.peksag, 2018-07-25 08:43
PR 8489 merged miss-islington, 2018-07-27 04:35
PR 8490 merged miss-islington, 2018-07-27 04:36
Messages (8)
msg141959 - (view) Author: JJeffries (JJeffries) Date: 2011-08-12 14:43
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.
msg141974 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-08-12 17:31
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.
msg142055 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2011-08-14 07:20
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.
msg191116 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2013-06-14 11:08
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.
msg263919 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-04-21 13:49
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().
msg322458 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-07-27 04:35
New changeset defcffdf86780e3a184ebb25dc9a7b807753d57a by Berker Peksag in branch 'master':
bpo-12743: Delete comment from marshal.rst (GH-8457)
https://github.com/python/cpython/commit/defcffdf86780e3a184ebb25dc9a7b807753d57a
msg322459 - (view) Author: miss-islington (miss-islington) Date: 2018-07-27 04:40
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/21ed29ac290b10d31dcac947f9246ae4d8b94a86
msg322460 - (view) Author: miss-islington (miss-islington) Date: 2018-07-27 04:42
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/146ba436cc0457b8ef7fea8b054b9ccb15e24748
History
Date User Action Args
2018-07-27 04:47:22berker.peksagsetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: + Python 3.8, - Python 3.5
2018-07-27 04:42:47miss-islingtonsetmessages: + msg322460
2018-07-27 04:40:40miss-islingtonsetnosy: + miss-islington
messages: + msg322459
2018-07-27 04:36:21miss-islingtonsetpull_requests: + pull_request8011
2018-07-27 04:35:23miss-islingtonsetpull_requests: + pull_request8010
2018-07-27 04:35:19berker.peksagsetmessages: + msg322458
2018-07-25 08:43:04berker.peksagsetpull_requests: + pull_request7981
2016-09-28 16:29:27berker.peksagsetfiles: + issue12743_v2.diff
versions: + Python 3.7
2016-04-21 13:49:25berker.peksagsetfiles: + issue12743.diff

versions: + Python 3.5, Python 3.6
keywords: + patch
nosy: + vstinner, berker.peksag

messages: + msg263919
stage: needs patch -> patch review
2015-01-29 16:42:09serhiy.storchakasettype: behavior
stage: needs patch
2013-06-14 11:08:34ronaldoussorensetnosy: + ronaldoussoren
messages: + msg191116
2011-08-14 07:20:36loewissetnosy: + loewis
messages: + msg142055
2011-08-12 17:31:33eric.araujosetnosy: + eric.araujo
messages: + msg141974
2011-08-12 14:43:03JJeffriescreate