ralphlange / procServ

Wrapper to start arbitrary interactive commands in the background, with telnet or Unix domain socket access to stdin/stdout
GNU General Public License v3.0
23 stars 23 forks source link

Add a PROCSERV_INFO environment variable #33

Closed kmpeters closed 4 years ago

kmpeters commented 4 years ago

That contains the info from the pid file and the info file.

kmpeters commented 4 years ago

This shouldn't be merged yet. I've only tested on Linux.

ralphlange commented 4 years ago

Looks good so far, though.

ralphlange commented 4 years ago

@mdavidsaver: Should we rather...

kmpeters commented 4 years ago

Also, is there a specific version of cygwin I should use for testing on Windows?

ralphlange commented 4 years ago

Nah. I have always tested with a current one and never really cared. The stuff procServ uses is pretty basic and has been stable for a long time.

mdavidsaver commented 4 years ago

How about including both the new and old formats? So three lines in total.

kmpeters commented 4 years ago

How about including both the new and old formats? So three lines in total.

The new format is always a single line. The old format is a variable number of lines that is only limited by the number of control ports a user chooses to create.

mdavidsaver commented 4 years ago

Can you give an example of what the content would look like?

kmpeters commented 4 years ago

Can you give an example of what the content would look like?

Old format, written to info file (tcp):

pid:32120
tcp:0.0.0.0:8023
tcp:127.0.0.1:31337

New format, written to the PROCSERV_INFO env var (tcp):

PID=32120;LOG=tcp:0.0.0.0:8023;CTL=tcp:127.0.0.1:31337

Old format, written to info file (socket):

pid:32150
unix:log.sock
unix:ctrl.sock

New format, written to the PROCSERV_INFO env var (socket):

PID=32150;LOG=unix:log.sock;CTL=unix:ctrl.sock

There can be 0 or 1 log ports. There can be many control ports, presumably only limited by the number of free ports on the system.

ralphlange commented 4 years ago

I would probably keep the newlines instead of semicolon as a separator in the file. Seems more appropriate.

ralphlange commented 4 years ago

The problem with the old format: You cannot distinguish between log and control ports. If there is only one port, it must be of control type. As soon as there are more than one, the first might be a log port or not.

mdavidsaver commented 4 years ago

Ok, I understand. You want to avoid newlines in an environment variable. While not necessary, this is certainly friendly. I also consider it friendly to do the opposite in a file. This suggests different syntax. At which point, why break compatibility? (adding new lines to the file won't break compatibility)

ralphlange commented 4 years ago

The existing file format is ambiguous.

pid:32150
tcp:1.2.3.4:5678
tcp:9.8.7.6:5432

could be one log port and one control port or two control ports.

mdavidsaver commented 4 years ago

Since I asked for an example, I should give one...

pid:32150 tcp:1.2.3.4:5678 tcp:9.8.7.6:5432

would become

pid:32150 PID=32150 tcp:1.2.3.4:5678 CTL=tcp:1.2.3.4:5678 LOG=tcp:9.8.7.6:5432

So I would resolve the ambiguity in favor of the control ports.

ralphlange commented 4 years ago

But in the old format a log port is always the first one and printed before the control port(s). So

pid:32150
tcp:1.2.3.4:5678
tcp:9.8.7.6:5432

must become

pid:32150
PID=32150
tcp:1.2.3.4:5678
LOG=tcp:1.2.3.4:5678
tcp:9.8.7.6:5432
CTL=tcp:9.8.7.6:5432

in the case of one log and one control port, but

pid:32150
PID=32150
tcp:1.2.3.4:5678
CTL=tcp:1.2.3.4:5678
tcp:9.8.7.6:5432
CTL=tcp:9.8.7.6:5432

in the case of two control ports. Correct?

ralphlange commented 4 years ago

Still, that new format would need any parser to be changed and thus break compatibility. What is the advantage of keeping the old format lines interspersed?

mdavidsaver commented 4 years ago

would need any parser to be changed

Actually not.

https://github.com/ralphlange/procServ/blob/2659e26bdb3ed2114db1645c42b9d8a0688d104e/procServUtils/attach.py#L16-L26

ralphlange commented 4 years ago

ok, you win.

mdavidsaver commented 4 years ago

Also

https://github.com/ralphlange/procServ/blob/2659e26bdb3ed2114db1645c42b9d8a0688d104e/procServUtils/manage.py#L36-L44

mdavidsaver commented 4 years ago

Do I get a prize?

ralphlange commented 4 years ago

But does that logically work if you define a log endpoint? Or would it happily connect you to a read-only port?

mdavidsaver commented 4 years ago

No, I think log endpoints would break this scheme. Which is why I'm inclined to break this breakage.

ralphlange commented 4 years ago

Do you know of any third-party parsers? If not (and since the info file did not arrive in the official Debian/Fedora packages yet): would breaking compatibility be that bad?

mdavidsaver commented 4 years ago

Where have I heard that question before ;)

No, I don't know of any. And no, it wouldn't be that bad. It's also doesn't seem that expensive to me. Of course the ultimate decision is yours to make.

kmpeters commented 4 years ago

I'm struggling to get a working cygwin install on my Windows 10 machine. A number of packages seem to be missing from all the mirrors I've tried:

w32api-runtime-7.0.0-1
binutils-2.34-1
texinfo-6.7-1

As soon as I get cygwin installed, I'll test procServ on it. The binutils eventually installs after a number of retries, but the w32api-runtime seems to be consistently a problem.

ralphlange commented 4 years ago

