[E-devel] Re: Re: Re: E CVS: apps/entrance xcomputerman

[Corey D. <at...@at...>]

* Ibukun Olumuyiwa (ib...@co...) wrote:
> On Thu 25 Mar 2004, Corey Donohoe wrote:
> > * Ibukun Olumuyiwa (ib...@co...) wrote:
> > > On Thu 25 Mar 2004, Corey Donohoe wrote:
> > > > All of these diff's looked pretty good, but ... :)
> > > > 
> > > > You don't ever seem to close the connection when you successfully
> > > > authenticate, so the removed entrance_ipc_shutdown() in
> > > > entrance_session.c should probably go back in.  
> > > 
> > > Ah, right...that shutdown doesn't happen. Will fix.
> > Would it be better to return the pointer to the server, and keep it
> > encapsulated in the Entrance_Session?  That way we can shutdown the ipc
> > when we cleanup the session.  hmm...
> 
> I'm not sure putting ipc data inside of the session is a good idea. That
> could lead to complications later down the road, possibly with Xdmcp and
> any other communication issues that may need to be resolved outside the
> context of the session.

If entrance always atleast has an Entrance_Session in memory, we COULD
nest the initialization and shutdown of the ipc system into the new/free
calls to entrance_session.  I admittedly have nfc about Xdmcp, and
what's necessary there tho.  Entrance w/o an Entrance_Session struct
doesn't go very far, unless a lot of the data flow gets gutted.  Even in
that case, the callbacks that require the _session pointer
(_entrance_ipc_server_data) won't work anyways.

> 
> > > 
> > > > What's the purpose of the static _session variable that was added in
> > > > entrance_ipc.c ?  Would it be better handled as the data variables to
> > > > the different IPC_EVENT types ?  I'm assuming this is just something
> > > > that isn't being used yet, but probably is there for checking things
> > > > when the callbacks roll in.
> > > >     
> > > > ecore_event_handler_add(ECORE_IPC_EVENT .., _entrance_ipc..., session);
> > > > 
> > > 
> > > At the time of entrance_ipc_init(), the session has not yet been
> > > initialized ... can't pass an empty pointer. So I have to manually set it
> > > afterwards using ipc_session_set(). This approach is still OK since we're
> > > using a static variable. That approach was what I wanted to use at first.
> > How about keeping the session_set function, and assigning the callbacks
> > at that point.  Or have the entrance_ipc_init() function also take an
> > Entrance_Session parameter?  See the attached diff.
> > 
> 
> Nice idea, but I'm not convinced it's the best way to go for the reason
> outlined above ... in other words, I'm not sure the ipc code should be
> confined to the session. Think of IPC as a separate module or object that
> other modules in entrance (session, auth) can use to communicate with
> entranced. When you look at it from a pseudo-object-oriented standpoint,
> session-related data should be modified using a publicly available method
> (i.e. ipc_session_set()). The use of a static var emphasises the fact that
> it is data local to the ipc module/object.

Well I wouldn't really call it data that's local to the ipc module,
since it's created/destroyed elsewhere. Its (planned) use at certain
times(callbacks) makes sense to just feed it as data to those callbacks.

The Entrance_Session struct that we allocate is always present, and
there is only one per entrance process(singleton sorta).  I don't
see any reason to duplicate that pointer in another place and keep it
static.  I understand the subsystem information hiding-esque concept, I
just don't think it makes sense here. 

Entrance_Session is a handle to all of the memory we've allocated in
entrance, you can basically get to anything with it.  This is true
except in the case of the ipc subsystem.  From a fresh co
(This isn't the perfect way to grep this, but it'll hopefully make sense)

atmos@neige:~/Source/e17/apps/entrance/src/client$ grep static *.[ch] | grep = 
entrance_ipc.c:static Ecore_Ipc_Server *server = NULL;
entrance_ipc.c:static Entrance_Session *_session = NULL;
main.c:static Entrance_Session *session = NULL;
... + two lines from entrance_edit.c ...

callbacks with extra data parameters are tastey, we should use them. 
I can patch in the above mentioned ideas.
I must seem like a raving idiot here, well... :)

__
Corey Donohoe
atmos.org