On 08/08/16 21:25, Gustavo Sverzut Barbieri wrote: > Hi all, > > Now it's time to review Ecore_Con_Url towards a better API and then > provide that as a Eo object. The current ecore_con_url will stay the > same, but the new one can have some enhancements or simplifications if > needed. > > Before going into the review, the core question will be to have a > single core class with all methods even if they do not make sense (ie: > HTTP-specific) or multiple subclasses with protocol specific methods. > You can see methods split below. > > A second point is whether to allow reuse of Ecore_Con_URL handle to do > other requests. Reviewing the current code and cURL documentation I > found that we may enter inconsistent behavior, for example if a POST > is done then HEAD, it will need to reset some flags and we're not > doing that. To me it looks like create for a single operation and then > destroy, creating a new one if desired is the common pattern.
That is not a decision that should be made based on the backend and what it supports, but rather based on what we want to support. It's easy enough to just destroy the curl handle and create a new one if someone wants to reuse the handle. I for one think a new object should be created per request anyway, so there is no problem. > > Review: > > * Eo.Base change to Eo.Loop_User > * data event: payload: conn (?), size, data. Remove conn, since the > event callback provides it. > * complete event: payload: conn (?), code (200, 404...). Remove conn, > since the event callback provides it. > * progress event: payload: conn (?), down/up x total/now (double?). > Remove conn, since the event callback provides it. Change from double > to off_t or uint64_t? > * `ecore_con_url_pipeline_set(bool)/get` only usable with http, so > add that to name? HTTP in comparison to what? > > # Base > > Should change: > * `ecore_con_url_get(con)` execute a regular fetch (HTTP: GET, FTP: > download) request. GET is confusing since ressembles HTTP-only, maybe > rename to 'fetch'? > * `ecore_con_url_head(con)` execute body-less fetch (HTTP: HEAD, FTP: > no data) request. HEAD also ressembles HTTP-only, maybe rename to > 'fetch_no_body' or something like that? Suggestions? The problem I have with these two is that yes, head/get are confusing in the signature, but they are also the correct standard names. ecore_con_url_action_get would maybe be better suiting, but that's, as you said an HTTPism. I'm not sure what's the correct way of doing it. Maybe it should be: ecore_con_url_request_type_set(con, GET); ecore_cun_url_request/execute(...); ? I think that's cleaner and kind solves the whole HTTPism with FTP, no? It also means that you can defer the "action" to later without having to keep information about the type of action. The action type is saved on the object. > * `ecore_con_url_received_bytes_get(con): int` **(issue: progress is > double! change to uint64_t/off_t?)** > * `ecore_con_url_proxy_set(con, proxy)`, take user and password to > avoid 3 functions? Maybe ask them to be url-encoded such as > http://user:pass@domain/path and then use a single string? > * `ecore_con_url_proxy_username_set(con, username)` > * `ecore_con_url_proxy_password_set(con, password)` > * `ecore_con_url_url_set(con, url)` remove? > > Okay: > > * `ecore_con_url_new(url): con` > * `ecore_con_url_custom_new(url, http_method)` (works for HTTP, FTP, > IMAP, POP3, SMTP...) > * `ecore_con_url_free(con)` > * `ecore_con_url_fd_set(con, write_fd)` download to file (http/ftp > download, etc) > * `ecore_con_url_verbose_set(con, bool)` set cURL verbosity > * `ecore_con_url_data_set(con, void*)` -> `eo_key_data_set()` > * `ecore_con_url_data_get(con)` -> `eo_key_data_get()` > * `ecore_con_url_timeout_set(con, double)` > * `ecore_con_url_status_code_get(con): int` > * `ecore_con_url_response_headers_get(con): list` > * `ecore_con_url_ssl_verify_peer_set(con, bool)` > * `ecore_con_url_ssl_ca_set(con, file)` Actually, all of these feel bad. Especially the first 4. They should die. As for cURL verbosity: also bad, I'd rather we hook it to the EINA LOG mechanism, and depending on the level of verbosity detected, set the curl verbosity. We are essentially exposing implementation details here, which is very bad. Response headers: should it be a list or iterator? We kinda switched to iterators all around. I'm not sure... Not sure about the rest, bu the above are my main comments. -- Tom. > > To be introduced: > > * `ecore_con_url_download_bytes_get(con): uint64` to complement > `ecore_con_url_received_bytes_get()` > * `ecore_con_url_authentication_set(con, user, pass)` general > authentication data (replaces ftp_upload() and httpauth_set()). > * `ecore_con_url_upload_file(con, src_fname)` works for most > protocols such http and ftp, replaces ecore_con_url_ftp_upload(). > * `ecore_con_url_upload_data(con, cb)` to upload data created by a > given function, allows streaming and pipe from other sources, not just > files. > > > # HTTP specific: > > * `ecore_con_url_post(con, data, len, content_type)` execute POST request > * `ecore_con_url_http_version_set(con, ver)` set http version to use > **(issue: add http2, etc)** > * `ecore_con_url_additional_header_add(con, key, val)` add Key: Value to > header > * `ecore_con_url_additional_headers_clear(con)` remove all headers > * `ecore_con_url_httpauth_set(con, user, pass, safe): bool` safe > blocks plain-text authentications such as HTTP Basic. **Should be > replaced with ecore_con_url_authentication_set() + > ecore_con_url_http_authentication_method_set()** > * `ecore_con_url_cookies_init(con)` > * `ecore_con_url_cookies_ignore_old_session_set(con, url, bool)` > * `ecore_con_url_cookies_clear(con)` > * `ecore_con_url_cookies_session_clear(con)` > * `ecore_con_url_cookies_file_add(con, file)` > * `ecore_con_url_cookies_jar_file_set(con, file)` > * `ecore_con_url_cookies_jar_write(con)` > > To be introduced: > * `ecore_con_url_http_authentication_method_set(con, method)` (basic, > digest, digest-ie, negotiate, ntlm, ntlm_wb, any, any safe, only) > * `ecore_con_url_http_allow_redirects_set(con, bool)` to set > CURLOPT_FOLLOWLOCATION and allow HTTP 3xx redirects. > > > # FTP specific: > > * `ecore_con_url_ftp_upload(con, src_fname, user, pass, upload_dir)` > execute ftp upload. Could be removed if `ecore_con_url_upload_file()` > is added. > * `ecore_con_url_ftp_use_epsv_set(con, epsv)`: just examples use > this, should be removed? > > > > For further reference, you can find an analysis of users of > ecore-con-url at > https://gist.github.com/barbieri/773440bfc747d902b34b33dbbe89bd1a > ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. http://sdm.link/zohodev2dev _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
