grpc-proxy: Re-work bound_ok/bound_fail events handling

This bigger patch changes the way gRPC proxy client connects to the
server, more specifically how it binds to the same agl_shell interface
as the shell client.

I debated if this should be split into more pieces but I think it makes
more sense to have contained into a single patch as it doesn't make
sense to have some bits off and some bits on, as both the server and the
client side need to be changed at the same time.

This biggest change with this patch is to avoid a potential race condition
between the gRPC proxy client and the shell-client, but also to simplify
the way the gRPC client connects to the server as there aren't sufficient
guards in the server to the provent various corner cases to take place.
Rather than attempting to fix that, simplifying the entire bit and
allow only a single agl-shell client and a sigle gRPC proxy client
connected at a single time and remove any other potential clients
trying to connect at all, later on.

Context and usage:

Both the gRPC proxy and shell-client bind to the same agl_shell
interface, meaning that both use the shell-client (homescreen,
flutter-ics-homescreen, flutter-auto, wam) and the gRPC proxy can
issue agl_shell requests, with some minor differences --
setting background and panels which do not make sense for
gRPC proxy client.

The compositor can signal back to clients when a bind was OK or
not with the bound_ok/bound_fail event. Previously to
adding the bound_ok/bound_fail events, any other client trying to use
agl_shell interface, more than once, would get a protocol violation.
With the bound_ok/bound_fail events clients would instead receive this
event and could theoretically act upon it.

The race that this patch tries to overcome is that the gRPC client will
periodically attempt to verify if there's a shell-client already
connected to server by binding to agl_shell interface and waiting for
bound_fail event. In case it got bound_ok, it would disconnect and
attempt at latter point in time, until it got the bound_failed event, and thus
signalling that there's shell client already connected, allowing to
proceed further.

This worked fine most of the time, depending on how fast the shell
client also bind to agl_shell.  For a brief period of time, it might be
that shell client also gets a bound_fail, requiring it to retry at a
later point in time. The disconnected/reconnect of the gRPC proxy would
then complicate matters as the client resources will get overwritten. So
rather than trying to try that fix that, a better approach is to
simplify the way this works entirely.

The change in this patch is that the gRPC proxy doesn't
connect/disconnect periodically but rather it waits for the compositor
to signal when it is ready to bind to agl_shell interface.

That happens with the help of the doas event status, from the
agl_shell_ext interface. If this event status is alright (OK) then it means
the gRPC can then bind to agl_shell interface.

If it gets a failed status it would just wait and then issue another
doas request and continue as such forever until it got an alright
status. The distinct is that in this case the gRPC proxy would not disconnect
and then reconnect retrying the same thing.

This change to simplify some of the assumption in server, and
client side implementation would just race with the shell client when
binding to the agl_shell interface.

Bug-AGL: SPEC-4977
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
Change-Id: Ib74f789553d3b130ee8e61d0068e617dc2209a58

5 files changed, 166 insertions, 280 deletions

diff --git a/grpc-proxy/log.h b/grpc-proxy/log.h
index d0a5275..9bbba41 100644
--- a/grpc-proxy/log.h
+++ b/grpc-proxy/log.h
@@ -2,7 +2,7 @@
 
 #include <cstdio>
 
-//#define DEBUG
+#define DEBUG
 
 #if !defined(LOG) && defined(DEBUG)
 #define LOG(fmt, ...) do { fprintf(stderr, "%s() " fmt, __func__, ##__VA_ARGS__); } while (0)
diff --git a/grpc-proxy/main-grpc.cpp b/grpc-proxy/main-grpc.cpp
index 545b40f..b86f3d8 100644
--- a/grpc-proxy/main-grpc.cpp
+++ b/grpc-proxy/main-grpc.cpp
@@ -29,6 +29,7 @@
 #include <queue>
 #include <thread>
 #include <mutex>
+#include <chrono>
 #include <condition_variable>
 
 #include "shell.h"
@@ -36,39 +37,11 @@
 #include "main-grpc.h"
 #include "grpc-async-cb.h"
 
-struct shell_data_init {
-	struct agl_shell *shell;
-	bool wait_for_bound;
-	bool bound_ok;
-	bool bound_fail;
-	int version;
-};
+using namespace std::chrono_literals;
 
 static int running = 1;
 
 static void
