OPEN-X-STREAM refactoring, abstract UNIX support, IPv6 support #167
Conversation
| :local) | ||
| (protocol) | ||
| (t | ||
| :internet))) |
There was a problem hiding this comment.
please add a comment here (maybe move it from below), that it is a special case when the host is either "" or "unix" -- protocol is then ignored. I was convinced that it is a braino until I saw a comment later in this file.
Also there is darwin with :||. Maybe:
(if (or (member host '("" "unix") :test #'equal)
(member protocol '(:unix :||))
:local
(or protocol :internet))
There was a problem hiding this comment.
I tried to improve a few things around this normalization.
That said, I was mainly looking for feedback regarding the general direction and approach at this point.
There was a problem hiding this comment.
I first need to build an understanding of the general direction by looking at the code. I will keep remarks about the code to myself then.
There was a problem hiding this comment.
Thanks for the suggestion either way. I just didn't want you to waste your time on code that may turn out to be misguided.
* OPEN-DISPLAY calls the new helper function PROTOCOL-FROM-HOST. No
other function performs special handling of the host string being
empty or "unix".
* Define OPEN-X-STREAM as an implementation-independent function.
* The new functions MAKE-{UNIX,INET}-X-SOCKET and MAKE-X-SOCKET-STREAM
are implementation-specific.
scymtym
force-pushed
the
open-x-stream-refactoring
branch
from
1a17f70 to
c05518a
Compare
| via UNIX socket ~S:~@:_~ | ||
| ~{~{* ~@<Using a ~S failed: ~ | ||
| ~A~@:>~}~^~@:_~}~:>" | ||
| path errors)))))) |
There was a problem hiding this comment.
neither class is implemented in ecl. I understand that it is meant to be future-proof, but in that case maybe adding a fallback operation which prepends 0 to the path (could be used for other implementations too) is a good idea?
There was a problem hiding this comment.
I thought ECL and Clasp copied the entire interface. I will have to rethink the approach.
| #+(or sbcl ecl clasp) | ||
| (defun make-x-socket-stream (socket) | ||
| (socket-make-stream socket :element-type '(unsigned-byte 8) | ||
| :input t :output t :buffering :none)) |
There was a problem hiding this comment.
name socket may be a bit misleading, make-x-stream, which directly corresponds to open-x-stream?
| (lose (list "~S is not a known protocol" protocol)))) | ||
| (when (null socket) | ||
| (lose reason)) | ||
| (make-x-socket-stream socket)))) |
There was a problem hiding this comment.
having three functions to implement instead of one doesn't sound appealing. maybe open-x-stream should provide a canonical form of arguments to the make-x-stream which is implemented for each lisp and is responsible for creating the socket/fd?
(defun make-unix-stream (&key path host port) …)
where some options are mutually exclusive
There was a problem hiding this comment.
One downside of a single function would be a more complicated signature as evidenced by the need for mutually exclusive keyword parameters.
The other potential downside is less sharing between implementations. Going by the previous comment, maybe it is necessary to implement the function for local sockets differently for SBCL and ECL, but maybe the function for TCP sockets can still be shared.
This is a draft for which I would like to receive feedback. Since the changes touch a central part of CLX, going further without a proper discussion doesn't seem like a good idea.
OK, so these changes restructure the functions around
open-displayinto a few larger implementation-independent pieces and a few much smaller implementation-dependent pieces.One benefit is that the detailed error messages which were previously only implemented for CMUCL are now in the implementation-independent part.
Similarly, the logic for deriving the protocol from the host (i.e. the host being
""or"unix"means protocol:local) is now in a helper function instead of being repeated in multiple places.One of the things I'm not sure about is Clisp. It seems to handle more of the X stream opening by itself compared to other implementations, so the changed design may not be appropriate for Clisp.
Furthermore, these changes add support for abstract UNIX sockets (some Linux distributions seem to play containerization tricks with this) and IPv6. These two enhancements could be separate (follow-up) pull requests, if this one is considered too big.