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
<wine-devel(a)kievinfo.com> 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
> <wine-devel(a)kievinfo.com> 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
<wine-devel(a)kievinfo.com> 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
<wine-devel(a)kievinfo.com> 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
> <wine-devel(a)kievinfo.com> 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.