Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 17 Jun 2020 09:50:15 +0200
From:   =?utf-8?Q?Micha=C5=82?= Lowas-Rzechonek 
        <michal.lowas-rzechonek@silvair.com>
To:     Brian Gix <brian.gix@intel.com>
Cc:     linux-bluetooth@vger.kernel.org, inga.stotland@intel.com
Subject: Re: [PATCH BlueZ v2] mesh: Add deferral of Attach() and Leave() if
 busy
Message-ID: <20200617075015.4hntygf64dzz6fka@mlowasrzechonek2133>
Mail-Followup-To: Brian Gix <brian.gix@intel.com>,
        linux-bluetooth@vger.kernel.org, inga.stotland@intel.com
References: <20200616181436.50319-1-brian.gix@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200616181436.50319-1-brian.gix@intel.com>
User-Agent: NeoMutt/20180716
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

On 06/16, Brian Gix wrote:
> We require the successful return of JoinComplete() method before
> handling subsequent Attach() or Leave() method calls. To simplify the
> construction of Applications, we will accept one of these calls up to 1
> second prior to receiving the final return status of JoinComplete,
> which tells us that the Application is ready to use the node.
> 
> If the node is still not ready after the deferral, Attach and/or Leave
> will fail.

Nice, I think this solution is simpler than the patch I came up with.

> ---
>  mesh/mesh.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/mesh/mesh.c b/mesh/mesh.c
> index ab2393deb..bc170371d 100644
> --- a/mesh/mesh.c
> +++ b/mesh/mesh.c
> @@ -104,6 +104,10 @@ static struct l_queue *pending_queue;
>  
>  static const char *storage_dir;
>  
> +/* Forward static decalrations */
> +static void def_attach(struct l_timeout *timeout, void *user_data);
> +static void def_leave(struct l_timeout *timeout, void *user_data);
> +
>  static bool simple_match(const void *a, const void *b)
>  {
>  	return a == b;
> @@ -634,12 +638,26 @@ static struct l_dbus_message *attach_call(struct l_dbus *dbus,
>  	uint64_t token;
>  	const char *app_path, *sender;
>  	struct l_dbus_message *pending_msg;
> +	struct mesh_node *node;
>  
>  	l_debug("Attach");
>  
>  	if (!l_dbus_message_get_arguments(msg, "ot", &app_path, &token))
>  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
>  
> +	node = node_find_by_token(token);
> +	if (!node)
> +		return dbus_error(msg, MESH_ERROR_NOT_FOUND, "Attach failed");
> +
> +	if (node_is_busy(node)) {
> +		if (user_data)
> +			return dbus_error(msg, MESH_ERROR_BUSY, NULL);
> +
> +		/* Try once more in 1 second */
> +		l_timeout_create(1, def_attach, l_dbus_message_ref(msg), NULL);
> +		return NULL;
> +	}
> +
>  	sender = l_dbus_message_get_sender(msg);
>  
>  	pending_msg = l_dbus_message_ref(msg);
> @@ -650,6 +668,19 @@ static struct l_dbus_message *attach_call(struct l_dbus *dbus,
>  	return NULL;
>  }
>  
> +static void def_attach(struct l_timeout *timeout, void *user_data)
> +{
> +	struct l_dbus *dbus = dbus_get_bus();
> +	struct l_dbus_message *msg = user_data;
> +	struct l_dbus_message *reply;
> +
> +	l_timeout_remove(timeout);
> +
> +	reply = attach_call(dbus, msg, (void *) true);

Why not pass NULL as the 3rd argument? We register the interface with
empty user data, so this would be an equivalent.

> +	l_dbus_send(dbus, reply);
> +	l_dbus_message_unref(msg);
> +}
> +
>  static struct l_dbus_message *leave_call(struct l_dbus *dbus,
>  						struct l_dbus_message *msg,
>  						void *user_data)
> @@ -666,14 +697,33 @@ static struct l_dbus_message *leave_call(struct l_dbus *dbus,
>  	if (!node)
>  		return dbus_error(msg, MESH_ERROR_NOT_FOUND, NULL);
>  
> -	if (node_is_busy(node))
> -		return dbus_error(msg, MESH_ERROR_BUSY, NULL);
> +	if (node_is_busy(node)) {
> +		if (user_data)
> +			return dbus_error(msg, MESH_ERROR_BUSY, NULL);
> +
> +		/* Try once more in 1 second */
> +		l_timeout_create(1, def_leave, l_dbus_message_ref(msg), NULL);
> +		return NULL;
> +	}
>  
>  	node_remove(node);
>  
>  	return l_dbus_message_new_method_return(msg);
>  }
>  
> +static void def_leave(struct l_timeout *timeout, void *user_data)
> +{
> +	struct l_dbus *dbus = dbus_get_bus();
> +	struct l_dbus_message *msg = user_data;
> +	struct l_dbus_message *reply;
> +
> +	l_timeout_remove(timeout);
> +
> +	reply = leave_call(dbus, msg, (void *) true);

Same here.

> +	l_dbus_send(dbus, reply);
> +	l_dbus_message_unref(msg);
> +}
> +
>  static void create_join_complete_reply_cb(struct l_dbus_message *msg,
>  								void *user_data)
>  {
> -- 
> 2.25.4
> 

-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND