connection: Print the content of arrays in closures

Submitted by Emmanuel Gil Peyrot on July 10, 2017, 6:28 p.m.

Details

Message ID 20170710182822.13406-1-emmanuel.peyrot@collabora.com
State New
Headers show
Series "connection: Print the content of arrays in closures" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Emmanuel Gil Peyrot July 10, 2017, 6:28 p.m.
The current behaviour when WAYLAND_DEBUG is set is to print “array”,
which is quite unhelpful.

This patch prints a list of the bytes present in the array.  It doesn’t
try to interpret it as uint32_t or anything, leaving that to the reader
because this information isn’t present in the protocol description.

Signed-off-by: Emmanuel Gil Peyrot <emmanuel.peyrot@collabora.com>
---
 src/connection.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/connection.c b/src/connection.c
index 5c3d187..d616ddd 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -1170,11 +1170,13 @@  wl_closure_queue(struct wl_closure *closure, struct wl_connection *connection)
 void
 wl_closure_print(struct wl_closure *closure, struct wl_object *target, int send)
 {
-	int i;
+	int i, j;
 	struct argument_details arg;
 	const char *signature = closure->message->signature;
 	struct timespec tp;
 	unsigned int time;
+	size_t size;
+	char *data;
 
 	clock_gettime(CLOCK_REALTIME, &tp);
 	time = (tp.tv_sec * 1000000L) + (tp.tv_nsec / 1000);
@@ -1223,7 +1225,15 @@  wl_closure_print(struct wl_closure *closure, struct wl_object *target, int send)
 				fprintf(stderr, "nil");
 			break;
 		case 'a':
-			fprintf(stderr, "array");
+			fprintf(stderr, "array(");
+			size = closure->args[i].a->size;
+			data = (char*)closure->args[i].a->data;
+			if (size) {
+				for (j = 0; j < (int)size - 1; j++)
+					fprintf(stderr, "0x%02x, ", data[j]);
+				fprintf(stderr, "0x%02x", data[size - 1]);
+			}
+			fprintf(stderr, ")");
 			break;
 		case 'h':
 			fprintf(stderr, "fd %d", closure->args[i].h);

Comments

Hi,

On 10 July 2017 at 19:28, Emmanuel Gil Peyrot
<emmanuel.peyrot@collabora.com> wrote:
> The current behaviour when WAYLAND_DEBUG is set is to print “array”,
> which is quite unhelpful.
>
> This patch prints a list of the bytes present in the array.  It doesn’t
> try to interpret it as uint32_t or anything, leaving that to the reader
> because this information isn’t present in the protocol description.

To be honest, I'm not really wild about this one. All the array users
I know of (key/button, xdg surface states) are uint32_t. I'd prefer to
add a pretty-printing hint to the protocol - this could specify how to
interpret arrays, and also scratch my long-standing itch of printing
uints with %x rather than %u when it makes sense to ...

Cheers,
Daniel
On Fri, Dec 01, 2017 at 05:08:15PM +0000, Daniel Stone wrote:
> Hi,
> 
> On 10 July 2017 at 19:28, Emmanuel Gil Peyrot
> <emmanuel.peyrot@collabora.com> wrote:
> > The current behaviour when WAYLAND_DEBUG is set is to print “array”,
> > which is quite unhelpful.
> >
> > This patch prints a list of the bytes present in the array.  It doesn’t
> > try to interpret it as uint32_t or anything, leaving that to the reader
> > because this information isn’t present in the protocol description.
> 
> To be honest, I'm not really wild about this one. All the array users
> I know of (key/button, xdg surface states) are uint32_t. I'd prefer to
> add a pretty-printing hint to the protocol - this could specify how to
> interpret arrays, and also scratch my long-standing itch of printing
> uints with %x rather than %u when it makes sense to ...

I would be all for a machine-readable description of the array’s
content, especially to help bindings in languages such as Java or Rust,
which currently have to hardcode the representation on a case by case
basis.  Better debug would only be a nice side effect.

In the meantime, displaying each byte as an uint8_t guarantees there is
no unintended (wrong) interpretation, and reminds us that we don’t have
that yet. :)

> 
> Cheers,
> Daniel
On Fri, 1 Dec 2017 17:08:15 +0000
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
> 
> On 10 July 2017 at 19:28, Emmanuel Gil Peyrot
> <emmanuel.peyrot@collabora.com> wrote:
> > The current behaviour when WAYLAND_DEBUG is set is to print “array”,
> > which is quite unhelpful.
> >
> > This patch prints a list of the bytes present in the array.  It doesn’t
> > try to interpret it as uint32_t or anything, leaving that to the reader
> > because this information isn’t present in the protocol description.  
> 
> To be honest, I'm not really wild about this one. All the array users
> I know of (key/button, xdg surface states) are uint32_t. I'd prefer to
> add a pretty-printing hint to the protocol - this could specify how to
> interpret arrays, and also scratch my long-standing itch of printing
> uints with %x rather than %u when it makes sense to ...

