Hi Daniel, thanks for your review. I will drop the storing and forwarding of the errno code.
Regards, Mathias ________________________________________ From: Daniel Stone <[email protected]> Sent: Monday, December 4, 2017 10:06 PM To: Fiedler, Mathias Cc: [email protected] 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, <[email protected]> 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 _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
