Skip to content

ftp: split ftp_state_use_port into sub functions#20685

Closed
bagder wants to merge 2 commits intomasterfrom
bagder/ftp-port-use-split
Closed

ftp: split ftp_state_use_port into sub functions#20685
bagder wants to merge 2 commits intomasterfrom
bagder/ftp-port-use-split

Conversation

@bagder
Copy link
Member

@bagder bagder commented Feb 23, 2026

For readability and reduced complexity.

@bagder bagder added the FTP label Feb 23, 2026
@bagder bagder marked this pull request as ready for review February 23, 2026 12:34
@bagder bagder requested a review from Copilot February 23, 2026 12:34
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the large ftp_state_use_port() function into seven smaller, focused sub-functions to improve code readability and reduce complexity. The refactoring splits the original ~220-line function into logical steps: parsing the FTPPORT string, determining the default host, resolving the host, opening a socket, binding the socket, listening, and sending the FTP command. The main function now orchestrates these steps with clear error handling between each phase.

Changes:

  • Extracted parsing logic into ftp_port_parse_string() to handle the CURLOPT_FTPPORT string format
  • Extracted socket setup and FTP protocol commands into dedicated helper functions (ftp_port_open_socket, ftp_port_bind_socket, ftp_port_listen, ftp_port_send_command)
  • Extracted fallback logic into ftp_port_default_host() and ftp_port_resolve_host()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bagder
Copy link
Member Author

bagder commented Feb 23, 2026

augment review

@augmentcode
Copy link

augmentcode bot commented Feb 23, 2026

🤖 Augment PR Summary

Summary: Refactors active-mode FTP setup by splitting ftp_state_use_port() into focused helper functions to reduce complexity.

Changes:

  • Extracted parsing of CURLOPT_FTPPORT into ftp_port_parse_string() (address/interface + optional port range).
  • Added ftp_port_default_host() fallback to derive a local IP from the control connection when no host is specified.
  • Centralized DNS resolving into ftp_port_resolve_host().
  • Moved socket creation into ftp_port_open_socket() and binding logic into ftp_port_bind_socket() (including non-local bind fallback).
  • Split listen and command send steps into ftp_port_listen() and ftp_port_send_command().
  • Kept existing behavior of installing the listening socket on SECONDARYSOCKET via Curl_conn_tcp_listen_set() and enabling data-SSL when configured.

Technical Notes: Error handling/cleanup remains centralized in ftp_state_use_port() (DNS unlink, state transition to FTP_STOP, and socket close on failure).

🤖 Was this summary useful? React with 👍 or 👎

Copy link

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

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

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

@bagder
Copy link
Member Author

bagder commented Feb 23, 2026

augment review

Copy link

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

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

Review completed. 1 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

@bagder
Copy link
Member Author

bagder commented Feb 23, 2026

@aisle-analyzer

@aisle-research-bot
Copy link

aisle-research-bot bot commented Feb 23, 2026

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

# Severity Title
1 🔵 Low Incorrect parsing of unbracketed IPv6 in CURLOPT_FTPPORT may trigger unintended DNS resolution and wrong PORT/EPRT target

See details in the comment below.


Analyzed PR: #20685 at commit f568b60

@bagder bagder force-pushed the bagder/ftp-port-use-split branch from 9dd710c to 21df07a Compare February 23, 2026 15:57
@bagder bagder closed this in 29bca12 Feb 23, 2026
@bagder bagder deleted the bagder/ftp-port-use-split branch February 23, 2026 16:22
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.

2 participants