On Mon, 1 Aug 2016 00:50:31 -0300 Gustavo Sverzut Barbieri <barbi...@gmail.com> said:
Hey gustoavo! LTNS! :) First... This mail is long. I mean i think you have beaten me totally out of the ballpark with longness of this mail! well done. I must take a bow. I am no longer king of long emails! :) > Hi all, > > I'm entitled to review the new Eo API for Ecore_Con.h. Find below my first > draft proposal and after the "code" you can see my detailed review of > current code and competitors. > > Please reply in line with your points. As the email is super-long, PLEASE > cut non-relevant points and text so we can easily see what you want to mean. > > You may see this as a Gist at > https://gist.github.com/barbieri/5bdf6c49b207543d764435d2494361ba ... > import eina_types; > > struct Efl.Net.Address { > family: Efl.Net.Family; > address: union { in, in6, bt... }; > } are you sure this shouldn't just be a string above for address? and as onefang said - the string can tell us what we need. it can just be a domain name of an ipv4 addr or an ipv6 addr and so on... ? the actual representation like this should be internal and not at any API level. at an API return a stringified thing. set the destination address, get it back as-is with a string name. request a result and once resolve is done (callback or promise) you can get the string resolution of the address. eg ipv4 addr string or ipv6 addr string for example. this just de-complicates API across bindings to not need a union (a bit of an ugly issue) and who really deals with addresses at this format/level? > struct Eina.Blob { > mem: void_ptr; > parent: Eina.Blob; > size: size; > refcount: int; > free: Eina_Blob_Free_Cb; > } this is a big problem for bindings. a big fat "no - find another way". no function pointers in structs or parameters to methods etc. no void *'s (reserved for base eo classes only). use eina_binbuf - a list/array or chain of binbufs or perhaps something else. > abstract Efl.Net.BaseSocket (Eo.Base) { > [[Abstract class representing a network connection socket]] > > events { > read @hot: Eina.Binbuf; [[data is available to read, > if consumed use eina_binbuf_remove().]] > sent: Eina.Blob, error; [[data specified by blob was sent, > if error != 0, contains errno]] binbufs for out too. > error: int; [[errno]] > } > > methods { > send @virtual_pure { > [[Queue data to be sent to remote]] > params { > @in data: Eina.Blob; [[data to queue for sending]] binbuf or something else. as in. > @out error: int; [[0 on success, ENOSPC if no space left]] > } why not return error? why have it as an @out param? why not just return a bool and be done with it? failure here can ONLY mean the inability to store the binbuf in a list internally or the object is totally invalid. either way out of memory or another pretty fatal condition for this connection object. > flush @virtual_pure { > [[Try to send as much as data without blocking]] > /* NOTE: changes behavior from current flush() that > * really blocks! > */ > params { > @out fully_done: bool; [[true if all data was sent]] > } > } return bool value please - much better that @out param. > @property local_address { > [[the local IP or unix-local address]] > get @virtual_pure { > } > values { > address: Efl.Net.Address; > } > } naming: address_local what local address is this exactly? if tis a server i'ts the address being listened on? the ip address (ipv4/6/whatever) of the outgoing interface or something? > @property remote_address { > [[the remote IP or unix-local address]] > get @virtual_pure { > } > values { > address: Efl.Net.Address; > } > } naming: address_remote and remote - this is the address you probably want to SET right so its set {} AND get {}. the get gets what you set. now i mention - what about a address_resolved_remote property you can just get {} only that tells you the resolving of the name or whatever specified by setting the address_remote property. how do we deal with resolving? a promise? an event callback? we have multiple stages here. first a resolve (async) then a connect (async) then finally we are established. > @property internal_fd { > [[the internal file descriptor (use with caution!)]] > /* it seems the usage is to steal the fd to do something > * else or set some flags. If so we could expose some > * primitives such as fcntl_add(type, flags) and > * fcntl_del(type, flags) as well as a destructor that > * will return the stolen fd (destroys the object and > * keeps fd alive). > */ > get @virtual_pure { > } > values { > fd: int; > } > } this has issues possibly on windows. fd's don't quote behave the same way here. can we avoid this? if we need to do deep down ugly things with the fd... can we just support them with the API? like fd passing on local unix sockets. have our api deal with sending and getting fd's as a specific send method and an input event. i get why you have this - as a "way out" for when we don't support something. this is kind of bad for bindings though and portability. :( > @property timeout { /* this usually doesn't change, > should be constructor-only */ > [[timeout to close the connection.]] > values { > timeout: double; > } > } i see no reason that this can't change. a timeout for a connect or a timeout for buffers being able to send ANYTHING (if buffers can't drain for writes for more than N sec - consider this connection dead? should they be the same timer?) > @property uptime { /* is this really required? > currently unused in our code! > Could be done outside with ease */ > [[the time since the connection was established]] > get @virtual_pure { > } > values { > uptime: double; > } > } > } > } i see very little value in the connection tracking it's own uptime at this point. it can be done some time in the future if people really want it. > class Efl.Net.Socket (Efl.Net.BaseSocket) { > [[This is a client socket that is created and connected to a > specific server given to the constructor]] > > methods { > connect { > [[The constructor that connects the socket to a given server]] > params { > @in type: Efl.Net.Socket.Type; > @in address: Efl.Net.SocketAddress; > @in timeout: double; > } > } > } as people mentioned. we could have type implicit in socketaddress with strings. but let's talk constructors. shouldn't we just have connect() take no params and use the currently set up remote address, timeout and other parameters at the time? this works well with eo's finalize constructing mechanisms. these constructors atm are only really useful for c++ even then we'r elooking at using lambdas for construction calling methods. > class Efl.Net.Server (Eo.Base) { > events { > connection @hot: Efl.Net.BaseSocket; [[new connection was accepted]] > error: int; [[errno]] > /* no disconnect, listen on each individual connection */ > } i really don't like error as an int. really don't. a proper enum and/or struct with fields like enum for error type and maybe a string for error extra info (human readable from perror() style?) what about disconnects? you expect peolpe to listen on del events on the connection objects? > methods { > constructor { > params { > /* these could be converted into a struct to scale > * gracefully and allow just some members to be set, > * like only timeout without setting client_limit or > * reject_excess_clients. > */ > @in type: Efl.Net.Socket.Type; > @in address: Efl.Net.SocketAddress; > @in client_limit: int; > @in reject_excess_clients: bool; > @in timeout: double; > @in bind_flags: Efl.Net.Socket.Bind.Flags; /* reuse addr..*/ > @in accept_flags: Efl.Net.Socket.Accept.Flags; /* just > SOCK_CLOEXEC */ > } > } same as the above constructor to connect() - properties with an empty param connect() that as a server is the same as bind+listen(). > @property connections { > [[active connections]] > /* Needed? > * > * actual members or just a counter? It seems all others > * do not keep a list and we should be careful if > * iterating over the list happens to modify it. > * > * count would be useful to gracefully quit if no > * connections, but is easily done elswhere. > * > * the only user in our repo is eeze_scanner.c that uses > * to send a single eet to all connections. Also easily > * done at its own side (actually it already stores the > * clients in a hash, could foreach() that one). > */ > } how about return an eina iterator for connections that will list children. that would help and not define the storage type. i think its good to have this in some form, but best might be an iterator. i'm going to skip the analysis below... and focus on... 1. missing anything to do with SSL. events for certificates, validity, being able to register certs, verify etc. etc. etc. you touch on this in analysis of ecore_con server/client. 2. no way to request a resolve FIRST separately to a connect. this would be good i think (a connect implies a resolve then connect if not already resolved, but allow a stand-alone resolve to then verify results before doing a connect). 3. addresses i think should be just strings. we could then overload this to do things like websockets just by a special address format (ie url) and the details are hidden 4. how do you disconnect a client? del the client object you see in the event info? 5. i think controls on max number of clients would be nice so it auto drops clients without you bothering to listen for the connect cb's then disconnect them by deleting the objects. 6. have you considered freezing or thawing clients so write buffers are frozen (and input buffers are ignored) while frozen? this would be like pause/resume in node.js 7. missing things like tcp keepalive and nodelay. 8. what about udp? 9. no way to GET the current buffer status of a connection you made to a server. e.g. how much data is buffers and not sent yet (not sent to kernel). 10. perhaps add a drain event to know when all write buffers are finished being written to kernel? or maybe sent out of interface to other end? 11. should we consider things like socks proxies - how they work with this? 12. what about api's so do things like fd pass (and receive), do things like get the uid/pid and other process unfo or the other end of the connection of a unix socket? what should we do about things like mqtt, mq/zeromq, ocf/oic - they belong in another interface layer? http is a bit different too due to its nature. websockets i can see fitting underneath the above, but not real http itself -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
