Re: [PATCH 2/2] dpwsockx: Implementation of Open callback

[Vitaliy Margolen]

Ismael Barros wrote:
> +static DWORD WINAPI udp_listener_thread( LPVOID lpParameter )
> +{
> - for ( ;; )
> - {

.....
> - }
>
> +}

You do not have exit from the loop.
> - /* Launch thread */
> - listener->handle = CreateThread( NULL, 0, udp_listener_thread,
> -
>                                 listener, 0, NULL );

I haven't found the place where you closing this handle.
> - /* Launch thread */
> - listener->handle = CreateThread( NULL, 0,
> -
>                                 ( is_tcp
> -
>                                   ? tcp_listener_thread
> -
>                                   : udp_listener_thread ),
> -
>                                 listener, 0, NULL );

Same here.

Vitaliy.

[PATCH 2/2] dpwsockx: Implementation of Open callback

[Vitaliy Margolen]

Forgot this part:
> Ismael Barros wrote:
> +static DWORD WINAPI tcp_listener_thread( LPVOID lpParameter )
> +{
> - for ( ;; )
> - {
> -
>    if ( clientSock == INVALID_SOCKET )
> -
>    {
> -
>        goto end;
> -
>    }
> - }
> -
>
> +end:
> +}

It's more cleaner to use "break;" instead of goto to exit the loop.

Vitaliy.

[PATCH 2/2] dpwsockx: Implementation of Open callback

[Ismael Barros²]

On Sun, Sep 13, 2009 at 11:46 PM, Vitaliy Margolen
 wrote:
> Forgot this part:
> > Ismael Barros wrote:
> > +static DWORD WINAPI tcp_listener_thread( LPVOID lpParameter )
> > +{
> > - for ( ;; )
> > - {
> > - if ( clientSock == INVALID_SOCKET )
> > - {
> > - goto end;
> > - }
> > - }
> > -
> >
> > +end:
> > +}
>
> It's more cleaner to use "break;" instead of goto to exit the loop.

It's a way to tell between abruptly ending the thread due to an error
(so it just executes cleanup code and returns), and ending the thread
gracefully (which would be done with a break or a thread termination
boolean). In this case is equivalent because there's no code between
the end of the loop and the "end" label.

If it's better to use always a break I will change it.

[PATCH 2/2] dpwsockx: Implementation of Open callback

[Vitaliy Margolen]

Ismael Barros² wrote:
> On Sun, Sep 13, 2009 at 11:46 PM, Vitaliy Margolen
>  wrote:
> > Forgot this part:
> > > Ismael Barros wrote:
> > > +static DWORD WINAPI tcp_listener_thread( LPVOID lpParameter )
> > > +{
> > > - for ( ;; )
> > > - {
> > > -
> > >    if ( clientSock == INVALID_SOCKET )
> > > -
> > >    {
> > > -
> > >        goto end;
> > > -
> > >    }
> > > - }
> > > -
> > >
> > > +end:
> > > +}
> >
> > It's more cleaner to use "break;" instead of goto to exit the loop.
>
> It's a way to tell between abruptly ending the thread due to an error,
> and ending the thread gracefully.

That makes no sense to me. You have no other point of exit from the loop.
Using goto for no good reason is always a bad thing.

Vitaliy.

[PATCH 2/2] dpwsockx: Implementation of Open callback

[Ismael Barros²]

On Mon, Sep 14, 2009 at 12:46 AM, Vitaliy Margolen
 wrote:
> > It's a way to tell between abruptly ending the thread due to an error,
> > and ending the thread gracefully.
>
> That makes no sense to me. You have no other point of exit from the loop.
> Using goto for no good reason is always a bad thing.
>
> Vitaliy.

Actually I'm considering dropping that code and using nonblocking
sockets with WSARecv/From() and a callback, which would make the code
much simpler, as I wouldn't have to manage my own threads.

However the blocking approach (recv/from()) seems to be more popular
in the codebase; is there any drawback I'm not aware of, or using
nonblocking sockets is okay?

[PATCH 2/2] dpwsockx: Implementation of Open callback

[James McKenzie]

Vitaliy Margolen wrote:
> Forgot this part:
> > Ismael Barros wrote:
> > +static DWORD WINAPI tcp_listener_thread( LPVOID lpParameter )
> > +{
> > - for ( ;; )
> > - {
> > -
> >    if ( clientSock == INVALID_SOCKET )
> > -
> >    {
> > -
> >        goto end;
> > -
> >    }
> > - }
> > -
> >
> > +end:
> > +}
>
> It's more cleaner to use "break;" instead of goto to exit the loop.

That is some ugly code there.

Ismael:  Please use a break; to get out of the loop.  It is cleaner and
good code practice.

James McKenzie

[PATCH 2/2] dpwsockx: Implementation of Open callback

[Ismael Barros²]

On Sun, Sep 13, 2009 at 11:39 PM, Vitaliy Margolen
 wrote:
> You do not have exit from the loop.
>
> I haven't found the place where you closing this handle.
>
> Same here.

The code to close those threads and their listener sockets is meant to
go in DPWSCB_CloseEx(), but I still haven't worked on it. But you are
right, I should already work on the code to gracefully tear the
listeners down.

[PATCH 2/2] dpwsockx: Implementation of Open callback

[Vitaliy Margolen]

Ismael Barros² wrote:
> On Sun, Sep 13, 2009 at 11:39 PM, Vitaliy Margolen
>  wrote:
> > You do not have exit from the loop.
> >
> > I haven't found the place where you closing this handle.
> >
> > Same here.
>
> The code to close those threads and their listener sockets is meant to
> go in DPWSCB_CloseEx(),

Then it should be in the same patch. I guess it might have been ok if you've
sent it as the 3rd patch in the series but you didn't.

Vitaliy.