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:   Mon, 15 Jun 2020 22:32:47 +0200
From:   "michal.lowas-rzechonek@silvair.com" 
        <michal.lowas-rzechonek@silvair.com>
To:     "Gix, Brian" <brian.gix@intel.com>
Cc:     "Stotland, Inga" <inga.stotland@intel.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Subject: Re: [PATCH BlueZ v3 5/5] mesh: Add "node is busy" check for Leave()
 & Attach()
Message-ID: <20200615203247.4zw2vhauuducewws@kynes>
Mail-Followup-To: "Gix, Brian" <brian.gix@intel.com>,
        "Stotland, Inga" <inga.stotland@intel.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
References: <20200610171121.46910-1-inga.stotland@intel.com>
 <20200610171121.46910-6-inga.stotland@intel.com>
 <20200615082259.bpstucduyunyqvmt@mlowasrzechonek2133>
 <72efc0bd23669143a0c1b9749a6c523f1ea49f5f.camel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <72efc0bd23669143a0c1b9749a6c523f1ea49f5f.camel@intel.com>
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Inga, Brian,

On 06/15, Gix, Brian wrote:
> > With this patch, this call sequence fails, because now we're supposed to
> > send a *reply* to JoinComplete first, and only then call Attach()?
> 
> A couple months ago, we made the decision (with your input, I believe)
> that we needed to use JoinComplete on every node creation (Join,
> Import, Create), to ensure that the App has the token (...)

Yes, I remember that discussion. The rationale was ability to catch
bugs in the application, and get rid of created, but effectively
unusable nodes.

> This creates (...) the unfortunate situation revealed in one of your
> test-suite cases where if Leave() was called before returning the
> JoinComplete() call, we Seg-Faulted.

Indeed, although I don't think it's necessary to introduce a "busy"
state...

In case of Leave(), this call removes the node anyway, so what I think
would suffice is to check whether the node still exists when
JoinComplete reply arrives, to avoid freeing the node twice (causing
SEGV).

void node_finalize_new_node(struct mesh_node *node, struct mesh_io *io)
{
	if (!node)
		return;

	if (!l_queue_find(nodes, match_simple, L_UINT_TO_PTR(node)))
		return;

    // ...
}

This would allow the application to call Leave *before* sending a reply
to JoinComplete.

As for Attach(), I also think it should be legal to call it before
replying to JoinComplete. The worst thing that can happen is that
application successfully attaches, then replies to JoinComplete with an
error. This would simply remove the node, and the application would be
promptly detached.


> > I'm using a high-level API for D-Bus, so I don't really control when the
> > reply is sent, so at the moment the only way to implement this would be
> > by delaying Attach() by a fixed timeout - and I'm not comfortable with
> > that.
> 
> 
> Yeah, I can see how this is now required...  
> 
> In the mesh-cfgclient tool (which is also built on ELL) we accomplish
> this by scheduling an idle_oneshot for the Attach.  

Unfortunately, not all main loops have API to schedule "idle" calls,
i.e. calls executed when the loop doesn't have anything better to do.

I know that both ELL and Glib do, but AFAIR Qt does not (it uses timers
with timeout set to 0, if I'm not mistaken), and Python's asyncio
doesn't either.

I don't think requiring a specific sequence of dbus messages is a good
idea :(

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