Change in ...libosmocore[master]: add osmo_fsm_inst_watch()

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

neels gerrit-no-reply at lists.osmocom.org
Fri Oct 4 17:41:22 UTC 2019


neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15660 )

Change subject: add osmo_fsm_inst_watch()
......................................................................


Patch Set 2:

I wish my reply could be shorter, but this is a quite important aspect of osmo_fsm design, so I am taking the time necessary for this.

There are two proposals, by Pau and Vadim.

In short, I disagree with Pau, but find Vadim's idea interesting.

But even if Vadim's idea solves most cases,
I have another use case that needs to "watch" an FSM instance past main loops.
I still have another idea to solve that differently, too.

So I think I will now try Vadim's idea to replace this patch.

In long:


* Pau's callback idea:

> TBH I don't like this API, I think it's too specific.
> 
> I'd rather add a free_cb, which is more common for readers and more generic (can be reused for other purposes)

(BTW, your proposal lacks a osmo_fsm_free_cb_remove(), I'll just assume that is included)

IMHO use cases that need a callback function should rather be solved by event dispatch.
If you're aiming at arbitrary cleanup actions, I'd rather use the FSM instance cleanup() functions.
This here is really aiming only at the use case "Skip some action if an instance was freed".

Usually we notify parent / child FSM instances of deallocation by events.
But this doesn't work well in functions that are already running.
Even if the term event sets a flag in the fi->priv struct, that may also be deallocated along.

I am really aiming at only a 1bit information whether an FSM instance with its storage still exists,
within a running function.

A callback would also work, but would make implementing it more complex.

Let's compare in pseudocode:

Your proposal:

  my_free_callback()
  {
       complex_action1();
       complex_action2();
       complex_action3();
  }

  safe_event_handling()
  {
       add_free_callback(fi2, my_free_callback, NULL);
       event_dispatch(fi1);
       remove_free_callback(fi2, my_free_callback);
  }

But since this calling my_free_callback() is in essence a 1bit information, we might as well do:

  safe_event_handling()
  {
       fi_watch(&watch, fi2);
       event_dispatch(fi1);
       if (!watch.exists) {
         complex_action1();
         complex_action2();
         complex_action3();
       }
       fi_unwatch(&watch);
  }

I like this better because we can read the code in the flow of a single function.

Furthermore, I am not aiming at adding more cleanup actions, but rather want to *skip* some usually normal action if a free has occured due to error handling. For that I ultimately need a bool flag to be evaluated in a running function.

Your proposal solves it this way:

  my_free_callback(void *data)
  {
       bool *flag = data;
       *flag = false;
  }

  safe_event_handling()
  {
       bool flag = true;
       add_free_callback(fi2, my_free_callback, &flag);
       event_dispatch(fi1);
       if (!flag)
          return;
       normal_action();
  }

This would work, but since a bool is all we really need to be notified of a free, my patch is simpler.

  safe_event_handling()
  {
       fi_watch(&watch, fi2);
       event_dispatch(fi1);
       fi_unwatch(&watch);
       if (!watch.exists)
           return;
       normal_action();
  }


* Vadim's idea to free FSM instances in the main loop:

We already have this osmo_fsm_inst_term_safely() feature, where terminated FSM instances get collected in a talloc pool, and only get deallocated when the last one is done. This solves various use-after-free problems in the same way as proposed, but indeed deferring deallocation to the main loop would more generally solve the problem in a simpler fashion.
Implementation: We could talloc_reparent terminating FSM instances to this OTC_SELECT context, and then FSM instances would be deallocated by the next main loop iteration.
It would be only a slight modification of the existing osmo_fsm_inst_term_safely() mechanisms.

If we can be sure that terminated FSM instances are never deallocated before a function call returns back to osmo_select_main*(),
we may not even need any code changes, if we drop events when the target inst is already terminating,
or we could also do this:

   event_dispatch(fi1);
   if (fi2->proc.terminated)
        return;

That would indeed be much simpler than my 'watcher' proposal. I like that.

The only problem I see is that so far osmo_fsm API does not necessarily require an osmo_select_main_ctx(), or any select at all.
But this is easily solved by adding another flag to the API that changes behavior only when set to true.
A program needing deferred allocation can set the flag to true and then knows that it should use osmo_select_main_ctx().


* Other use case for "watcher":

osmo-mgw/src/libosmo-mgcp-client/mgcp_client_endpoint_fsm.c: this FSM instance handles MGCP and the state of an MGW endpoint with its individual connections. There we have the feature to notify an arbitrary FSM instance of failure or success:

  void osmo_mgcpc_ep_ci_request(...,
                              struct osmo_fsm_inst *notify,
                              uint32_t event_success, uint32_t event_failure,
                              void *notify_data);

This triggers e.g. a CRCX or MDCX, and once the response comes back, dispatches an arbitrary event to an arbitrary FSM instance.
So far we had no check whether that 'notify' FSM instance still exists when the MGCP response is received.
Obviously the MGCP response will come in a later main loop select iteration.

So far there would be the need to somehow tell the osmo_mgcpc_ep_ci FSM that a 'notify' target has disappeared,
but since we so far lack such API, we basically hope that the instance is still around when the reply comes in.
I now I realized that this 'notify' is still vulnerable to use-after-free, by the order of messages coming in.
Say a CC Release comes in in-between an MDCX and its OK response, so the CC Release deallocates,
then the OK response dispatches to deallocated 'notify'.

One way to solve that would be the proposed "watcher", which I have already implemented in a patch.
That would work generally, and the calling FSM instance would not need to take any more action to prevent
use-after-free during accessing the 'notify' FSM instance.

Another way would be to add an osmo_mgcpc_ep_cancel_notify() API that makes sure no 'notify' are dispatched.
Here a calling FSM instance would have to make sure to call this function before deallocation.
That would be fair enough.

I think it would be nice to have a generalized way of telling whether an FSM instance is still there or not.
But I also noticed that it can be hard to make sure to call the osmo_fsm_inst_unwatch() in all cases.

So I will go for Vadim's solution and the osmo_mgcpc_ep_cancel_notify().


-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15660
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I4d8306488506c60b4c2fc1c4cb3ac04654db9c43
Gerrit-Change-Number: 15660
Gerrit-PatchSet: 2
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-CC: fixeria <axilirator at gmail.com>
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Fri, 04 Oct 2019 17:41:22 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20191004/3b4ed4cf/attachment.htm>


More information about the gerrit-log mailing list