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

Passive DCC #2004

Closed
Closed

Conversation

mario-campos
Copy link
Contributor

@mario-campos mario-campos commented Aug 23, 2023

This PR adds initial support for passive DCC. Specifically, it supports a non-WeeChat client sending DCC SEND with a port of 0, which causes weechat to open a listening socket, accept the first client that connects, and start a new xfer.

  • It does (or should) respect the xfer.network.own_ip config property.
  • It does (or should) respect the xfer.network.port_range config property.
  • It does (or should) support the passive DCC RESUME extension (linked above).

Closes #487.

This makes it easier to handle the optional "token" argument at the (right) end, which will be necessary to support passive DCC.

Incidentally, this is RtL parsing order is the reason why you'd get a cryptic "0" address error when attempting to do passive DCC: the "token" argument gets misinterpreted as the "size" argument. Every argument "shifts" over by one, leaving an address (port) of "0".
Before making any significant changes, let's identify the existing xfer types by either active or passive.
@flashcode flashcode added the feature New feature request label Aug 23, 2023
Copy link
Member

@flashcode flashcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The token is never set by WeeChat itself so if I understand WeeChat only supports passive DCC as a "receiver".

The help on command /dcc (ie /help dcc) should maybe mention passive DCC being supported.

Besides, the unit tests are failing, because some network address resolution seems to be made and causes a test to fail.

src/plugins/irc/irc-command.c Show resolved Hide resolved
src/plugins/irc/irc-ctcp.c Show resolved Hide resolved
src/plugins/irc/irc-ctcp.c Show resolved Hide resolved
src/plugins/xfer/xfer.c Outdated Show resolved Hide resolved
@flashcode flashcode self-assigned this Aug 23, 2023
@mario-campos
Copy link
Contributor Author

The token is never set by WeeChat itself so if I understand WeeChat only supports passive DCC as a "receiver".

Correct.

The help on command /dcc (ie /help dcc) should maybe mention passive DCC being supported.
Besides, the unit tests are failing, because some network address resolution seems to be made and causes a test to fail.

I'll address these.

@flashcode
Copy link
Member

How can I test this passive DCC, which client and/or IRC server are you using?

@mario-campos
Copy link
Contributor Author

You'll need a public IP address and to expose a port—I tested with IPv4.

Then, join #THE.SOURCE and #The.Lounge on irc.scenep2p.net. Look for a XDCC bot with P in the nick to ensure it will trigger the passive-DCC code. See here for SceneP2P's instructions.

As for the clients, I've tested with irssi and Weechat.

@mario-campos
Copy link
Contributor Author

@flashcode, can you please point me to the file that contains the help docs (/help dcc)?

@flashcode
Copy link
Member

@flashcode, can you please point me to the file that contains the help docs (/help dcc)?

/dcc is an IRC command so it's located in irc-command.c: https://github.com/weechat/weechat/blob/master/src/plugins/irc/irc-command.c#L7097

@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2023

Codecov Report

Merging #2004 (06306b9) into master (49f52cc) will increase coverage by 0.11%.
Report is 36 commits behind head on master.
The diff coverage is 46.93%.

❗ Current head 06306b9 differs from pull request most recent head 46d0601. Consider uploading reports for the commit 46d0601 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #2004      +/-   ##
==========================================
+ Coverage   47.10%   47.22%   +0.11%     
==========================================
  Files         216      216              
  Lines       91314    91774     +460     
==========================================
+ Hits        43017    43342     +325     
- Misses      48297    48432     +135     
Files Changed Coverage Δ
src/core/hook/wee-hook-fd.c 33.07% <0.00%> (ø)
src/core/hook/wee-hook-focus.c 18.34% <0.00%> (ø)
src/core/hook/wee-hook-process.c 0.00% <0.00%> (ø)
src/core/hook/wee-hook-timer.c 31.11% <0.00%> (ø)
src/core/wee-debug.c 23.35% <0.00%> (+0.08%) ⬆️
src/plugins/fset/fset-buffer.c 3.16% <0.00%> (-0.02%) ⬇️
src/plugins/fset/fset-config.c 85.88% <0.00%> (ø)
src/plugins/irc/irc-command.c 20.16% <0.00%> (-0.09%) ⬇️
src/plugins/irc/irc-config.c 61.02% <0.00%> (-0.08%) ⬇️
src/plugins/irc/irc-list.c 14.02% <0.00%> (-0.03%) ⬇️
... and 37 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

This commit also includes support for passive DCC RESUME.

There was also a potential segfault with calling `atoi(pos_token)` when `pos_token` is NULL, so `token` is set to be stored as a string. Although it is an integer, we don't need to store it as such. That's really more of an implementation detail.
@mario-campos
Copy link
Contributor Author

@flashcode, can you try running the test suite again? It should pass now.

@mario-campos
Copy link
Contributor Author

@flashcode, have you been able to test this PR?

@flashcode
Copy link
Member

I'll test it soon, in the next days.

@flashcode flashcode added the in progress Someone is working on this issue label Sep 6, 2023
flashcode added a commit that referenced this pull request Sep 6, 2023
flashcode added a commit that referenced this pull request Sep 6, 2023
flashcode added a commit that referenced this pull request Sep 6, 2023
flashcode added a commit that referenced this pull request Sep 6, 2023
flashcode added a commit that referenced this pull request Sep 6, 2023
@flashcode flashcode removed the in progress Someone is working on this issue label Sep 6, 2023
@flashcode
Copy link
Member

Merged, thanks for adding this new feature!

@flashcode flashcode closed this Sep 6, 2023
@flashcode flashcode added this to the 4.1.0 milestone Sep 6, 2023
@flashcode flashcode mentioned this pull request Sep 6, 2023
@mario-campos mario-campos deleted the mario-campos/passive-dcc branch September 7, 2023 16:39
@flashcode
Copy link
Member

Hi @mario-campos,

For your information, this PR introduced a regression with DCC chat, causing a crash when you do /dcc chat <nick>.

I fixed the issue in this commit e62ff28 and you could look if my changes are OK for you.
At least I tested the DCC chat which is now working again, but didn't fully retest the passive DCC.

@mario-campos
Copy link
Contributor Author

Sorry about introducing that regression. I retested the passive DCC functionality against the HEAD of master (f49810e) and it still works. Thanks!

@ksurl
Copy link

ksurl commented Nov 16, 2023

how should the port range be written in xfer.conf?

will a dash work? or does it use a colon

port_range = "2000-2010"

@flashcode
Copy link
Member

@ksurl: it's a dash, see /help xfer.network.port_range:

description: restricts outgoing files/chats and incoming/passive files to use only ports
in the given range (useful for NAT) (syntax: a single port, ie. 5000 or a port range,
ie. 5000-5015, empty value means any port, it's recommended to use ports greater than 1024,
because only root can use ports below 1024)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Passive DCC
4 participants