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

Add a new flag in remote for connecting through a Unix domain socket. #3552

Merged
merged 3 commits into from Aug 19, 2022
Merged

Add a new flag in remote for connecting through a Unix domain socket. #3552

merged 3 commits into from Aug 19, 2022

Conversation

Kobaxidze256
Copy link
Contributor

I added a new argument and passed it over to libcurl like how curl's --unix-socket argument works.
Closes #3544.

utils/remote.cc Outdated Show resolved Hide resolved
utils/remote.cc Outdated Show resolved Hide resolved
@LaserEyess
Copy link
Contributor

Codewise this LGTM and how I would have done it. It appears curl is doing the right thing behind the scenes in checking for the socket path length. However, it isn't passing this feedback to the user. Consider this:

$ ./utils/transmission-remote google.com:443 --unix-socket /tmp/transmission.sock -l                                       
    ID   Done       Have  ETA           Up    Down  Ratio  Status       Name
     1   100%    3.12 GB  Done         0.0     0.0   0.00  Idle         ubuntu-21.10-desktop-amd64.iso
Sum:             3.12 GB               0.0     0.0
$ echo $?
0

vs.

$ ./utils/transmission-remote google.com:443 --unix-socket /tmp/transmission.sockasdahdfoasihdfoaishdfoiashdfoiashdfoiahsdofihasodifhasoidfhsssssssssssssssssssssssssssssssssssssssssssssssssss -l
$ echo $?
1

This is not good UI imo, especially given the fact that curl is in fact providing this as feedback:

$ ./utils/transmission-remote google.com:443 -b --unix-socket /tmp/transmission.sockasdahdfoasihdfoaishdfoiashdfoiashdfoiahsdofihasodifhasoidfhsssssssssssssssssssssssssssssssssssssssssssssssssss -l 
posting:
--------
{"arguments":{"fields":["error","errorString","eta","id","isFinished","leftUntilDone","name","peersGettingFromUs","peersSendingToUs","rateDownload","rateUpload","sizeWhenDone","status","uploadRatio"]},"method":"torrent-get","tag":6}

--------
* Couldn't find host google.com in the (nil) file; using defaults
* Unix socket path too long: '/tmp/transmission.sockasdahdfoasihdfoaishdfoiashdfoiashdfoiahsdofihasodifhasoidfhsssssssssssssssssssssssssssssssssssssssssssssssssss'
* Closing connection 0

The second thing to notice from this is that I can use whatever host/port I want and it works, I think that's confusing but truth be told it doesn't really affect anything, --unix-socket is pretty explicit.

Besides that UI nit, I'm wondering long term if @mikedld's was right in #3544. One of my goals if I stop being lazy is to make unix sockets also accept raw JSON instead of using HTTP for rpc. In this case it would make sense to parse unix:// vs http:// or unix+http://. However, the way transmission-remote works right now it doesn't even take http{s,}:// as an argument so that really has nothing to do with this PR. Plus, non-http-rpc is hypothetical at this point so I don't think it should be a blocker.

@Kobaxidze256
Copy link
Contributor Author

@LaserEyess I don't understand what I should change about the output. Also are the check fails my fault?

@LaserEyess
Copy link
Contributor

The check doesn't fail, it succeeds, but the feedback isn't being given to the user. That's my point. I don't really think it's a dealbreaker, I haven't even looked at how transmission-remote handles other errors from curl.

@Kobaxidze256
Copy link
Contributor Author

The check doesn't fail, it succeeds, but the feedback isn't being given to the user. That's my point. I don't really think it's a dealbreaker, I haven't even looked at how transmission-remote handles other errors from curl.

Sorry for the delay. Other errors are handled the same way. This seems to be a problem only on the master branch, the last release works fine.

@ckerr ckerr merged commit c4537e6 into transmission:main Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Unix domain socket support in transmission-remote
3 participants