Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

relay: UNIX socket support (implement #733) #1333

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@ryan-farley
Copy link
Contributor

commented Apr 11, 2019

This adds support for relay servers listening on UNIX domain sockets.
They can be created using the /relay addpath command and specifying the
un protocol option. 'path' is now used instead of 'port' in messages,
which contains either a string representation of the port or the path to
a socket; port is negative when a UNIX socket is used. Path based relays
are stored in their own config section, with existing callbacks reused
as much as possible. Documentation has been updated as well.

relay: UNIX socket support (implement #733)
This adds support for relay servers listening on UNIX domain sockets.
They can be created using the /relay addpath command and specifying the
un protocol option. 'path' is now used instead of 'port' in messages,
which contains either a string representation of the port or the path to
a socket; port is negative when a UNIX socket is used. Path based relays
are stored in their own config section, with existing callbacks reused
as much as possible. Documentation has been updated as well.
@codecov-io

This comment has been minimized.

Copy link

commented Apr 11, 2019

Codecov Report

Merging #1333 into master will decrease coverage by 0.02%.
The diff coverage is 3.4%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1333      +/-   ##
==========================================
- Coverage   25.94%   25.92%   -0.03%     
==========================================
  Files         198      198              
  Lines       80859    80935      +76     
==========================================
+ Hits        20980    20983       +3     
- Misses      59879    59952      +73
Impacted Files Coverage Δ
src/plugins/relay/relay-client.c 1.09% <ø> (ø) ⬆️
src/plugins/relay/relay-server.c 0.58% <0%> (-0.1%) ⬇️
src/plugins/relay/relay-command.c 1.61% <0%> (-0.04%) ⬇️
src/plugins/relay/relay-config.c 39.32% <8.82%> (-3.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1582d92...ca3cc4f. Read the comment docs.

@sim642
Copy link
Contributor

left a comment

The protocol option prefix un. seems kind of vague. To me unix. would be a clearer prefix.

Is the subcommand addpath actually necessary? Seems like the same functionality could just be added to the existing add subcommand, which could accept a port (number) or a path (string) depending on the protocol option used.

Show resolved Hide resolved src/plugins/relay/relay-client.c Outdated
Show resolved Hide resolved src/plugins/relay/relay-command.c Outdated
Show resolved Hide resolved src/plugins/relay/relay-command.c Outdated
Show resolved Hide resolved src/plugins/relay/relay-command.c Outdated
Show resolved Hide resolved src/plugins/relay/relay-command.c Outdated
Show resolved Hide resolved src/plugins/relay/relay-server.c Outdated
Show resolved Hide resolved src/plugins/relay/relay-server.c Outdated
Show resolved Hide resolved src/plugins/relay/relay-server.c Outdated
Show resolved Hide resolved src/plugins/relay/relay-server.c Outdated
Show resolved Hide resolved src/plugins/relay/relay-config.h Outdated
relay: unix socket fixes per review
Strings are now translatable, the add command handles unix sockets as
well as ports, the protocol option is 'unix', not a vague 'un', and
ports are just -1 for UNIX sockets (not arbitrary negative numbers).

also allow for numeric paths by limiting path search to UNIX socket
servers only

@ryan-farley ryan-farley force-pushed the ryan-farley:unix-socket branch from ee9db7e to 0583eb4 Apr 14, 2019

@ryan-farley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Thank you for the review. I've implemented all the requested changes, which do make it a bit simpler (with no new commands and a more sensible protocol option) and actually translatable (the lack of which was just stupid on my part).

@flashcode flashcode self-assigned this May 12, 2019

@flashcode flashcode added this to the 2.5 milestone May 12, 2019

@flashcode

This comment has been minimized.

Copy link
Member

commented May 12, 2019

I agree with @sim642, I'll rename un to unix and I'll do some other minor changes (mostly style).
The merge is in progress.

@flashcode

This comment has been minimized.

Copy link
Member

commented May 12, 2019

I'll rename to unix_socket, since unix seems to be a defined integer constant (I have no idea where it is defined).

@ryan-farley

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Thank you for even considering this (let alone working to merge it). Regarding un, the constant unix is why I'd avoided it in the code, but unix_socket is more readable, yes.

@flashcode flashcode closed this in 8287107 May 13, 2019

@flashcode flashcode removed the in progress label May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.