From 086f96a702a66e561b22d03cdeb98c2f97f1328b Mon Sep 17 00:00:00 2001
From: Peter Hutterer <peter.hutterer@who-t.net>
Date: Wed, 1 Mar 2023 10:32:32 +1000
Subject: [PATCH] proto: split up message length and opcode again

Now that we have 64 bit integers on the wire and 64 bit object IDs,
we're already different to the Wayland protocol. So we might as well get
the full length and split message length and opcode again to make header
parsing and composing simpler.

This effectively reverts commit bf45a7182cb2f4c13f11e141fc846244d3ac6212.
---
 proto/protocol.xml   |  7 ++--
 src/brei-shared.c    | 95 +++++++++++++++++++++++++++-----------------
 src/util-macros.h    |  1 +
 test/eiproto.py.tmpl | 16 ++++----
 4 files changed, 69 insertions(+), 50 deletions(-)

diff --git a/proto/protocol.xml b/proto/protocol.xml
index bfbb583..0d7a02b 100644
--- a/proto/protocol.xml
+++ b/proto/protocol.xml
@@ -31,13 +31,12 @@
 
   <!--
 
-    Protocol wire format: [sender-id, length | opcode, ...]
+    Protocol wire format: [sender-id, length, opcode, ...]
     Where:
     - sender-id is one 64-bit integer
-    - length and opcode share one 32-bit integer, and
-      length are the high 16 bits and opcode are the low 16 bits.
+    - length and opcode are 32-bit integers
     - all integers are in the EIS implementation's native byte order.
-    - length is the length of the message in bytes, including the 8 header bytes
+    - length is the length of the message in bytes, including the 16 header bytes
     - sender-id is the id of the object sending the request/event. The sender-id
       0 is reserved for the special "ei_handshake" object.
     - opcode is the event or request-specific opcode, starting at 0.
diff --git a/src/brei-shared.c b/src/brei-shared.c
index 059e11d..62d8b69 100644
--- a/src/brei-shared.c
+++ b/src/brei-shared.c
@@ -42,6 +42,13 @@ struct brei_context {
 	void *log_context;
 };
 
