On Tue, Aug 9, 2016 at 9:21 AM, Tom Hacohen <[email protected]> wrote:
> 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.
sure, what I wanted to point is that: either we force the do-not-reuse
pattern or we fix the issue if someone tries to reuse. Currently it's
broken (did not test, but looks so reading the docs).
> I for one think a new object should be
> created per request anyway, so there is no problem.
me too :-)
>> * `ecore_con_url_pipeline_set(bool)/get` only usable with http, so
>> add that to name?
>
> HTTP in comparison to what?
instead of:
ecore_con_url_pipeline_set(bool)
use
ecore_con_url_http_pipeline_set(bool)
(actually, with the new-prefix, so efl_net_url_http_pipeline_set(bool)).
>> # 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);
this still looks too much HTTP-ism. cURL supports POP3, there "LIST"
would make more sense. Do we expect people to provide the correct
string for each protocol? I find that also strange...
> 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.
not sure it will help much the usability, since the whole creation of
the request is based on some premises, like post() will need data...
get cannot have data. Then the action is to be known.
>> 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.
please be more specific. The first 2 are constructors, granted the
"custom_new()" is not used outside of tests and from the cURL docs
(https://curl.haxx.se/libcurl/c/CURLOPT_CUSTOMREQUEST.html) is not
that useful in real life, thus could go.
but what's the problem with new()/free()? They are pre-Eo, so needs that.
fd_set() is indeed some hack, an optimization to write to a file
(common case). If we have the "Efl.Loop_User" extra methods I've
mentioned in my other thread about ecore_con, it would be much more
reasonable to give that Eo object here instead of a simple fd. But
that's okay in concept, you make a common case easy to use.
OTOH, if you mean the name is misleading, as fd_set() doesn't imply
it's a writer or reader... then okay, it's bad :-)
> 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.
you're right, I believe this can be done by checking eina log level at
ecore_con_url_init() and setting the curl verbosity when the curlm is
created. Actually it could also set the log function, so we get all
logs via eina_log (both operations are needed, as we do not want curl
to produce logs if they wouldn't be displayed).
> Response headers: should it be a list or iterator? We kinda switched to
> iterators all around. I'm not sure...
iterators look good to me, the current API was written before that,
thus the list.
--
Gustavo Sverzut Barbieri
--------------------------------------
Mobile: +55 (16) 99354-9890
------------------------------------------------------------------------------
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
