From ab496b62705d3b7908b4bb56bd407bf7155ea649 Mon Sep 17 00:00:00 2001 From: Ingela Anderton Andin Date: Fri, 29 Jan 2021 18:43:28 +0100 Subject: ftp: Use OTP supervisor as intended Due to legacy reasons FTP clients are not part of the FTP applications supervisor tree unless they where started with start_service/1. This function is a legcy from a mechanism in inets that was never intended to be part of the ftp application. --- lib/ftp/doc/src/ftp.xml | 23 ++++++++----------- lib/ftp/doc/src/ftp_client.xml | 4 ++-- lib/ftp/src/ftp.erl | 44 ++++++++---------------------------- lib/ftp/test/ftp_SUITE.erl | 41 ++++----------------------------- lib/stdlib/src/otp_internal.erl | 4 ++++ system/doc/general_info/DEPRECATIONS | 3 +++ 6 files changed, 34 insertions(+), 85 deletions(-) diff --git a/lib/ftp/doc/src/ftp.xml b/lib/ftp/doc/src/ftp.xml index c643ba4fa0..3dc0c097ad 100644 --- a/lib/ftp/doc/src/ftp.xml +++ b/lib/ftp/doc/src/ftp.xml @@ -42,12 +42,10 @@ to active FTP mode if this fails. This default behavior can be changed by start option mode.

- - -

An FTP client can be started in two ways. One is using the - service_start function, - the other is to start it directly as a standalone process - using function open.

+

An FTP client is always started as part of the ftp application + and legacy + start_service function, + is deprecated in OTP-24

For a simple example of an FTP session, see FTP User's Guide.

@@ -74,16 +72,16 @@ error if the request is a listing of the contents of a directory that exists but is empty.

- +
- FTP CLIENT SERVICE START/STOP + FTP CLIENT START/STOP

The FTP client can be started and stopped dynamically in runtime by calling the ftp application API - ftp:start_service(ServiceConfig) and - ftp:stop_service(Pid).

+ ftp:open(Host, Options) and + ftp:close(Client).

The available configuration options are as follows:

@@ -538,7 +536,7 @@ open(Host) -> {ok, Pid} | {error, Reason} open(Host, Opts) -> {ok, Pid} | {error, Reason} - Starts a standalone FTP client. + Starts a FTP client. Host = string() | ip_address() Opts = options() @@ -564,8 +562,7 @@ -

Starts a standalone FTP client process - (without the ftp service framework) and +

Starts a FTP client process and opens a session with the FTP server at Host.