-agl_shell_bound_ok_init(void *data, struct agl_shell *agl_shell)
-{
-	(void) agl_shell;
-
-	struct shell_data_init *sh = static_cast<struct shell_data_init *>(data);
-	sh->wait_for_bound = false;
-
-	sh->bound_ok = true;
-}
-
-static void
-agl_shell_bound_fail_init(void *data, struct agl_shell *agl_shell)
-{
-	(void) agl_shell;
-
-	struct shell_data_init *sh = static_cast<struct shell_data_init *>(data);
-	sh->wait_for_bound = false;
-
-	sh->bound_fail = true;
-}
-
-static void
 agl_shell_bound_ok(void *data, struct agl_shell *agl_shell)
 {
 	(void) agl_shell;
@@ -76,6 +49,7 @@ agl_shell_bound_ok(void *data, struct agl_shell *agl_shell)
 	struct shell_data *sh = static_cast<struct shell_data *>(data);
 	sh->wait_for_bound = false;
 
+	LOG("bound_ok event!\n");
 	sh->bound_ok = true;
 }
 
@@ -87,7 +61,9 @@ agl_shell_bound_fail(void *data, struct agl_shell *agl_shell)
 	struct shell_data *sh = static_cast<struct shell_data *>(data);
 	sh->wait_for_bound = false;
 
+	LOG("bound_fail event!\n");
 	sh->bound_ok = false;
+	sh->bound_fail = true;
 }
 
 static void
@@ -140,13 +116,6 @@ static const struct agl_shell_listener shell_listener = {
 	agl_shell_app_on_output,
 };
 
