[vlc-devel] [vlc-commits] Add lua console handling code for windows.

[Antoine Cellerier]

Hi, 

Sorry for the late answer.

On Thu, Feb 09, 2012, Pierre Ynard wrote:
> > --- a/modules/lua/Modules.am
> > +++ b/modules/lua/Modules.am
> > @@ -37,4 +37,8 @@ SOURCES_lua = \
> >  	libs/xml.c \
> >  	$(NULL)
> >  
> > +if HAVE_WIN32
> > +SOURCES_lua += libs/win.c
> > +endif
> 
> This won't add the file on WinCE...
> 
> >              luaopen_vlm( L );
> >              luaopen_volume( L );
> >              luaopen_xml( L );
> > +#ifdef WIN32
> > +            luaopen_win( L );
> > +#endif
> 
> ... however this would try to call luaopen_win() on WinCE, resulting in
> a build failure.

The buildbot WinCE build seems to be fine.

> > +    int i_timeout = luaL_optint( L, 2, -1 );
> >  
> >      int i_ret;
> >      do
> > -        i_ret = poll( p_fds, i_fds, -1 );
> > +        i_ret = poll( p_fds, i_fds, i_timeout );
> >      while( i_ret == -1 );
> 
> This fails to adjust the timeout if poll() is interrupted. Also exposing
> the timeout in the API favors proliferation or low-quality code. See
> 314c242ab041d515c1b2f1a82ee1a3fe4fe715a5

This was removed.

> > +/* Based on modules/control/rc.c and include/vlc_interface.h */
> 
> I understand for rc.c, but:
> 
> > +    AllocConsole();
> > +
> > +    freopen( "CONOUT$", "w", stdout );
> > +    freopen( "CONOUT$", "w", stderr );
> > +    freopen( "CONIN$", "r", stdin );
> 
> This code is taken from a macro in vlc_interface.h. Couldn't you just
> call the macro?

Probably. I didn't want the messages that were bundled in the macro. I
could have split the macro in two to reuse the code but I have to admit
that having a macro called CONSOLE_INTRO_MSG hide init code doesn't make
it really easy to follow the code.

> > +    fprintf(stdout, "Tadam!\n");
> > +    fflush(stdout);
> 
> What is the flush for?

That was used when debugging. I thought that I'd removed it with the
Tadam message in a subsequent commit. I'll double check and remove it if
it's still there.

> > -        local ret = vlc.net.poll( pollfds )
> > +        local timeout = -1
> > +        if vlc.win and listeners.stdio then
> > +            timeout = 50
> > +        end
> > +        local ret = 0
> > +        if not vlc.win or listeners.tcp then
> > +            ret = vlc.net.poll( pollfds, timeout )
> > +        end
> 
> I'm not sure to understand well, if there are a windows console and TCP
> sockets, then what prevents bogus file descriptors 0 and 1 from still
> being polled along with all the clients?

Agreed. This isn't an issue anymore though as both modes were made
exclusive on Windows by a subsequent commit.

> >      local function destructor( h )
> > -        print "destructor"
> >          for _,client in pairs(clients) do
> > -            --client:send("Shutting down.")
> > +            client:send("Shutting down.")
> 
> This would re-add a duplicate of this message. "Shutting down." is
> already broadcast in the shutdown function. Choose one.

This has been fixed too.

> > -    -- FIXME: this is never called, client sockets are leaked
> > [...]
> > +              { -- metatable
> > +                __gc = destructor,
> > +                __metatable = "",
> > +              })
> 
> This won't work because __gc only works with userdata variables. See
> 6addf026892913a7b2518485bc35d2f9385722d9

This too. Thanks for pointing it out as I was mislead by the Shutting
down. message already being printed.

Cheers,

-- 
Antoine Cellerier
dionoea