Hi,

just a side note, if you start working on that, do also think of
bindings for (strongly) typed languages. ISTR requests to add type
information so that better bindings could be generated, and the
problem was where to stop defining the type system. I'm pretty sure if
printing hints are added, people will use them for bindings, whether we
wanted or not.


Thanks,
pq
Hi,

On 4 December 2017 at 08:08, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Fri, 1 Dec 2017 17:08:15 +0000 Daniel Stone <daniel@fooishbar.org> wrote:
>> On 10 July 2017 at 19:28, Emmanuel Gil Peyrot
>> <emmanuel.peyrot@collabora.com> wrote:
>> > The current behaviour when WAYLAND_DEBUG is set is to print “array”,
>> > which is quite unhelpful.
>> >
>> > This patch prints a list of the bytes present in the array.  It doesn’t
>> > try to interpret it as uint32_t or anything, leaving that to the reader
>> > because this information isn’t present in the protocol description.
>>
>> To be honest, I'm not really wild about this one. All the array users
>> I know of (key/button, xdg surface states) are uint32_t. I'd prefer to
>> add a pretty-printing hint to the protocol - this could specify how to
>> interpret arrays, and also scratch my long-standing itch of printing
>> uints with %x rather than %u when it makes sense to ...
>
> just a side note, if you start working on that, do also think of
> bindings for (strongly) typed languages. ISTR requests to add type
> information so that better bindings could be generated, and the
> problem was where to stop defining the type system. I'm pretty sure if
> printing hints are added, people will use them for bindings, whether we
> wanted or not.

Here's a strawman.

Since libwayland allows the signature to contain unknown/new values,
as a hint, optionally allow the 'a' type to be preceded by:
  - one character of either 'X', 'I', or 'U', indicating respectively
that the value should be pretty-printed as hexidecimal / signed int /
unsigned int, followed by
  - either '1', '2' or '4' to indicate the byte width of the type
(respectively 8/16/32 bits)

Similarly, allow the 'u' type to be preceded by 'X' indicating that
the value should be pretty-printed in hex.

Cheers,
Daniel
On Tue, 27 Feb 2018 12:39:59 +0000
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
> 
> On 4 December 2017 at 08:08, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Fri, 1 Dec 2017 17:08:15 +0000 Daniel Stone <daniel@fooishbar.org> wrote:  
> >> On 10 July 2017 at 19:28, Emmanuel Gil Peyrot
> >> <emmanuel.peyrot@collabora.com> wrote:  
> >> > The current behaviour when WAYLAND_DEBUG is set is to print “array”,
> >> > which is quite unhelpful.
> >> >
> >> > This patch prints a list of the bytes present in the array.  It doesn’t
> >> > try to interpret it as uint32_t or anything, leaving that to the reader
> >> > because this information isn’t present in the protocol description.  
> >>
> >> To be honest, I'm not really wild about this one. All the array users
> >> I know of (key/button, xdg surface states) are uint32_t. I'd prefer to
> >> add a pretty-printing hint to the protocol - this could specify how to
> >> interpret arrays, and also scratch my long-standing itch of printing
> >> uints with %x rather than %u when it makes sense to ...  
> >
> > just a side note, if you start working on that, do also think of
> > bindings for (strongly) typed languages. ISTR requests to add type
> > information so that better bindings could be generated, and the
> > problem was where to stop defining the type system. I'm pretty sure if
> > printing hints are added, people will use them for bindings, whether we
> > wanted or not.  
> 
> Here's a strawman.
> 
> Since libwayland allows the signature to contain unknown/new values,
> as a hint, optionally allow the 'a' type to be preceded by:
>   - one character of either 'X', 'I', or 'U', indicating respectively
> that the value should be pretty-printed as hexidecimal / signed int /
> unsigned int, followed by
>   - either '1', '2' or '4' to indicate the byte width of the type
> (respectively 8/16/32 bits)

What about 64-bit integers, and float and double? I think the
touchscreen calibration extension will use an array of doubles.

> Similarly, allow the 'u' type to be preceded by 'X' indicating that
> the value should be pretty-printed in hex.

Sounds ok to me.

An alternative proposal is to just print whatever format that could be
fed into od or hexdump, which would avoid the feature creep in
libwayland and message signatures.


Thanks,
pq