Author nascheme
Recipients brett.cannon, davin, eric.snow, giampaolo.rodola, lukasz.langa, nascheme, osvenskan, pitrou, pmpp, rhettinger, ronaldoussoren, skrah, terry.reedy, yselivanov
Date 2019-02-07.17:52:30
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1549561951.36.0.982090985835.issue35813@roundup.psfhosted.org>
In-reply-to
Content
I didn't finish reviewing completely yet but here are some comments.

I think the random filename generation can be pulled out of
_posixshmem.  Make it require that a filename is passed into it.
That moves a fair bit of complexity out of C and into Python.  In
the Python module, I suggest that you could use secrets.token_hex()
to build the filename.  Put the loop that retries because of name
collisions in the Python module as well.

If you like, I can try to make a patch that does the above.

When looking at at how the Python code would handle a name collision,
I see this code:


+        switch (errno) {
+            case EACCES:
+                PyErr_Format(pPermissionsException,
+                                "No permission to %s this segment",
+                                (flags & O_TRUNC) ? "truncate" : "access"
+                                );
+            break;
+
+            case EEXIST:
+                PyErr_SetString(pExistentialException,
+                                "Shared memory with the specified name already exists");
+            break;
+
+            case ENOENT:
+                PyErr_SetString(pExistentialException,
+                                "No shared memory exists with the specified name");
+            break;
+
+            case EINVAL:
+                PyErr_SetString(PyExc_ValueError, "Invalid parameter(s)");
+            break;
+
+            case EMFILE:
+                PyErr_SetString(PyExc_OSError,
+                                 "This process already has the maximum number of files open");
+            break;
+
+            case ENFILE:
+                PyErr_SetString(PyExc_OSError,
+                                 "The system limit on the total number of open files has been reached");
+            break;
+
+            case ENAMETOOLONG:
+                PyErr_SetString(PyExc_ValueError,
+                                 "The name is too long");
+            break;
+
+            default:
+                PyErr_SetFromErrno(PyExc_OSError);
+            break;
+        }


I think it might be cleaner just to make it:

        PyErr_SetFromErrno(PyExc_OSError);

Then, if you need a customized exception or message, you can
re-raise inside the Python code.  To me, it seems simpler and more
direct to just preserve the errno and always raise OSError.  Changing things
to ValueError means that callers need to look at the message text to 
differentiate between some errno values.

Is it the case that _posixshmem started life as a module that would
be used directly and not something hidden by another layer of
abstraction?  If so, having these customized exceptions and having
it do the filename generation itself makes sense.  However, it is an
internal implementation detail of shared_memory.py, I think it
should be simplified to do only what is needed.  E.g. a thin layer
of the system calls.
History
Date User Action Args
2019-02-07 17:52:33naschemesetrecipients: + nascheme, brett.cannon, rhettinger, terry.reedy, ronaldoussoren, pitrou, osvenskan, giampaolo.rodola, skrah, pmpp, lukasz.langa, eric.snow, yselivanov, davin
2019-02-07 17:52:31naschemesetmessageid: <1549561951.36.0.982090985835.issue35813@roundup.psfhosted.org>
2019-02-07 17:52:31naschemelinkissue35813 messages
2019-02-07 17:52:30naschemecreate