[PATCH 2/2] proto, server: Add internal server error message.

[Pekka Paalanen]

On Tue, 20 Feb 2018 18:07:27 +1100
raof at ubuntu.com wrote:

> From: Christopher James Halse Rogers <christopher.halse.rogers at canonical.com>
> 
> Many languages such as C++ or Rust have an unwinding error-reporting
> mechanism. Code in these languages can (and must!) wrap request handling
> callbacks in unwind guards to avoid undefined behaviour.
> 
> As a consequence such code will detect internal server errors, but have
> no way to communicate such failures to the client.
> 
> This adds a WL_DISPLAY_ERROR_INTERNAL error to wl_display so that
> such code can notify (and disconnect) client which hit internal bugs.
> 
> Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers at canonical.com>
> ---
>  protocol/wayland.xml      |  2 ++
>  src/wayland-client.c      |  3 +++
>  src/wayland-server-core.h |  4 ++++
>  src/wayland-server.c      | 23 +++++++++++++++++++++++
>  tests/display-test.c      | 40 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 72 insertions(+)
> 
> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> index b5662e0..1db31a6 100644
> --- a/protocol/wayland.xml
> +++ b/protocol/wayland.xml
> @@ -94,6 +94,8 @@
>  	     summary="method doesn't exist on the specified interface"/>
>        <entry name="no_memory" value="2"
>  	     summary="server is out of memory"/>
> +      <entry name="internal" value="3"
> +	     summary="internal server error"/>
>      </enum>
>  
>      <event name="delete_id">
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index c1369b8..7c442b1 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -192,6 +192,9 @@ display_protocol_error(struct wl_display *display, uint32_t code,
>  		case WL_DISPLAY_ERROR_NO_MEMORY:
>  			err = ENOMEM;
>  			break;
> +		case WL_DISPLAY_ERROR_INTERNAL:
> +			err = EPROTO;
> +			break;
>  		default:
>  			err = EFAULT;
>  		}
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index 2e725d9..7137da6 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -324,6 +324,10 @@ wl_client_get_object(struct wl_client *client, uint32_t id);
>  void
>  wl_client_post_no_memory(struct wl_client *client);
>  
> +void
> +wl_client_post_internal_error(struct wl_client *client,
> +			      const char* msg, ...) WL_PRINTF(2,3);
> +
>  void
>  wl_client_add_resource_created_listener(struct wl_client *client,
>                                          struct wl_listener *listener);
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 00c93f7..6317f8f 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -650,6 +650,29 @@ wl_client_post_no_memory(struct wl_client *client)
>  			       WL_DISPLAY_ERROR_NO_MEMORY, "no memory");
>  }
>  
> +/** Report an internal server error
> + *
> + * \param client The client object
> + * \param msg A printf-style format string
> + * \param ... Format string arguments
> + *
> + * Report an unspecified internal error and disconnect the client.
> + *
> + * \memberof wl_client
> + */
> +WL_EXPORT void
> +wl_client_post_internal_error(struct wl_client *client,
> +			      char const *msg, ...)
> +{
> +	va_list ap;
> +
> +	va_start(ap, msg);
> +	wl_resource_post_error_vargs(client->display_resource,
> +				     WL_DISPLAY_ERROR_INTERNAL,
> +				     msg, ap);
> +	va_end(ap);
> +}
> +

Hi,

the explanation here is more explicit than what we discussed in IRC,
and both together give a nice justifiction for adding this. I'm for it,
and I am very happy to see the tests extended as well.

What we discussed in IRC was around why add a new error code instead of
abusing the existing wl_display error codes (it is possible to send
wl_display errors explicitly with custom server code). The outcome of
that was that it would be useful for applications to tell the
difference between their fault and server's fault, to guide automated
bug reporting. This would be nice to add to the commit message.

The one thing I'm wondering is, is "internal" obvious enough to say
it's a server internal error and not a (lib)Wayland internal error?

I've seen Wayland blamed so wildly for things it has nothing to do with
that I might prefer something more explicit, like "implementation"
mentioned by Daniel in IRC. OTOH, what user would see is probably only
the numerical value of the error with the message, so maybe it's
unlikely to be confused.

Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180221/14bbe1f3/attachment.sig>