Hm. I will try - if I remember correctly I can install cygwin without Admin rights. (ITER laptop managed by the IT group - I needed to escalate to division head level only to get a second bank of memory...)

kmpeters commented 4 years ago

I was able to get the missing cygwin packages installed by downloading them manually from a mirror. w32api-runtime-7.0.0-1 in particular took a very long time to download, presumably due to Argonne's virus scanner that scans everything before I download it. I've built procServ on Windows. I'll report the results of my testing soon.

kmpeters commented 4 years ago

There are no issues with this pull request on Windows. I've successfully run procServ and confirmed the new PROCSERV_INFO environment variable is set. The info file is successfully written in the original format.

My search for a current OS X machine continues.

kmpeters commented 4 years ago

I've successfully tested this pull request with TCP endpoints on an OS X 10.13.6 machine.

When I try to use unix socket endpoints, I get the following message:

Unix sockets not supported on this host

Here is the configure output:

$ ./configure --disable-doc
checking build system type... x86_64-apple-darwin17.7.0
checking host system type... x86_64-apple-darwin17.7.0
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... build-aux/install-sh -c -d
checking for gawk... no
checking for mawk... no
checking for nawk... no
checking for awk... awk
checking whether make sets $(MAKE)... yes
checking whether make supports nested variables... yes
checking for g++... g++
checking whether the C++ compiler works... yes
checking for C++ compiler default output file name... a.out
checking for suffix of executables... 
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C++ compiler... yes
checking whether g++ accepts -g... yes
checking whether make supports the include directive... yes (GNU style)
checking dependency style of g++... gcc3
checking for gcc... gcc
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking whether gcc understands -c and -o together... rm: conftest.dSYM: is a directory
yes
checking dependency style of gcc... gcc3
checking how to run the C preprocessor... gcc -E
checking for grep that handles long lines and -e... /usr/bin/grep
checking for egrep... /usr/bin/grep -E
checking for ANSI C header files... rm: conftest.dSYM: is a directory
rm: conftest.dSYM: is a directory
yes
checking for sys/types.h... yes
checking for sys/stat.h... yes
checking for stdlib.h... yes
checking for string.h... yes
checking for memory.h... yes
checking for strings.h... yes
checking for inttypes.h... yes
checking for stdint.h... yes
checking for unistd.h... yes
checking pty.h usability... no
checking pty.h presence... no
checking for pty.h... no
checking libutil.h usability... no
checking libutil.h presence... no
checking for libutil.h... no
checking util.h usability... yes
checking util.h presence... yes
checking for util.h... yes
checking for stdbool.h that conforms to C99... yes
checking for _Bool... yes
checking for an ANSI C-conforming const... yes
checking for uid_t in sys/types.h... yes
checking for mode_t... yes
checking for pid_t... yes
checking for C/C++ restrict keyword... __restrict
checking whether struct tm is in sys/time.h or time.h... time.h
checking vfork.h usability... no
checking vfork.h presence... no
checking for vfork.h... no
checking for fork... yes
checking for vfork... yes
checking for working fork... yes
checking for working vfork... (cached) yes
checking type of array argument to getgroups... gid_t
checking for size_t... yes
checking for getgroups... yes
checking for working getgroups... yes
checking whether gcc needs -traditional... no
checking for ranlib... ranlib
checking for strftime... yes
checking for telnet_recv in -ltelnet... no
checking for library containing socket... none required
checking for library containing setsockopt... none required
checking for library containing inet_aton... none required
checking for library containing inet_ntop... none required
checking for library containing forkpty... none required
checking for forkpty... yes
checking for a2x... no
checking for asciidoc... no
checking for xmllint... xmllint
checking for xsltproc... xsltproc
checking for fop... no
checking for dblatex... no
checking that generated files are newer than configure... done
configure: creating ./config.status
config.status: creating Makefile
config.status: executing depfiles commands
kmpeters commented 4 years ago

Is there a specific socket lib that should be installed on OS X? I expected one to be available by default...

ralphlange commented 4 years ago

procServ.h has:

/* whether to enable UNIX domain sockets */
#ifdef __unix__
#include <sys/un.h>
# define USOCKS
#endif

According to stack overflow, this should be changed to

#if defined(unix) || defined(__unix__) || defined(__unix)
kmpeters commented 4 years ago

This didn't enable sockets on OS X for me:

#if defined(unix) || defined(__unix__) || defined(__unix)

But this did:

#if defined(unix) || defined(__unix__) || defined(__unix) || defined(__APPLE__)

This pull request has now been successfully tested on OS X with TCP and unix socket endpoints.

Note: I have NOT applied the above change to the pull request. I assume it is desirable to have the commit that enables unix sockets on OS X be separate from this pull request. If that isn't the case, I'm happy to commit the change above.

ralphlange commented 4 years ago

Separate, rightly guessed. So - tested on Linux, Windows and MacOS, right?

kmpeters commented 4 years ago

So - tested on Linux, Windows and MacOS, right?

Yes. On all three platforms the info in the PROCSERV_INFO environment variable matches the info in the info file and the info file retains the old format, when both tcp and unix socket endpoints are used.

kmpeters commented 4 years ago

Is there anything else I should do for this pull request?

ralphlange commented 4 years ago

No. Thanks a lot!! I will commit the change to enable Unix domain sockets on MacOS on the master branch. That line was all that was needed, right?

kmpeters commented 4 years ago

I will commit the change to enable Unix domain sockets on MacOS on the master branch. That line was all that was needed, right?

Yes. That is all that was needed.