If option {tls, tls_options()} is present, the FTP session diff --git a/lib/ftp/doc/src/ftp_client.xml b/lib/ftp/doc/src/ftp_client.xml index 047b055be7..7686548388 100644 --- a/lib/ftp/doc/src/ftp_client.xml +++ b/lib/ftp/doc/src/ftp_client.xml @@ -55,7 +55,7 @@ ftp:start(). ok - 2> {ok, Pid} = ftp:start_service([{host, "erlang.org"}]). + 2> {ok, Pid} = ftp:open([{host, "erlang.org"}]). {ok,<0.22.0>} 3> ftp:user(Pid, "guest", "password"). ok @@ -69,7 +69,7 @@ ok 8> ftp:recv(Pid, "appl.erl"). ok - 9> ftp:stop_service(Pid). + 9> ftp:close(Pid). ok 10> ftp:stop(). ok diff --git a/lib/ftp/src/ftp.erl b/lib/ftp/src/ftp.erl index 4ad6acaef0..c4d909a1d0 100644 --- a/lib/ftp/src/ftp.erl +++ b/lib/ftp/src/ftp.erl @@ -23,17 +23,15 @@ -behaviour(gen_server). +-deprecated([{start_service, 1, "use ftp:open/2 instead"}, + {stop_service, 1, "use ftp:close/1 instead"}]). + -export([start/0, start_service/1, stop/0, - stop_service/1, - services/0, - service_info/1 + stop_service/1 ]). -%% Added for backward compatibility --export([start_standalone/1]). - -export([start_link/1, start_link/2]). %% API - Client interface @@ -136,20 +134,10 @@ start() -> application:start(ftp). -start_standalone(Options) -> - try - {ok, StartOptions} = start_options(Options), - case start_link(StartOptions, []) of - {ok, Pid} -> - call(Pid, {open, ip_comm, Options}, plain); - Error1 -> - Error1 - end - catch - throw:Error2 -> - Error2 - end. - +%% This should be made an internal function when we remove the deprecation +%% ftp client processes should always be part of ftp supervisor tree. +%% We consider it a bug that the "standalone" concept of inets was +%% not removed when ftp was broken out, and it is now fixed. start_service(Options) -> try {ok, StartOptions} = start_options(Options), @@ -170,17 +158,6 @@ stop() -> stop_service(Pid) -> close(Pid). -services() -> - [{ftpc, Pid} || {_, Pid, _, _} <- - supervisor:which_children(ftp_sup)]. -service_info(Pid) -> - {ok, Info} = call(Pid, info, list), - {ok, [proplists:lookup(mode, Info), - proplists:lookup(local_port, Info), - proplists:lookup(peer, Info), - proplists:lookup(peer_port, Info)]}. - - %%%========================================================================= %%% API - CLIENT FUNCTIONS %%%========================================================================= @@ -215,7 +192,7 @@ open(Host, Port) when is_integer(Port) -> %% open(Host, Options) when is_list(Options) -> - start_standalone([{host,Host}|Options]). + start_service([{host,Host}|Options]). %%-------------------------------------------------------------------------- %% user(Pid, User, Pass, ) -> ok | {error, euser} | {error, econn} @@ -1520,8 +1497,7 @@ code_change(_Vsn, State, _Extra) -> %% start_link([Opts, GenServerOptions]) -> {ok, Pid} | {error, Reason} %% %% Description: Callback function for the ftp supervisor. It is called -%% : when start_service/1 calls ftp_sup:start_child/1 to start an -%% : instance of the ftp process. Also called by start_standalone/1 +%% : when open or legacy is called. %%-------------------------------------------------------------------------- start_link([Opts, GenServerOptions]) -> start_link(Opts, GenServerOptions). diff --git a/lib/ftp/test/ftp_SUITE.erl b/lib/ftp/test/ftp_SUITE.erl index d5c827f0f2..b2cdd882c6 100644 --- a/lib/ftp/test/ftp_SUITE.erl +++ b/lib/ftp/test/ftp_SUITE.erl @@ -128,7 +128,6 @@ ftp_tests_smoke() -> ftp_sup_tests() -> [ - start_ftp, ftp_worker ]. @@ -292,22 +291,10 @@ init_per_group(Group, Config) when Group == ftpes_passive; _:_ -> {skip, "Crypto did not start"} end; -init_per_group(ftp_sup, Config) -> - try ftp:start() of - ok -> - start_ftpd(Config) - catch - _:_ -> - {skip, "Ftp did not start"} - end; + init_per_group(_Group, Config) -> start_ftpd(Config). - -end_per_group(ftp_sup, Config) -> - ftp:stop(), - stop_ftpd(Config), - Config; end_per_group(_Group, Config) -> stop_ftpd(Config), Config. @@ -316,6 +303,7 @@ end_per_group(_Group, Config) -> init_per_testcase(T, Config0) when T =:= app; T =:= appup -> Config0; init_per_testcase(Case, Config0) -> + application:ensure_started(ftp), case Case of error_datafail -> catch crypto:stop(), @@ -362,7 +350,7 @@ init_per_testcase2(Case, Config0) -> ftpes_active_reuse -> ftp__open(Config0, TLSReuse ++ ACTIVE ++ ExtraOpts); ftps_passive_reuse -> ftp__open(Config0, SSLReuse ++ PASSIVE ++ ExtraOpts); ftps_active_reuse -> ftp__open(Config0, SSLReuse ++ ACTIVE ++ ExtraOpts); - ftp_sup -> ftp_start_service(Config0, ACTIVE ++ ExtraOpts); + ftp_sup -> ftp__open(Config0, ACTIVE ++ ExtraOpts); undefined -> Config0 end, case Case of @@ -1025,26 +1013,6 @@ clean_shutdown(Config) -> end end. -%%------------------------------------------------------------------------- -start_ftp() -> - [{doc, "Start/stop of ftp service"}]. -start_ftp(Config) -> - Pid0 = proplists:get_value(ftp,Config), - Pids0 = [ServicePid || {_, ServicePid} <- ftp:services()], - true = lists:member(Pid0, Pids0), - {ok, [_|_]} = ftp:service_info(Pid0), - ftp:stop_service(Pid0), - ct:sleep(100), - Pids1 = [ServicePid || {_, ServicePid} <- ftp:services()], - false = lists:member(Pid0, Pids1), - - Host = proplists:get_value(ftpd_host,Config), - Port = proplists:get_value(ftpd_port,Config), - - {ok, Pid1} = ftp:start_standalone([{host, Host},{port, Port}]), - Pids2 = [ServicePid || {_, ServicePid} <- ftp:services()], - false = lists:member(Pid1, Pids2). - %%------------------------------------------------------------------------- ftp_worker() -> [{doc, "Makes sure the ftp worker processes are added and removed " @@ -1053,7 +1021,7 @@ ftp_worker(Config) -> Pid = proplists:get_value(ftp,Config), case supervisor:which_children(ftp_sup) of [{_,_, worker, [ftp]}] -> - ftp:stop_service(Pid), + ftp:close(Pid), ct:sleep(5000), [] = supervisor:which_children(ftp_sup), ok; @@ -1264,6 +1232,7 @@ ftpd_running(Config) -> undefined =/= ChkUp(proplists:get_value(ftpd_start_result,Config)). ftp__open(Config, Options) -> + application:ensure_started(ftp), Host = proplists:get_value(ftpd_host,Config), Port = proplists:get_value(ftpd_port,Config), ct:log("Host=~p, Port=~p",[Host,Port]), diff --git a/lib/stdlib/src/otp_internal.erl b/lib/stdlib/src/otp_internal.erl index 6f691f2c3c..55fbbc4a6e 100644 --- a/lib/stdlib/src/otp_internal.erl +++ b/lib/stdlib/src/otp_internal.erl @@ -23,6 +23,10 @@ -include("otp_internal.hrl"). %% -dialyzer({no_match, obsolete/3}). +obsolete(ftp, start_service, 1) -> + {deprecated, "use ftp:open/2 instead"}; +obsolete(ftp, stop_service, 1) -> + {deprecated, "use ftp:close/1 instead"}; obsolete(auth, cookie, 0) -> {deprecated, "use erlang:get_cookie/0 instead"}; obsolete(auth, cookie, 1) -> diff --git a/system/doc/general_info/DEPRECATIONS b/system/doc/general_info/DEPRECATIONS index 0b18d6216f..54994ce80f 100644 --- a/system/doc/general_info/DEPRECATIONS +++ b/system/doc/general_info/DEPRECATIONS @@ -20,6 +20,9 @@ # # Added in OTP 24. # + +ftp:start_service/1 since=24 remove=26 +ftp:start_service/1 since=24 remove=26 httpd_util:flatlength/1 since=24 remove=26 httpd_util:hexlist_to_integer/1 since=24 remove=26 httpd_util:integer_to_hexlist/1 since=24 remove=26 -- cgit v1.2.1 From 586d671d384675cbaf67c42eab3a16d14a0386b7 Mon Sep 17 00:00:00 2001 From: Ingela Anderton Andin Date: Thu, 8 Oct 2020 09:15:12 +0200 Subject: ftp: Handle control channel ack arriving before all data --- lib/ftp/src/ftp.erl | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/ftp/src/ftp.erl b/lib/ftp/src/ftp.erl index c4d909a1d0..4b15c58b19 100644 --- a/lib/ftp/src/ftp.erl +++ b/lib/ftp/src/ftp.erl @@ -1133,11 +1133,17 @@ handle_call({_, recv_chunk}, _From, #state{chunk = true, } } = State0) -> %% The ftp:recv_chunk call was the last event we waited for, finnish and clean up - ?DBG("recv_chunk_closing ftp:recv_chunk, last event",[]), + ?DBG("Data connection closed recv_chunk_closing ftp:recv_chunk, last event",[]), State = activate_ctrl_connection(State0), {reply, ok, State#state{caller = undefined, chunk = false, client = undefined}}; +handle_call({_, recv_chunk}, From, #state{chunk = true, + caller = #recv_chunk_closing{pos_compl_received = true + } = R + } = State0) -> + State = activate_data_connection(State0), + {reply, ok, State#state{client = From, caller = R#recv_chunk_closing{client_called_us=true}}}; handle_call({_, recv_chunk}, From, #state{chunk = true, caller = #recv_chunk_closing{} = R -- cgit v1.2.1 From 98c6d8e4447350a316724d4b699661b2e530dc30 Mon Sep 17 00:00:00 2001 From: Ingela Anderton Andin Date: Mon, 8 Feb 2021 12:31:14 +0100 Subject: ftp: Fix test case logic --- lib/ftp/test/ftp_SUITE.erl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/ftp/test/ftp_SUITE.erl b/lib/ftp/test/ftp_SUITE.erl index b2cdd882c6..dfb85ff387 100644 --- a/lib/ftp/test/ftp_SUITE.erl +++ b/lib/ftp/test/ftp_SUITE.erl @@ -1007,9 +1007,9 @@ clean_shutdown(Config) -> exit(HelperPid, shutdown), timer:sleep(2000), error_logger:logfile(close), - case is_error_report_6035(LogFile) of - true -> ok; - false -> {fail, "Bad logfile"} + case unwanted_error_report(LogFile) of + true -> {fail, "Bad logfile"}; + false -> ok end end. @@ -1395,11 +1395,11 @@ progress_report_receiver_loop(Parent, N) -> %%%---------------------------------------------------------------- %%% Help functions for bug OTP-6035 -is_error_report_6035(LogFile) -> +unwanted_error_report(LogFile) -> case file:read_file(LogFile) of {ok, Bin} -> nomatch =/= binary:match(Bin, <<"=ERROR REPORT====">>); _ -> - false + ct:fail({no_logfile, LogFile}) end. -- cgit v1.2.1 From 1047227b007529832bb17d0ee6775a1c47e4f56d Mon Sep 17 00:00:00 2001 From: Ingela Anderton Andin Date: Tue, 6 Oct 2020 11:24:05 +0200 Subject: ftp: Wait for user to call recv_chunk/1 --- lib/ftp/src/ftp.erl | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/ftp/src/ftp.erl b/lib/ftp/src/ftp.erl index 4b15c58b19..92d2119945 100644 --- a/lib/ftp/src/ftp.erl +++ b/lib/ftp/src/ftp.erl @@ -1887,6 +1887,10 @@ handle_ctrl_result({pos_compl, _}, #state{caller = #recv_chunk_closing{}=R} ?DBG("recv_chunk_closing pos_compl, wait more",[]), {noreply, State0#state{caller = R#recv_chunk_closing{pos_compl_received=true}}}; +handle_ctrl_result({pos_compl, _}, #state{caller = undefined, chunk = true} + = State0) -> + %% Waiting for user to call recv_chunk + {noreply, State0#state{caller = #recv_chunk_closing{pos_compl_received=true}}}; %%-------------------------------------------------------------------------- %% File handling - recv_file -- cgit v1.2.1 From c1c7a32a80aae3bcb4a56ad8ef31bfeba7b6af3e Mon Sep 17 00:00:00 2001 From: Ingela Anderton Andin Date: Wed, 14 Oct 2020 15:31:16 +0200 Subject: ftp: Maybe handle unprocessed chunk message before acking final chunk --- lib/ftp/src/ftp.erl | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/ftp/src/ftp.erl b/lib/ftp/src/ftp.erl index 92d2119945..3941e695f2 100644 --- a/lib/ftp/src/ftp.erl +++ b/lib/ftp/src/ftp.erl @@ -1143,7 +1143,7 @@ handle_call({_, recv_chunk}, From, #state{chunk = true, } = R } = State0) -> State = activate_data_connection(State0), - {reply, ok, State#state{client = From, caller = R#recv_chunk_closing{client_called_us=true}}}; + {noreply, State#state{client = From, caller = R#recv_chunk_closing{client_called_us=true}}}; handle_call({_, recv_chunk}, From, #state{chunk = true, caller = #recv_chunk_closing{} = R @@ -1302,6 +1302,13 @@ handle_info({Cls, Socket}, #state{dsock = {Trpt,Socket}, caller = #recv_chunk_closing{dconn_closed = true, client_called_us = Client =/= undefined} }}; +handle_info({Cls, Socket}, #state{dsock = {Trpt,Socket}, + caller = #recv_chunk_closing{client_called_us = true, + pos_compl_received = true} = R} = State) + when {Cls,Trpt}=={tcp_closed,tcp} ; {Cls,Trpt}=={ssl_closed,ssl} -> + %% Maybe handle unprocessed chunk message before acking final chunk + self() ! {Cls, Socket}, + {noreply, State#state{caller = R#recv_chunk_closing{dconn_closed = true}}}; handle_info({Cls, Socket}, #state{dsock = {Trpt,Socket}, caller = recv_bin, data = Data} = State0) -- cgit v1.2.1 From 7662aa4e2ef041f7b2896b8a85cf77c2ad549abd Mon Sep 17 00:00:00 2001 From: Ingela Anderton Andin Date: Mon, 19 Oct 2020 12:40:20 +0200 Subject: ftp: Deliver already recived data --- lib/ftp/src/ftp.erl | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/ftp/src/ftp.erl b/lib/ftp/src/ftp.erl index 3941e695f2..53e9976c96 100644 --- a/lib/ftp/src/ftp.erl +++ b/lib/ftp/src/ftp.erl @@ -1097,7 +1097,6 @@ handle_call({_,{type, Type}}, From, #state{chunk = false} = State0) -> _ -> {reply, {error, etype}, State0} end; - handle_call({_,{recv, RemoteFile, LocalFile}}, From, #state{chunk = false, ldir = LocalDir} = State) -> progress_report({remote_file, RemoteFile}, State), @@ -1112,12 +1111,10 @@ handle_call({_,{recv, RemoteFile, LocalFile}}, From, {error, _What} -> {reply, {error, epath}, State} end; - handle_call({_, {recv_bin, RemoteFile}}, From, #state{chunk = false} = State) -> setup_data_connection(State#state{caller = {recv_bin, RemoteFile}, client = From}); - handle_call({_,{recv_chunk_start, RemoteFile}}, From, #state{chunk = false} = State) -> setup_data_connection(State#state{caller = {start_chunk_transfer, @@ -1126,7 +1123,21 @@ handle_call({_,{recv_chunk_start, RemoteFile}}, From, #state{chunk = false} handle_call({_, recv_chunk}, _, #state{chunk = false} = State) -> {reply, {error, "ftp:recv_chunk_start/2 not called"}, State}; - +handle_call({_, recv_chunk}, _From, #state{chunk = true, + data = Bin, + caller = #recv_chunk_closing{dconn_closed = true, + pos_compl_received = true, + client_called_us = true + } + } = State0) -> + case Bin of + <<>> -> + {reply, ok, State0#state{caller = undefined, + chunk = false, + client = undefined}}; + Data -> + {reply, Data, State0} + end; handle_call({_, recv_chunk}, _From, #state{chunk = true, caller = #recv_chunk_closing{dconn_closed = true, pos_compl_received = true -- cgit v1.2.1