On Mon, Sep 28, 2026 at 01:34:40PM +0200, Kirill A. Korinsky wrote:
> On Mon, 28 Sep 2026 13:14:44 +0200,
> Stefan Sperling <stsp@stsp.name> wrote:
> > 
> > On Mon, Sep 28, 2026 at 12:06:22PM +0200, Kirill A. Korinsky wrote:
> > > From 8636d1b7d6412861f72d8c9ebfe9064b3337e5c8 Mon Sep 17 00:00:00 2001
> > > From: "Kirill A. Korinsky" <kirill@korins.ky>
> > > Date: Sun, 27 Sep 2026 12:57:24 +0200
> > > Subject: [PATCH 05/11] sys/qwz: wait for confirmed peer release
> > > 
> > > Wait for both an explicit peer-unmap event and the delete response
> > > before releasing a peer. This also covers a creation that timed out
> > > before its mapping event arrived.
> > 
> > All the other diffs are OK by me.
> > 
> > Only this one commit looks a bit strange, and I cannot understand
> > the reasoning behind it without more details.
> > 
> > I cannot tell what the first part of your log message is trying to say
> > with "Wait for both an explicit peer-unmap event and the delete response".
> > The code which unmaps and deletes a peer already handles both events.
> > How is the existing code broken?
> > 
> > Previously, the peer_mapped flag was used to track both map and unmap events,
> > for no particular reason other than that the same flag can be used for both.
> > 
> > Now you've added a dedicated unmapped flag to track unmap events separately,
> > but still report unmapping events to code which sleeps on &sc->peer_mapped.
> > Is this intentional? Is this what the following part of your log message is
> > referring to? "This also covers a creation that timed out before its mapping
> > event arrived."  What problem does this solve exactly? A timeout would rather
> > be handled via error codes returned from tsleep, wouldn't it?
> > 
> 
> Well, not only. If timeout had happened we never call this block:
> 
> 	TAILQ_REMOVE(&sc->peers, peer, entry);
> 	qwz_node_clear_peer_id(sc, peer);
> 	free(peer, M_DEVBUF, sizeof(*peer));
> 	sc->num_peers--;
> 
> and we ignore error in qwz_stop() for example.

You mean if either of the delete command confirmations times out we never
remove the peer from the list, right? That is correct.

And we don't remove the peer from our list if firmware fails to confirm
creation of the peer, either.

> If I not wrong, it will leak a bit.
> 
> Am I wrong?

Not wrong, it's a small leak, but I believe this leak is temporary.
We only leak it until the interface goes down because qwz_core_deinit()
will call qwz_free_peers(). So I don't think this is a big problem.
These timeout cases should be rare edge cases anyway.