[PATCH 2/2] server: add log message when client connection is destroyed due to an error

[Fiedler, Mathias]

Hi Daniel,

thanks for your review. I will drop the storing and forwarding of the errno code.

Regards,
Mathias
________________________________________
From: Daniel Stone <daniel at fooishbar.org>
Sent: Monday, December 4, 2017 10:06 PM
To: Fiedler, Mathias
Cc: wayland-devel at lists.freedesktop.org
Subject: Re: [PATCH 2/2] server: add log message when client connection is destroyed due to an error

Hi Mathias,
Thanks for your patch! The idea seems fine, but I have a few comments.

On 8 June 2017 at 08:39,  <mathias_fiedler at mentor.com> wrote:
> @@ -313,16 +326,30 @@ wl_client_connection_data(int fd, uint32_t mask, void *data)
>         uint32_t resource_flags;
>         int opcode, size, since;
>         int len;
> +       int err;
> +       socklen_t errlen;
>
> -       if (mask & (WL_EVENT_ERROR | WL_EVENT_HANGUP)) {
> +       if (mask & WL_EVENT_HANGUP) {
>                 wl_client_destroy(client);
>                 return 1;
>         }
>
> +       if (mask & WL_EVENT_ERROR) {
> +               // query socket error
> +               errlen = sizeof(err);
> +               if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &errlen)) {
> +                       // when getsockopt fails use its error instead
> +                       err = errno;

I don't believe that errno is necessarily related in this case, so we
can't pass it to the client.

> @@ -334,7 +361,8 @@ wl_client_connection_data(int fd, uint32_t mask, void *data)
>         if (mask & WL_EVENT_READABLE) {
>                 len = wl_connection_read(connection);
>                 if (len == 0 || (len < 0 && errno != EAGAIN)) {
> -                       wl_client_destroy(client);
> +                       destroy_client_with_error(
> +                           client, "failed to read client connection", errno);

If (len == 0), errno may be irrelevant.

The same is true in your first patch: I believe there are a few places
where we might leak errno to the client when it is not actually
relevant. It would be nice to change the 'if (client->error)' checks
in that patch to (client->error != 0) as well, just to be more
explicit.

Also, your patch uses // C++ comments like this.

Please use /* Old-style C comments */ instead, but whilst you're at
it, comments like 'query socket error' are obvious enough from reading
the surrounding context that they are not really required.

Cheers,
Daniel