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

[Christopher James Halse Rogers]

On Wed, Feb 21, 2018 at 7:15 PM, Pekka Paalanen <ppaalanen at gmail.com> 
wrote:
> 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.

I'm happy to respin this as wl_client_post_implementation_error; as 
Daniel pointed out, there's a nice consonance with the 
BadImplementation error from X11.

Does anyone want this particular bikeshed painted a different colour?