+struct brei_header {
+	object_id_t sender_id;
+	uint32_t msglen;	/* length of message in bytes including this header */
+	uint32_t opcode;
+} _packed_;
+static_assert(sizeof(struct brei_header) == 16, "Unexpected size for brei_header struct");
+
 struct brei_string {
 	uint32_t len;
 	const char str[];
@@ -323,15 +330,14 @@ brei_marshal_message(struct brei_context *brei,
 		     size_t nargs,
 		     va_list args)
 {
-
 	_cleanup_iobuf_ struct iobuf *buf = iobuf_new(128);
 
 	_unref_(brei_result) *result = brei_marshal(brei, buf, signature, nargs, args);
 	if (result)
 		return steal(&result);
 
-	size_t message_len = iobuf_len(buf) + 12;
-	uint32_t header[3] = {0, 0, message_len << 16 | opcode};
+	size_t message_len = iobuf_len(buf) + sizeof(struct brei_header);
+	uint32_t header[4] = {0, 0, message_len, opcode};
 	memcpy(header, &id, sizeof(id));
 	iobuf_prepend(buf, (const char *)header, sizeof(header));
 
@@ -361,20 +367,23 @@ brei_dispatch(struct brei_context *brei,
 		uint32_t *data = (uint32_t*)iobuf_data(buf);
 		size_t len = iobuf_len(buf);
 
-		const size_t headersize = 12; /* object-id, length | opcode */
+		const size_t headersize = sizeof(struct brei_header);
 
 		if (len < headersize)
 			break;
 
-		object_id_t object_id = *(uint64_t*)data; /* 8 bytes for object id */
-		size_t msglen = data[2] >> 16; /* 2 bytes message length */
-		uint32_t opcode = data[2] & 0xFFFF; /* 2 bytes for opcode */
+		const struct brei_header *header = (const struct brei_header*)data;
+		uint32_t msglen = header->msglen;
+
 		if (len < msglen)
 			break;
 
+		object_id_t object_id = header->sender_id;
+		uint32_t opcode = header->opcode;;
+
 		/* Find the object, it is stored in the ei/eis context  */
 		struct brei_object *object = NULL;
-		rc = lookup_object(object_id, &object, user_data);
+		rc = lookup_object(header->sender_id, &object, user_data);
 		if (rc == -ENOENT) {
 			iobuf_pop(buf, msglen);
 			continue;
@@ -395,7 +404,7 @@ brei_dispatch(struct brei_context *brei,
 		if (opcode >= interface->nincoming) {
 			result = brei_result_new(BREI_CONNECTION_DISCONNECT_REASON_PROTOCOL,
 						 "opcode %u exceeds interface %s method count %u",
-						 opcode, interface->name, interface->nincoming);
+						 header->opcode, interface->name, interface->nincoming);
 			goto error;
 		}
 
@@ -564,7 +573,7 @@ MUNIT_TEST(test_brei_send_message)
 	int sock_read = sv[0];
 	int sock_write = sv[1];
 
-	const int header_size = 12;
+	const int header_size = 16;
 
 	{
 		const int msglen = header_size + 8; /* 2 * 4 bytes */
@@ -575,13 +584,15 @@ MUNIT_TEST(test_brei_send_message)
 		munit_assert_int(rc, ==, msglen);
 
 		uint32_t buf[64];
-		uint64_t *buf_id = (uint64_t*)buf;
 		int len = read(sock_read, buf, sizeof(buf));
 		munit_assert_int(len, ==, msglen);
-		munit_assert_int(*buf_id, ==, id);
-		munit_assert_int(buf[2], ==, msglen << 16 | opcode);
-		munit_assert_int(buf[3], ==, 0xff);
-		munit_assert_int(buf[4], ==, 0xdddd);
+
+		const struct brei_header *header = (const struct brei_header*)buf;
+		munit_assert_int(header->sender_id, ==, id);
+		munit_assert_int(header->msglen, ==, msglen);
+		munit_assert_int(header->opcode, ==, opcode);
+		munit_assert_int(buf[4], ==, 0xff);
+		munit_assert_int(buf[5], ==, 0xdddd);
 	}
 	{
 		const int msglen = header_size + 8; /* 2 * 4 bytes */
@@ -592,18 +603,21 @@ MUNIT_TEST(test_brei_send_message)
 		munit_assert_int(rc, ==, msglen);
 
 		uint32_t buf[64];
-		uint64_t *buf_id = (uint64_t*)buf;
 		int len = read(sock_read, buf, sizeof(buf));
 		union {
 			uint32_t bytes;
 			float f;
 		} ufloat;
 		munit_assert_int(len, ==, msglen);
-		munit_assert_int(*buf_id, ==, id);
-		munit_assert_int(buf[2], ==, msglen << 16 | opcode);
-		ufloat.bytes = buf[3];
+
+		const struct brei_header *header = (const struct brei_header*)buf;
+		munit_assert_int(header->sender_id, ==, id);
+		munit_assert_int(header->msglen, ==, msglen);
+		munit_assert_int(header->opcode, ==, opcode);
+
+		ufloat.bytes = buf[4];
 		munit_assert_double_equal(ufloat.f, 1.234, 4/* precision */);
-		munit_assert_int(buf[4], ==, -12);
+		munit_assert_int(buf[5], ==, -12);
 	}
 
 	{
@@ -620,20 +634,23 @@ MUNIT_TEST(test_brei_send_message)
 		munit_assert_int(rc, ==, msglen);
 
 		uint32_t buf[64];
-		uint64_t *buf_id = (uint64_t*)buf;
 		int len = read(sock_read, buf, sizeof(buf));
 		munit_assert_int(len, ==, msglen);
-		munit_assert_int(*buf_id, ==, id);
-		munit_assert_int(buf[2], ==, msglen << 16 | opcode);
-		munit_assert_int(buf[3], ==, -42);
 
-		const struct brei_string *s = (const struct brei_string *)&buf[4];
+		const struct brei_header *header = (const struct brei_header*)buf;
+		munit_assert_int(header->sender_id, ==, id);
+		munit_assert_int(header->msglen, ==, msglen);
+		munit_assert_int(header->opcode, ==, opcode);
+
+		munit_assert_int(buf[4], ==, -42);
+
+		const struct brei_string *s = (const struct brei_string *)&buf[5];
 		munit_assert_int(s->len, ==, strlen0(string));
 		munit_assert_string_equal(s->str, string);
 		munit_assert_int(memcmp(s->str, string, brei_string_proto_length(s->len) - 4), ==, 0);
 
-		munit_assert_int(buf[5 + slen/4], ==, 0xab);
-		munit_assert_int(buf[7 + slen/4], ==, 0xcdef);
+		munit_assert_int(buf[6 + slen/4], ==, 0xab);
+		munit_assert_int(buf[8 + slen/4], ==, 0xcdef);
 	}
 
 	{
@@ -648,18 +665,20 @@ MUNIT_TEST(test_brei_send_message)
 		munit_assert_int(rc, ==, msglen);
 
 		uint32_t buf[64];
-		uint64_t *buf_id = (uint64_t*)buf;
 		int len = read(sock_read, buf, sizeof(buf));
 		munit_assert_int(len, ==, msglen);
-		munit_assert_int(*buf_id, ==, id);
-		munit_assert_int(buf[2], ==, msglen << 16 | opcode);
 
-		const struct brei_string *s1 = (const struct brei_string *)&buf[3];
+		const struct brei_header *header = (const struct brei_header*)buf;
+		munit_assert_int(header->sender_id, ==, id);
+		munit_assert_int(header->msglen, ==, msglen);
+		munit_assert_int(header->opcode, ==, opcode);
+
+		const struct brei_string *s1 = (const struct brei_string*)&buf[4];
 		munit_assert_int(s1->len, ==, strlen0(string1));
 		munit_assert_string_equal(s1->str, string1);
 		munit_assert_int(memcmp(s1->str, string1, brei_string_proto_length(s1->len) - 4), ==, 0);
 
-		const struct brei_string *s2 = (const struct brei_string *)&buf[7];
+		const struct brei_string *s2 = (const struct brei_string *)&buf[8];
 		munit_assert_int(s2->len, ==, strlen0(string2));
 		munit_assert_string_equal(s2->str, string2);
 		munit_assert_int(memcmp(s2->str, string2, brei_string_proto_length(s2->len) - 4), ==, 0);
@@ -691,13 +710,15 @@ MUNIT_TEST(test_brei_send_message)
 		int len = iobuf_recv_from_fd(recv, sock_read);
 
 		uint32_t *buf = (uint32_t*)iobuf_data(recv);
-		uint64_t *buf_id = (uint64_t*)buf;
 		munit_assert_int(len, ==, msglen);
-		munit_assert_int(*buf_id, ==, id);
-		munit_assert_int(buf[2], ==, msglen << 16 | opcode);
-		munit_assert_int(buf[3], ==, 0xab);
+		const struct brei_header *header = (const struct brei_header*)buf;
+		munit_assert_int(header->sender_id, ==, id);
+		munit_assert_int(header->msglen, ==, msglen);
+		munit_assert_int(header->opcode, ==, opcode);
+
+		munit_assert_int(buf[4], ==, 0xab);
 		/* fd is not in data */
-		munit_assert_int(buf[4], ==, 0xcd);
+		munit_assert_int(buf[5], ==, 0xcd);
 
 		_cleanup_close_ int fd = iobuf_take_fd(recv);
 		munit_assert_int(fd, !=, -1);
diff --git a/src/util-macros.h b/src/util-macros.h
index 7fb131e..62243db 100644
--- a/src/util-macros.h
+++ b/src/util-macros.h
@@ -54,6 +54,7 @@
 #define _printf_(_a, _b) __attribute__((format (printf, _a, _b)))
 #define _fallthrough_ __attribute__((fallthrough))
 #define _unused_ __attribute__((unused))
+#define _packed_ __attribute__((packed))
 
 #define run_only_once \
 	static int _once_per_##__func__ = 0; \
diff --git a/test/eiproto.py.tmpl b/test/eiproto.py.tmpl
index 788e5ed..a4c120d 100644
--- a/test/eiproto.py.tmpl
+++ b/test/eiproto.py.tmpl
@@ -63,18 +63,16 @@ class MessageHeader:
 
     @classmethod
     def size(cls) -> int:
-        return 12
+        return 16
 
     @classmethod
     def from_data(cls, data: bytes) -> "MessageHeader":
-        object_id, opcode = struct.unpack("=QI", data[:cls.size()])
-        msglen = opcode >> 16
-        opcode = opcode & 0xFFFF
+        object_id, msglen, opcode = struct.unpack("=QII", data[:cls.size()])
         return cls(object_id, msglen, opcode)
 
     @property
-    def as_tuple(self) -> Tuple[int, int]:
-        return self.object_id, self.msglen << 16 | self.opcode
+    def as_tuple(self) -> Tuple[int, int, int]:
+        return self.object_id, self.msglen, self.opcode
 
 
 @attr.s
@@ -150,7 +148,7 @@ class Interface:
     outgoing: dict[int, str] = attr.ib(default=attr.Factory(list), repr=False)
 
     def format(self, *args, opcode: int, signature: str) -> bytes:
-        encoding = ["=QI"]
+        encoding = ["=QII"]
         arguments = []
         for sig, arg in zip(signature, args):
             if sig in ["u"]:
@@ -181,7 +179,7 @@ class Interface:
         return struct.pack(format, *header.as_tuple, *arguments)
 
     def unpack(self, data, signature: str, names: list[str]) -> Tuple[int, dict[str, Any]]:
-        encoding = ["=QI"]  # the header
+        encoding = ["=QII"]  # the header
         for sig in signature:
             if sig in ["u"]:
                 encoding.append("I")
@@ -212,7 +210,7 @@ class Interface:
         # logger.debug(f"unpacked {format} to {values}")
 
         results = []
-        values = values[2:]  # drop id, length | opcode
+        values = values[3:]  # drop id, length, opcode
 
         # we had to insert the string length into the format, filter the
         # value for that out again.