On 09/08/16 16:24, Gustavo Sverzut Barbieri wrote: > 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...
No, that's bad too. I'm just saying I think we should come up with naming. Set the type separately in a function, and then just have a function that "executes". Actually though, and this is an interesting question, I'd expect reuse in POP3. POP3 feels like a protocol where the connection would be reused, no? Maybe I'm just not familiar enough. Same with IMAP. It feels more of a few roundtrip on the same resource kind of protocol (in comparison to HTTP). > > >> 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. Yeah, I'm not sure. It feels like good separation, but I can't really know. > > >>> 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. I meant, new/free should die now in eo land. The constructors: we don't construct like that anymore. We just set the http_method in eo_add(). By the first 4 are bad, I meant: they have no place in eo world, so I found it odd you put them there. > > 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 :-) > I meant the "hack" part. It just feels like a super hack in all senses. > > >> 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. > > > ------------------------------------------------------------------------------ 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
