On Mon, 1 Aug 2016 11:25:35 -0700 Cedric BAIL <cedric.b...@free.fr> said:
> Hello, > > On Sun, Jul 31, 2016 at 8:50 PM, Gustavo Sverzut Barbieri > <barbi...@gmail.com> wrote: > > 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. > > The old long emails are back ! I am sure that the all old geezer > around are going to enjoy it :-D > > > /* see http://solettaproject.github.io/docs/c-api/structsol__blob.html > > * > > * The idea is to allow an immutable memory to be reference counted and have > > * parent. > > * > > * Children reference their parent and releases the referece when they > > * die, this allows "windowing" or "slicing" of memory to be handled > > * easily, such as a subsurface keeping the parent framebuffer alive, > > * or a "line" keeping the backing mmap'ed file. > > * > > * Free function allows custom release, such as unloading an image or > > * munmap of a text file. > > * > > * This plays well with bindings, keep the object alive (inc ref) and > > * once the blob dies release them (dec ref). > > */ > > struct Eina.Blob { > > mem: void_ptr; > > parent: Eina.Blob; > > size: size; > > refcount: int; > > free: Eina_Blob_Free_Cb; > > } > > I really don't like the introduction of that type. Eina_Binbuf already > support static/read only content. See below for why. > > > /* > > * Using 'Net' instead of 'Network' since all others use a shorter > > * prefix. Also allows to have a side-by-side implementation without > > * the need to immediately convert the all users and allows changing > > * paradigm of the new class -- of course legacy wrappers will be kept > > * for old users. > > */ > > I am personnally against the shortening of our namespace and naming in > general. Good proper text editor will do the autocomplete efficiently > enough and it bring a general rules that is less confusing to user. > Are we going to go with Can insstead of Canvas ? And so on and so > forth. i disagree. Net is like one of the most universal terms meaning network. it's not a shortening by us. it's the entire world that does this everywhere. it's so common it's used in daily speech by average people like "i'm going to check on the net for that". > > abstract Efl.Net.BaseSocket (Eo.Base) { > > Pretty sure this one should be actually an Efl.Loop_User. The idea of > an Efl.Loop_User is that when you set a parent, that parent should be > able to return an EFL_LOOP_CLASS object when requested with > eo_provider_find. This will enable through reparent to attach to > different main loop and just reconfigure the fd handler. You can check > Efl.Loop.Fd for more information on this, but ideally when there is a > reparent on the base socket, all child loop.fd should reparent > themself (Or I should add a watcher on them to detect that there is a > change of main loop requested, anyway that will come later when we > introduce support for multi loop). > > > [[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]] > > error: int; [[errno]] > > } > > I am not to sure about this events, see below. > > > methods { > > send @virtual_pure { > > [[Queue data to be sent to remote]] > > params { > > @in data: Eina.Blob; [[data to queue for sending]] > > @out error: int; [[0 on success, ENOSPC if no space left]] > > } > > > > /* can provide a helper that sends memory, creating the > > blob internally, such as static-const (no free) or > > allocated memory (call free) > > */ > > } > > I would clearly prefer a send that take an Eina.Binbuf and return a > future/promise when the transaction is done. This would enable the > possibility to be notified of progress also. I am just wondering what > would be the useful value to return. Time when the operation was done > ? The failed case would just expose an Eina_Error that properly > describe the problem. It will make the code look a lot more > synchronous as we can just chain them. Not as nice as go, but closer I > think. no. no promises. why? because ALL we can guarantee is we submitted it to the kernel buffer. no more. it may or may not get sent or even arrive at the other end. the promise really provides no value here. no - no promise. just have an internal list/array of binbufs and submit them in order until kernel says "enough!" and keep submitting whenever kernel becomes available. in fact we don't even need to do this on mainloop. it can be done by a network slave thread to keep the mainloop uninterrupted. :) actually replace mainloop here with the owning loop of the network object. it could be any loop really. :) for now it'll be mainloop though. so no promises as they MEAN nothing when it comes to network. they provide no guarantees. i DO think they have a place higher up at an ipc protocol layer or with http etc. but not raw tcp/udp/unix sockets. > Also if we go with a future, should we also propose a read function > that return a future/promise to an Eina_Binbuf ? This would be nice if > we want to implement timeout on a read or chain it with something > else. > > Also possible additional operation would be sending a file. Eina_File > should be a native type to any binding and we do have a few place > where you get one, would be nice to do be able to transparently send > one. > > > 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]] > > } > > } > > Hum, what is the purpose of flush in that case ? > > > @property local_address { > > <snip> > > > @property remote_address { > > <snip> > > > @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 should not be exposed for the moment. I see a lot of wrong doing > possible. It will be better to had property that enable possible > fcntl. agree. > > @property connected { > > [[if the socket is still connected.]] > > get @virtual_pure { > > } > > values { > > connected: bool; > > } > > } > > > > @property timeout { /* this usually doesn't change, > > should be constructor-only */ > > [[timeout to close the connection.]] > > values { > > timeout: double; > > } > > } > > What does this timeout cover ? connection ? transmission ? both ? that's why i thought there might be a resolve timeout, a connect timeout and a send timeout. if the kernel refuses to accept more buffers for > n time something is stalled in sending. this si a bit different to a connect and same with resolve. i would expect resolve timeouts to be shorter than connect > > @property uptime { /* is this really required? > > currently unused in our code! > > Could be done outside with ease */ > > If it is unused, let's not put it back in for the moment. > > > 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; > > } > > } > > Maybe it should be a property on a connection manager that return > future<Efl.Network.Socket> ? Something like future<Efl.Network.Socket> > Efl.Network.Manager.connect(Efl.Network.Socket.Type, > Efl.Network,SocketAddress). But that would work only if we have no > other parameter to set. If this is just a property that will trigger a > connection on finalize, then we have a lot of possible flags to handle > maybe. > > > } > > > > constructors { > > .connect; > > Hum, didn't we abandon custom constructor ? I kind of forgot where we > are on this, but I remember that the idea was to rely just on > finalize. yeah. here though i even see it being explicit with a connect() method on the network interface. > > 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 */ > > Should we plan to automatically call eo_del on disconnection and > people are just supposed to listen on the del event actually ? > > > 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 */ > > } > > } > > Basically same as for the one of Efl.Network.Socket. > > > @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). > > */ > > Maybe proposing an iterator instead, but then how about when you need > to split the processing in smaller step ? I do agree with you, this is > a tricky one. For now, let's just expose a counter. i said the same. iterator :) > Nice review of all of the other toolkit. I do like the go one a lot, > but that isn't possible in C. Also one of the thing I would like to > see in the long term is that with the support for multiple main loop > we get the ability to send a socket to another thread and manage to > scale better. > -- > Cedric BAIL > > ------------------------------------------------------------------------------ > _______________________________________________ > enlightenment-devel mailing list > enlightenment-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ 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 enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