-static const struct agl_shell_listener shell_listener_init = {
-	agl_shell_bound_ok_init,
-	agl_shell_bound_fail_init,
-	nullptr,
-	nullptr,
-};
-
 static void
 agl_shell_ext_doas_done(void *data, struct agl_shell_ext *agl_shell_ext, uint32_t status)
 {
@@ -155,8 +124,10 @@ agl_shell_ext_doas_done(void *data, struct agl_shell_ext *agl_shell_ext, uint32_
 	struct shell_data *sh = static_cast<struct shell_data *>(data);
 	sh->wait_for_doas = false;
 
-	if (status == AGL_SHELL_EXT_DOAS_SHELL_CLIENT_STATUS_SUCCESS)
+	if (status == AGL_SHELL_EXT_DOAS_SHELL_CLIENT_STATUS_SUCCESS) {
+		LOG("got doas_ok true!\n");
 		sh->doas_ok = true;
+	}
 }
 
 static const struct agl_shell_ext_listener shell_ext_listener = {
@@ -270,259 +241,134 @@ global_add(void *data, struct wl_registry *reg, uint32_t id,
 	if (!sh)
 		return;
 
-	if (strcmp(interface, agl_shell_interface.name) == 0) {
-		// bind to at least v3 to get events
-		sh->shell =
-			static_cast<struct agl_shell *>(wl_registry_bind(reg, id,
-				&agl_shell_interface,
-				std::min(static_cast<uint32_t>(10), version)));
-		agl_shell_add_listener(sh->shell, &shell_listener, data);
-		sh->version = version;
-	} else if (strcmp(interface, "wl_output") == 0) {
-                display_add_output(sh, reg, id, version);
-        }
-}
+	struct global_data gb;
 
-// the purpose of this _init is to make sure we're not the first shell client
-// running to allow the 'main' shell client take over.
-static void
-global_add_init(void *data, struct wl_registry *reg, uint32_t id,
-		const char *interface, uint32_t version)
-{
-
-	struct shell_data_init *sh = static_cast<struct shell_data_init *>(data);
-
-	if (!sh)
-		return;
+	gb.version = version;
+	gb.id = id;
+	gb.interface_name = std::string(interface);
+	sh->globals.push_back(gb);
 
 	if (strcmp(interface, agl_shell_interface.name) == 0) {
-		sh->shell =
-			static_cast<struct agl_shell *>(wl_registry_bind(reg, id,
-				&agl_shell_interface,
-				std::min(static_cast<uint32_t>(10), version)));
-		agl_shell_add_listener(sh->shell, &shell_listener_init, data);
-		sh->version = version;
+		// nothing here, we're just going to bind a bit later after we
+		// got doas_ok event
+	} else if (strcmp(interface, "wl_output") == 0) {
+                display_add_output(sh, reg, id, version);
+        } else if (strcmp(interface, agl_shell_ext_interface.name) == 0) {
+		sh->shell_ext =
+			static_cast<struct agl_shell_ext *>(wl_registry_bind(reg, id,
+				&agl_shell_ext_interface,
+				std::min(static_cast<uint32_t>(1), version)));
+		agl_shell_ext_add_listener(sh->shell_ext,
+					   &shell_ext_listener, data);
 	}
 }
 
 static void
 global_remove(void *data, struct wl_registry *reg, uint32_t id)
 {
+	struct shell_data *sh = static_cast<struct shell_data *>(data);
 	/* Don't care */
-	(void) data;
 	(void) reg;
 	(void) id;
-}
-
-static void
-global_add_ext(void *data, struct wl_registry *reg, uint32_t id,
-		const char *interface, uint32_t version)
-{
-	struct shell_data *sh = static_cast<struct shell_data *>(data);
-
-	if (!sh)
-		return;
 
-	if (strcmp(interface, agl_shell_ext_interface.name) == 0) {
-		sh->shell_ext =
-			static_cast<struct agl_shell_ext *>(wl_registry_bind(reg, id,
-				&agl_shell_ext_interface,
-				std::min(static_cast<uint32_t>(1), version)));
-		agl_shell_ext_add_listener(sh->shell_ext,
-					   &shell_ext_listener, data);
+	for (std::list<global_data>::iterator it = sh->globals.begin();
+			it != sh->globals.end(); it++) {
+		sh->globals.erase(it);
 	}
 }
 
-static const struct wl_registry_listener registry_ext_listener = {
-	global_add_ext,
-	global_remove,
-};
-
 static const struct wl_registry_listener registry_listener = {
 	global_add,
 	global_remove,
 };
 
-static const struct wl_registry_listener registry_listener_init = {
-	global_add_init,
-	global_remove,
-};
-
-static void
-register_shell_ext(struct wl_display *wl_display, struct shell_data *sh)
-{
-	struct wl_registry *registry;
-
-	registry = wl_display_get_registry(wl_display);
 
-	wl_registry_add_listener(registry, &registry_ext_listener, sh);
-
-	wl_display_roundtrip(wl_display);
-	wl_registry_destroy(registry);
-}
-
-static void
-register_shell(struct wl_display *wl_display, struct shell_data *sh)
-{
-	struct wl_registry *registry;
-
-	wl_list_init(&sh->output_list);
-
-	registry = wl_display_get_registry(wl_display);
-
-	wl_registry_add_listener(registry, &registry_listener, sh);
-
-	wl_display_roundtrip(wl_display);
-	wl_registry_destroy(registry);
-}
-
-static int
-__register_shell_init(void)
+// we expect this client to be up & running *after* the shell client has
+// already set-up panels/backgrounds.
+//
+// this means we need to wait for doas_done event with doas_shell_client_status
+// set to sucess.
+struct shell_data *
+register_shell_ext(void)
 {
+	// try first to bind to agl_shell_ext
 	int ret = 0;
 	struct wl_registry *registry;
-	struct wl_display *wl_display;
-
-	struct shell_data_init *sh = new struct shell_data_init;
 
-	wl_display = wl_display_connect(NULL);
-	if (!wl_display) {
-		ret = -1;
-		goto err_failed_display;
-	}
-	registry = wl_display_get_registry(wl_display);
-	sh->wait_for_bound = true;
-	sh->bound_fail = false;
-	sh->bound_ok = false;
-
-	wl_registry_add_listener(registry, &registry_listener_init, sh);
-	wl_display_roundtrip(wl_display);
+	struct shell_data *sh = new struct shell_data;
 
-	if (!sh->shell || sh->version < 3) {
-		ret = -1;
+	sh->wl_display = wl_display_connect(NULL);
+	if (!sh->wl_display) {
 		goto err;
 	}
 
-	while (ret !=- 1 && sh->wait_for_bound) {
-		ret = wl_display_dispatch(wl_display);
+	registry = wl_display_get_registry(sh->wl_display);
 
-		if (sh->wait_for_bound)
-			continue;
-	}
-
-	ret = sh->bound_fail;
+	sh->wait_for_bound = true;
+	sh->wait_for_doas = true;
 
-	agl_shell_destroy(sh->shell);
-	wl_display_flush(wl_display);
-err:
-	wl_registry_destroy(registry);
-	wl_display_disconnect(wl_display);
+	sh->bound_fail = false;
+	sh->bound_ok = false;
 
-err_failed_display:
-	delete sh;
-	return ret;
-}
+	sh->doas_ok = false;
+	wl_list_init(&sh->output_list);
 
-// we expect this client to be up & running *after* the shell client has
-// already set-up panels/backgrounds.
-// this means the very first try to bind to agl_shell we wait for
-// 'bound_fail' event, which would tell us when it's ok to attempt to
-// bind agl_shell_ext, call doas request, then attempt to bind (one
-// more time) to agl_shell but this time wait for 'bound_ok' event.
-void
-register_shell_init(void)
-{
-	struct timespec ts = {};
+	wl_registry_add_listener(registry, &registry_listener, sh);
+	wl_display_roundtrip(sh->wl_display);
 
-	clock_gettime(CLOCK_MONOTONIC, &ts);
+	if (!sh->shell_ext) {
+		LOG("agl_shell_ext interface was not found!\n");
+		goto err;
+	}
 
-	ts.tv_sec = 0;
-	ts.tv_nsec = 50 * 1000 * 1000;	// 50 ms
+	do {
+		// this should loop until we get back an doas_ok event
+		agl_shell_ext_doas_shell_client(sh->shell_ext);
 
-	// verify if 'bound_fail' was received
-	while (true) {
+		while (ret !=- 1 && sh->wait_for_doas) {
+			ret = wl_display_dispatch(sh->wl_display);
 
-		int r = __register_shell_init();
+			if (sh->wait_for_doas)
+				continue;
+		}
 
-		if (r < 0) {
-			LOG("agl-shell extension not found or version too low\n");
-			exit(EXIT_FAILURE);
-		} else if (r == 1) {
-			// we need to get a 'bound_fail' event, if we get a 'bound_ok'
-			// it means we're the first shell to start so wait until the
-			// shell client actually started
-			LOG("Found another shell client running. "
-			     "Going further to bind to the agl_shell_ext interface\n");
+		if (sh->doas_ok) {
 			break;
 		}
 
-		LOG("No shell client detected running. Will wait until one starts up...\n");
-		nanosleep(&ts, NULL);
-	}
-
-}
-
-static void
-destroy_shell_data(struct shell_data *sh)
-{
-        struct window_output *w_output, *w_output_next;
-
-        wl_list_for_each_safe(w_output, w_output_next, &sh->output_list, link)
-                destroy_output(w_output);
-
-        wl_display_flush(sh->wl_display);
-        wl_display_disconnect(sh->wl_display);
-
-	delete sh;
-}
-
-static struct shell_data *
-start_agl_shell_client(void)
-{
-	int ret = 0;
-	struct wl_display *wl_display;
+		std::this_thread::sleep_for(250ms);
+		sh->wait_for_doas = true;
+	} while (!sh->doas_ok);
 
-	wl_display = wl_display_connect(NULL);
-
-	struct shell_data *sh = new struct shell_data;
-
-	if (!wl_display) {
-		goto err;
-	}
-
-	sh->wl_display = wl_display;
-	sh->wait_for_doas = true;
-	sh->wait_for_bound = true;
-
-	register_shell_ext(wl_display, sh);
-
-	// check for agl_shell_ext
-	if (!sh->shell_ext) {
-		LOG("Failed to bind to agl_shell_ext interface\n");
+	if (!sh->doas_ok) {
+		LOG("agl_shell_ext: failed to get doas_ok status\n");
 		goto err;
 	}
 
-	if (wl_list_empty(&sh->output_list)) {
-		LOG("Failed get any outputs!\n");
-		goto err;
-	}
 
-	agl_shell_ext_doas_shell_client(sh->shell_ext);
-	while (ret != -1 && sh->wait_for_doas) {
-		ret = wl_display_dispatch(sh->wl_display);
-		if (sh->wait_for_doas)
-			continue;
+	// search for the globals to get id and version
+	for (std::list<global_data>::iterator it = sh->globals.begin();
+			it != sh->globals.end(); it++) {
+		if (it->interface_name == "agl_shell") {
+			sh->shell =
+				static_cast<struct agl_shell *>(wl_registry_bind(registry, it->id,
+					&agl_shell_interface, std::min(static_cast<uint32_t>(10),
+						it->version))
+				);
+			agl_shell_add_listener(sh->shell, &shell_listener, sh);
+			break;
+		}
 	}
 
-	if (!sh->doas_ok) {
-		LOG("Failed to get doas_done event\n");
+	if (!sh->shell) {
+		LOG("agl_shell was not advertised!\n");
 		goto err;
 	}
 
-	// bind to agl-shell
-	register_shell(wl_display, sh);
+	// wait to bound now
 	while (ret != -1 && sh->wait_for_bound) {
 		ret = wl_display_dispatch(sh->wl_display);
+
 		if (sh->wait_for_bound)
 			continue;
 	}
@@ -533,12 +379,30 @@ start_agl_shell_client(void)
 		goto err;
 	}
 
-	LOG("agl_shell/agl_shell_ext interface OK\n");
-
+	LOG("agl_shell/agl_shell_ext interfaces OK\n");
 	return sh;
 err:
+	LOG("agl_shell/agl_shell_ext interfaces NOK\n");
+	return NULL;
+}
+
+static void
+destroy_shell_data(struct shell_data *sh)
+{
+        struct window_output *w_output, *w_output_next;
+
+        wl_list_for_each_safe(w_output, w_output_next, &sh->output_list, link)
+                destroy_output(w_output);
+
+       for (std::list<global_data>::iterator it = sh->globals.begin();
+		       it != sh->globals.end(); it++) {
+	       sh->globals.erase(it);
+       }
+
+        wl_display_flush(sh->wl_display);
+        wl_display_disconnect(sh->wl_display);
+
 	delete sh;
-	return nullptr;
 }
 
 static void
@@ -571,11 +435,10 @@ int main(int argc, char **argv)
 
 	// this blocks until we detect that another shell client started
 	// running
-	register_shell_init();
-
-	struct shell_data *sh = start_agl_shell_client();
+	struct shell_data *sh = register_shell_ext();
 	if (!sh) {
-		LOG("Failed to initialize agl-shell/agl-shell-ext\n");
+		LOG("Failed to get register ag_shell_ext\n");
+		thread.join();
 		exit(EXIT_FAILURE);
 	}
 
diff --git a/grpc-proxy/main-grpc.h b/grpc-proxy/main-grpc.h
index 282600d..c067a7c 100644
--- a/grpc-proxy/main-grpc.h
+++ b/grpc-proxy/main-grpc.h
@@ -38,6 +38,12 @@
 // forward declaration created in grpc-async-cb
 class Lister;
 
+struct global_data {
+	uint32_t id;
+	uint32_t version;
+	std::string interface_name;
+};
+
 struct shell_data {
 	struct wl_display *wl_display;
 	struct agl_shell *shell;
@@ -47,6 +53,7 @@ struct shell_data {
 	bool wait_for_doas;
 
 	bool bound_ok;
+	bool bound_fail;
 	bool doas_ok;
 
 	uint32_t version;
@@ -54,6 +61,7 @@ struct shell_data {
 
 	::agl_shell_ipc::AppStateResponse current_app_state;
 	std::list<std::pair<grpc::CallbackServerContext*, Lister *> > server_context_list;
+	std::list<global_data> globals;
 };
 
 struct window_output {
diff --git a/src/ivi-compositor.h b/src/ivi-compositor.h
index 6b6204e..4c04ec7 100644
--- a/src/ivi-compositor.h
+++ b/src/ivi-compositor.h
@@ -97,6 +97,7 @@ struct ivi_compositor {
 		struct wl_client *client;
 		struct wl_resource *resource;
 		bool doas_requested;
+		bool doas_requested_pending_bind;
 		enum agl_shell_bound_status status;
 	} shell_client_ext;
 
diff --git a/src/shell.c b/src/shell.c
index c089bec..2b6bd63 100644
--- a/src/shell.c
+++ b/src/shell.c
@@ -1698,6 +1698,9 @@ shell_set_app_scale(struct wl_client *client, struct wl_resource *res,
 static void
 shell_ext_destroy(struct wl_client *client, struct wl_resource *res)
 {
+	struct 	ivi_compositor *ivi = wl_resource_get_user_data(res);
+
+	ivi->shell_client_ext.doas_requested = false;
 	wl_resource_destroy(res);
 }
 
@@ -1707,8 +1710,17 @@ shell_ext_doas(struct wl_client *client, struct wl_resource *res)
 	struct 	ivi_compositor *ivi = wl_resource_get_user_data(res);
 
 	ivi->shell_client_ext.doas_requested = true;
-	agl_shell_ext_send_doas_done(ivi->shell_client_ext.resource,
-				     AGL_SHELL_EXT_DOAS_SHELL_CLIENT_STATUS_SUCCESS);
+
+	if (ivi->shell_client_ext.resource && ivi->shell_client.resource) {
+		ivi->shell_client_ext.doas_requested_pending_bind = true;
+
+		agl_shell_ext_send_doas_done(ivi->shell_client_ext.resource,
+					     AGL_SHELL_EXT_DOAS_SHELL_CLIENT_STATUS_SUCCESS);
+	} else {
+		agl_shell_ext_send_doas_done(ivi->shell_client_ext.resource,
+					     AGL_SHELL_EXT_DOAS_SHELL_CLIENT_STATUS_FAILED);
+	}
+
 }
 
 static const struct agl_shell_interface agl_shell_implementation = {
@@ -1871,6 +1883,7 @@ unbind_agl_shell_ext(struct wl_resource *resource)
 
 	ivi->shell_client_ext.resource = NULL;
 	ivi->shell_client.resource_ext = NULL;
+	ivi->shell_client_ext.doas_requested = false;
 }
 
 static void
@@ -1897,54 +1910,55 @@ bind_agl_shell(struct wl_client *client,
 		return;
 	}
 
-	if (ivi->shell_client.resource) {
-		if (wl_resource_get_version(resource) == 1) {
-			wl_resource_post_error(resource, WL_DISPLAY_ERROR_INVALID_OBJECT,
-					       "agl_shell has already been bound");
-			return;
-		}
-
-		if (ivi->shell_client_ext.resource && 
-		    ivi->shell_client_ext.doas_requested) {
-
-			/* reset status in case client-ext doesn't send an
-			 * explicit agl_shell_destroy request, see
-			 * shell_destroy() */
-			if (ivi->shell_client.status == BOUND_FAILED)
-				ivi->shell_client.status = BOUND_OK;
+	if (ivi->shell_client.resource && wl_resource_get_version(resource) == 1) {
+		wl_resource_post_error(resource, WL_DISPLAY_ERROR_INVALID_OBJECT,
+				"agl_shell has already been bound (version 1)");
+		wl_resource_destroy(resource);
+		return;
+	}
 
-			wl_resource_set_implementation(resource, &agl_shell_implementation,
-						       ivi, NULL);
-			ivi->shell_client.resource_ext = resource;
+	// for agl_shell client proxy
+	if (ivi->shell_client.resource &&
+	    ivi->shell_client_ext.doas_requested_pending_bind) {
 
-			ivi->shell_client_ext.status = BOUND_OK;
-			agl_shell_send_bound_ok(ivi->shell_client.resource_ext);
+		ivi->shell_client_ext.doas_requested_pending_bind = false;
 
-			return;
-		} else {
-			wl_resource_set_implementation(resource, &agl_shell_implementation,
-						       ivi, NULL);
-			agl_shell_send_bound_fail(resource);
-			ivi->shell_client.status = BOUND_FAILED;
+		// if there's one connected
+		if (ivi->shell_client.resource_ext) {
+			ivi->shell_client_ext.status = BOUND_FAILED;
+			agl_shell_send_bound_fail(ivi->shell_client.resource_ext);
+			wl_resource_destroy(resource);
 			return;
 		}
+
+		wl_resource_set_implementation(resource, &agl_shell_implementation,
+					       ivi, NULL);
+		ivi->shell_client.resource_ext = resource;
+		ivi->shell_client_ext.status = BOUND_OK;
+		agl_shell_send_bound_ok(ivi->shell_client.resource_ext);
+		return;
 	}
 
 
+	// if we already have an agl_shell client
+	if (ivi->shell_client.resource) {
+		agl_shell_send_bound_fail(resource);
+		ivi->shell_client.status = BOUND_FAILED;
+		wl_resource_destroy(resource);
+		return;
+	}
+
+	// default agl_shell client
+	wl_resource_set_implementation(resource, &agl_shell_implementation,
+				       ivi, unbind_agl_shell);
+	ivi->shell_client.resource = resource;
+
 	if (wl_resource_get_version(resource) >=
 	    AGL_SHELL_BOUND_OK_SINCE_VERSION) {
-		wl_resource_set_implementation(resource, &agl_shell_implementation,
-					       ivi, unbind_agl_shell);
-		ivi->shell_client.resource = resource;
 		/* if we land here we'll have BOUND_OK by default,
 		   but still do the assignment */
 		ivi->shell_client.status = BOUND_OK;
 		agl_shell_send_bound_ok(ivi->shell_client.resource);
-	} else {
-		/* fallback for just version 1 of the protocol */
-		wl_resource_set_implementation(resource, &agl_shell_implementation,
-					       ivi, unbind_agl_shell);
-		ivi->shell_client.resource = resource;
 	}
 }