From 24342db4766897b022e3ffc836253858221a638f Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Thu, 13 Jun 2024 15:25:59 +0200 Subject: [PATCH] base/signal.h: remove pointers from API This patch updates the signal API to avoid raw pointers, and replaces the Context_already_in_use and Context_not_associated exceptions by diagnostic messages. Fixes #5247 --- repos/base-hw/src/lib/base/signal_receiver.cc | 35 ++++++++++--------- repos/base-hw/src/test/cpu_quota/main.cc | 4 +-- repos/base/include/base/signal.h | 27 ++++---------- repos/base/include/timer_session/connection.h | 4 +-- repos/base/lib/symbols/ld | 4 +-- repos/base/src/core/signal_receiver.cc | 6 ++-- .../base/internal/expanding_parent_client.h | 2 +- repos/base/src/lib/base/component.cc | 2 +- repos/base/src/lib/base/entrypoint.cc | 4 +-- repos/base/src/lib/base/signal.cc | 22 ++++++------ repos/base/src/lib/base/signal_common.cc | 30 ++++++++-------- repos/os/include/audio_in_session/client.h | 4 +-- repos/os/include/audio_out_session/client.h | 4 +-- .../os/include/terminal_session/connection.h | 4 +-- repos/os/src/test/gpio_signal/main.cc | 2 +- repos/os/src/test/signal/main.cc | 24 ++++++------- repos/os/src/test/terminal_crosslink/main.cc | 2 +- 17 files changed, 85 insertions(+), 95 deletions(-) diff --git a/repos/base-hw/src/lib/base/signal_receiver.cc b/repos/base-hw/src/lib/base/signal_receiver.cc index 83595c7c87..cf41e4dc63 100644 --- a/repos/base-hw/src/lib/base/signal_receiver.cc +++ b/repos/base-hw/src/lib/base/signal_receiver.cc @@ -78,7 +78,7 @@ void Signal_receiver::_platform_destructor() } -void Signal_receiver::_platform_begin_dissolve(Signal_context * const c) +void Signal_receiver::_platform_begin_dissolve(Signal_context &context) { /** * Mark the Signal_context as already pending to prevent the receiver @@ -86,24 +86,27 @@ void Signal_receiver::_platform_begin_dissolve(Signal_context * const c) * processing */ { - Mutex::Guard context_guard(c->_mutex); - c->_pending = true; - c->_curr_signal = Signal::Data(); + Mutex::Guard context_guard(context._mutex); + context._pending = true; + context._curr_signal = Signal::Data(); } - Kernel::kill_signal_context(Capability_space::capid(c->_cap)); + Kernel::kill_signal_context(Capability_space::capid(context._cap)); } -void Signal_receiver::_platform_finish_dissolve(Signal_context *) { } +void Signal_receiver::_platform_finish_dissolve(Signal_context &) { } -Signal_context_capability Signal_receiver::manage(Signal_context * const c) +Signal_context_capability Signal_receiver::manage(Signal_context &context) { /* ensure that the context isn't managed already */ Mutex::Guard contexts_guard(_contexts_mutex); - Mutex::Guard context_guard(c->_mutex); - if (c->_receiver) - throw Context_already_in_use(); + Mutex::Guard context_guard(context._mutex); + + if (context._receiver) { + error("ill-attempt to manage an already managed signal context"); + return context._cap; + } Signal_receiver &this_receiver = *this; @@ -115,13 +118,13 @@ Signal_context_capability Signal_receiver::manage(Signal_context * const c) using Error = Pd_session::Alloc_context_error; /* use pointer to signal context as imprint */ - Pd_session::Imprint const imprint { addr_t(c) }; + Pd_session::Imprint const imprint { addr_t(&context) }; _pd.alloc_context(_cap, imprint).with_result( [&] (Capability cap) { - c->_cap = cap; - c->_receiver = &this_receiver; - _contexts.insert_as_tail(c); + context._cap = cap; + context._receiver = &this_receiver; + _contexts.insert_as_tail(&context); }, [&] (Error e) { switch (e) { @@ -138,8 +141,8 @@ Signal_context_capability Signal_receiver::manage(Signal_context * const c) } }); - if (c->_cap.valid()) - return c->_cap; + if (context._cap.valid()) + return context._cap; env().upgrade(Parent::Env::pd(), String<100>("ram_quota=", ram_upgrade, ", " diff --git a/repos/base-hw/src/test/cpu_quota/main.cc b/repos/base-hw/src/test/cpu_quota/main.cc index ead6ea5fb8..24523c69eb 100644 --- a/repos/base-hw/src/test/cpu_quota/main.cc +++ b/repos/base-hw/src/test/cpu_quota/main.cc @@ -30,9 +30,9 @@ struct Single_signal Signal_context_capability cap; Signal_transmitter transmitter; - Single_signal() : cap(receiver.manage(&context)), transmitter(cap) { } + Single_signal() : cap(receiver.manage(context)), transmitter(cap) { } - ~Single_signal() { receiver.dissolve(&context); } + ~Single_signal() { receiver.dissolve(context); } void receive() { receiver.wait_for_signal(); } void submit() { transmitter.submit(); } }; diff --git a/repos/base/include/base/signal.h b/repos/base/include/base/signal.h index 12463f7782..427a3731e5 100644 --- a/repos/base/include/base/signal.h +++ b/repos/base/include/base/signal.h @@ -343,7 +343,7 @@ class Genode::Signal_receiver : Noncopyable * and 'dissolve'. Note that '_contexts_mutex' must be held when * calling this method. */ - void _unsynchronized_dissolve(Signal_context *context); + void _unsynchronized_dissolve(Signal_context &); /** * Hook to platform specific destructor parts @@ -353,25 +353,12 @@ class Genode::Signal_receiver : Noncopyable /** * Hooks to platform specific dissolve parts */ - void _platform_begin_dissolve(Signal_context * const c); - void _platform_finish_dissolve(Signal_context * const c); + void _platform_begin_dissolve (Signal_context &); + void _platform_finish_dissolve(Signal_context &); public: - /** - * Exception class - */ - class Context_already_in_use { }; - class Context_not_associated { }; - - /** - * Constructor - */ Signal_receiver(); - - /** - * Destructor - */ ~Signal_receiver(); /** @@ -379,19 +366,17 @@ class Genode::Signal_receiver : Noncopyable * * \param context context associated with signals delivered to the * receiver - * \throw 'Context_already_in_use' * \return new signal-context capability that can be * passed to a signal transmitter */ - Signal_context_capability manage(Signal_context *context); + Signal_context_capability manage(Signal_context &context); /** * Dissolve signal context from receiver * * \param context context to remove from receiver - * \throw 'Context_not_associated' */ - void dissolve(Signal_context *context); + void dissolve(Signal_context &context); /** * Block until a signal is received and return the signal @@ -433,7 +418,7 @@ class Genode::Signal_receiver : Noncopyable * source associated with the process. It must not be used for other * purposes. */ - static void dispatch_signals(Signal_source *); + static void dispatch_signals(Signal_source &); }; diff --git a/repos/base/include/timer_session/connection.h b/repos/base/include/timer_session/connection.h index b90632216c..858fc9853c 100644 --- a/repos/base/include/timer_session/connection.h +++ b/repos/base/include/timer_session/connection.h @@ -186,7 +186,7 @@ class Timer::Connection : public Genode::Connection, Genode::Signal_context _default_sigh_ctx { }; Genode::Signal_context_capability - _default_sigh_cap = _sig_rec.manage(&_default_sigh_ctx); + _default_sigh_cap = _sig_rec.manage(_default_sigh_ctx); Genode::Signal_context_capability _custom_sigh_cap { }; @@ -266,7 +266,7 @@ class Timer::Connection : public Genode::Connection, Connection(Genode::Env &env, Label const &label = Label()) : Connection(env, env.ep(), label) { } - ~Connection() { _sig_rec.dissolve(&_default_sigh_ctx); } + ~Connection() { _sig_rec.dissolve(_default_sigh_ctx); } /* * Intercept 'sigh' to keep track of customized signal handlers diff --git a/repos/base/lib/symbols/ld b/repos/base/lib/symbols/ld index 406dc0362f..e2814e98a4 100644 --- a/repos/base/lib/symbols/ld +++ b/repos/base/lib/symbols/ld @@ -124,8 +124,8 @@ _ZN6Genode15Signal_receiver12local_submitENS_6Signal4DataE T _ZN6Genode15Signal_receiver14pending_signalEv T _ZN6Genode15Signal_receiver15wait_for_signalEv T _ZN6Genode15Signal_receiver16block_for_signalEv T -_ZN6Genode15Signal_receiver6manageEPNS_14Signal_contextE T -_ZN6Genode15Signal_receiver8dissolveEPNS_14Signal_contextE T +_ZN6Genode15Signal_receiver6manageERNS_14Signal_contextE T +_ZN6Genode15Signal_receiver8dissolveERNS_14Signal_contextE T _ZN6Genode15Signal_receiverC1Ev T _ZN6Genode15Signal_receiverC2Ev T _ZN6Genode15Signal_receiverD1Ev T diff --git a/repos/base/src/core/signal_receiver.cc b/repos/base/src/core/signal_receiver.cc index c332eab62f..c1f97c7855 100644 --- a/repos/base/src/core/signal_receiver.cc +++ b/repos/base/src/core/signal_receiver.cc @@ -42,8 +42,8 @@ Signal_receiver::Signal_receiver() : _pd(*_pd_ptr) void Signal_receiver::_platform_destructor() { } -void Signal_receiver::_platform_begin_dissolve (Signal_context *) { } -void Signal_receiver::_platform_finish_dissolve(Signal_context *) { } +void Signal_receiver::_platform_begin_dissolve (Signal_context &) { } +void Signal_receiver::_platform_finish_dissolve(Signal_context &) { } void Signal_receiver::unblock_signal_waiter(Rpc_entrypoint &) { ASSERT_NEVER_CALLED; } @@ -52,7 +52,7 @@ void Signal_receiver::unblock_signal_waiter(Rpc_entrypoint &) { ASSERT_NEVER_CAL typedef Signal_context_capability Sigh_cap; -Sigh_cap Signal_receiver::manage(Signal_context *) { ASSERT_NEVER_CALLED; } +Sigh_cap Signal_receiver::manage(Signal_context &) { ASSERT_NEVER_CALLED; } void Signal_receiver::block_for_signal() diff --git a/repos/base/src/include/base/internal/expanding_parent_client.h b/repos/base/src/include/base/internal/expanding_parent_client.h index a2ff3f6b74..fe59a35887 100644 --- a/repos/base/src/include/base/internal/expanding_parent_client.h +++ b/repos/base/src/include/base/internal/expanding_parent_client.h @@ -90,7 +90,7 @@ class Genode::Expanding_parent_client : public Parent_client { if (!_fallback_sig_cap.valid()) { _fallback_sig_rcv.construct(); - _fallback_sig_cap = _fallback_sig_rcv->manage(&_fallback_sig_ctx); + _fallback_sig_cap = _fallback_sig_rcv->manage(_fallback_sig_ctx); } } diff --git a/repos/base/src/lib/base/component.cc b/repos/base/src/lib/base/component.cc index 5903126c0b..ba4a132460 100644 --- a/repos/base/src/lib/base/component.cc +++ b/repos/base/src/lib/base/component.cc @@ -68,7 +68,7 @@ struct Genode::Component_env : Env Blockade(Parent &parent) : _parent(parent) { - _parent.session_sigh(_sig_rec.manage(&_sig_ctx)); + _parent.session_sigh(_sig_rec.manage(_sig_ctx)); } void block() { _sig_rec.wait_for_signal(); } diff --git a/repos/base/src/lib/base/entrypoint.cc b/repos/base/src/lib/base/entrypoint.cc index 80fb8ee2bc..fae5c53aa9 100644 --- a/repos/base/src/lib/base/entrypoint.cc +++ b/repos/base/src/lib/base/entrypoint.cc @@ -220,7 +220,7 @@ bool Entrypoint::_wait_and_dispatch_one_io_signal(bool const dont_block) Signal_context_capability Entrypoint::manage(Signal_dispatcher_base &dispatcher) { /* _sig_rec is invalid for a small window in _process_incoming_signals */ - return _sig_rec.constructed() ? _sig_rec->manage(&dispatcher) + return _sig_rec.constructed() ? _sig_rec->manage(dispatcher) : Signal_context_capability(); } @@ -229,7 +229,7 @@ void Genode::Entrypoint::dissolve(Signal_dispatcher_base &dispatcher) { /* _sig_rec is invalid for a small window in _process_incoming_signals */ if (_sig_rec.constructed()) - _sig_rec->dissolve(&dispatcher); + _sig_rec->dissolve(dispatcher); /* also remove context from deferred signal list */ { diff --git a/repos/base/src/lib/base/signal.cc b/repos/base/src/lib/base/signal.cc index 7b8e6f4230..e9996cd425 100644 --- a/repos/base/src/lib/base/signal.cc +++ b/repos/base/src/lib/base/signal.cc @@ -48,7 +48,7 @@ class Signal_handler_thread : Thread, Blockade { _signal_source.construct(_cpu, _pd.alloc_signal_source()); wakeup(); - Signal_receiver::dispatch_signals(&(*_signal_source)); + Signal_receiver::dispatch_signals(*_signal_source); } enum { STACK_SIZE = 4*1024*sizeof(addr_t) }; @@ -214,12 +214,12 @@ Signal_receiver::Signal_receiver() : _pd(*_pd_ptr) } -Signal_context_capability Signal_receiver::manage(Signal_context *context_ptr) +Signal_context_capability Signal_receiver::manage(Signal_context &context) { - Signal_context &context = *context_ptr; - - if (context._receiver) - throw Context_already_in_use(); + if (context._receiver) { + error("ill-attempt to manage an already managed signal context"); + return context._cap; + } context._receiver = this; @@ -340,10 +340,10 @@ void Signal_receiver::local_submit(Signal::Data data) } -void Signal_receiver::dispatch_signals(Signal_source *signal_source) +void Signal_receiver::dispatch_signals(Signal_source &signal_source) { for (;;) { - Signal_source::Signal source_signal = signal_source->wait_for_signal(); + Signal_source::Signal source_signal = signal_source.wait_for_signal(); /* look up context as pointed to by the signal imprint */ Signal_context *context = (Signal_context *)(source_signal.imprint()); @@ -372,18 +372,18 @@ void Signal_receiver::dispatch_signals(Signal_source *signal_source) } -void Signal_receiver::_platform_begin_dissolve(Signal_context *context) +void Signal_receiver::_platform_begin_dissolve(Signal_context &context) { /* * Because the 'remove' operation takes the registry mutex, the context * must not be acquired when calling this method. See the comment in * 'Signal_receiver::dissolve'. */ - signal_context_registry()->remove(&context->_registry_le); + signal_context_registry()->remove(&context._registry_le); } -void Signal_receiver::_platform_finish_dissolve(Signal_context *) { } +void Signal_receiver::_platform_finish_dissolve(Signal_context &) { } void Signal_receiver::_platform_destructor() { } diff --git a/repos/base/src/lib/base/signal_common.cc b/repos/base/src/lib/base/signal_common.cc index 9d842f6042..567c2dbb54 100644 --- a/repos/base/src/lib/base/signal_common.cc +++ b/repos/base/src/lib/base/signal_common.cc @@ -164,34 +164,36 @@ Signal_receiver::~Signal_receiver() Mutex::Guard contexts_guard(_contexts_mutex); /* disassociate contexts from the receiver */ - while (Signal_context *context = _contexts.head()) { - _platform_begin_dissolve(context); - _unsynchronized_dissolve(context); - _platform_finish_dissolve(context); + while (Signal_context *context_ptr = _contexts.head()) { + _platform_begin_dissolve(*context_ptr); + _unsynchronized_dissolve(*context_ptr); + _platform_finish_dissolve(*context_ptr); } _platform_destructor(); } -void Signal_receiver::_unsynchronized_dissolve(Signal_context * const context) +void Signal_receiver::_unsynchronized_dissolve(Signal_context &context) { /* tell core to stop sending signals referring to the context */ - _pd.free_context(context->_cap); + _pd.free_context(context._cap); /* restore default initialization of signal context */ - context->_receiver = nullptr; - context->_cap = Signal_context_capability(); + context._receiver = nullptr; + context._cap = Signal_context_capability(); /* remove context from context list */ - _contexts.remove(context); + _contexts.remove(&context); } -void Signal_receiver::dissolve(Signal_context *context) +void Signal_receiver::dissolve(Signal_context &context) { - if (context->_receiver != this) - throw Context_not_associated(); + if (context._receiver != this) { + error("ill-attempt to dissolve unmanaged signal context"); + return; + } { /* @@ -207,14 +209,14 @@ void Signal_receiver::dissolve(Signal_context *context) _platform_begin_dissolve(context); - Mutex::Guard context_guard(context->_mutex); + Mutex::Guard context_guard(context._mutex); _unsynchronized_dissolve(context); } _platform_finish_dissolve(context); - Mutex::Guard context_destroy_guard(context->_destroy_mutex); + Mutex::Guard context_destroy_guard(context._destroy_mutex); } diff --git a/repos/os/include/audio_in_session/client.h b/repos/os/include/audio_in_session/client.h index da3822b243..4432068524 100644 --- a/repos/os/include/audio_in_session/client.h +++ b/repos/os/include/audio_in_session/client.h @@ -33,8 +33,8 @@ struct Audio_in::Signal Genode::Signal_context context { }; Genode::Signal_context_capability cap; - Signal() : cap(recv.manage(&context)) { } - ~Signal() { recv.dissolve(&context); } + Signal() : cap(recv.manage(context)) { } + ~Signal() { recv.dissolve(context); } void wait() { recv.wait_for_signal(); } }; diff --git a/repos/os/include/audio_out_session/client.h b/repos/os/include/audio_out_session/client.h index 1ebab1369c..d0273ee2c3 100644 --- a/repos/os/include/audio_out_session/client.h +++ b/repos/os/include/audio_out_session/client.h @@ -31,8 +31,8 @@ struct Audio_out::Signal Genode::Signal_context context { }; Genode::Signal_context_capability cap; - Signal() : cap(recv.manage(&context)) { } - ~Signal() { recv.dissolve(&context); } + Signal() : cap(recv.manage(context)) { } + ~Signal() { recv.dissolve(context); } void wait() { recv.wait_for_signal(); } }; diff --git a/repos/os/include/terminal_session/connection.h b/repos/os/include/terminal_session/connection.h index d377e2b85d..a7d5b49738 100644 --- a/repos/os/include/terminal_session/connection.h +++ b/repos/os/include/terminal_session/connection.h @@ -34,14 +34,14 @@ struct Terminal::Connection : Genode::Connection, Session_client /* create signal receiver, just for the single signal */ Signal_context sig_ctx; Signal_receiver sig_rec; - Signal_context_capability sig_cap = sig_rec.manage(&sig_ctx); + Signal_context_capability sig_cap = sig_rec.manage(sig_ctx); /* register signal handler */ cap.call(sig_cap); /* wati for signal */ sig_rec.wait_for_signal(); - sig_rec.dissolve(&sig_ctx); + sig_rec.dissolve(sig_ctx); } Connection(Genode::Env &env, Label const &label = Label()) diff --git a/repos/os/src/test/gpio_signal/main.cc b/repos/os/src/test/gpio_signal/main.cc index c0ba9a11dc..db604586ed 100644 --- a/repos/os/src/test/gpio_signal/main.cc +++ b/repos/os/src/test/gpio_signal/main.cc @@ -67,7 +67,7 @@ Main::Main(Env &env) : env(env) * Initialize the pin IRQ signal */ Irq_session_client irq(signal_input.irq_session(Gpio::Session::HIGH_LEVEL)); - irq.sigh(sig_rec.manage(&sig_ctx)); + irq.sigh(sig_rec.manage(sig_ctx)); irq.ack_irq(); while(true) diff --git a/repos/os/src/test/signal/main.cc b/repos/os/src/test/signal/main.cc index 4cd1e9108a..b2c8a2346f 100644 --- a/repos/os/src/test/signal/main.cc +++ b/repos/os/src/test/signal/main.cc @@ -134,12 +134,12 @@ class Handler : Thread ~Handler() { Signal_context context; - Signal_context_capability context_cap { _receiver.manage(&context) }; + Signal_context_capability context_cap { _receiver.manage(context) }; _stop = true; Signal_transmitter(context_cap).submit(); Thread::join(); - _receiver.dissolve(&context); + _receiver.dissolve(context); } void print(Output &output) const { Genode::print(output, "handler ", _id); } @@ -185,7 +185,7 @@ struct Fast_sender_test : Signal_test Signal_context context { }; Signal_receiver receiver { }; Handler handler { env, receiver, HANDLER_INTERVAL_MS, false, 1 }; - Sender sender { env, receiver.manage(&context), + Sender sender { env, receiver.manage(context), SENDER_INTERVAL_MS, false }; Fast_sender_test(Env &env, int id) : Signal_test(id, brief), env(env) @@ -218,7 +218,7 @@ struct Stress_test : Signal_test Signal_context context { }; Signal_receiver receiver { }; Handler handler { env, receiver, 0, false, 1 }; - Sender sender { env, receiver.manage(&context), 0, false }; + Sender sender { env, receiver.manage(context), 0, false }; Stress_test(Env &env, int id) : Signal_test(id, brief), env(env) { @@ -254,8 +254,8 @@ struct Lazy_receivers_test : Signal_test Signal_context context_1 { }, context_2 { }; Signal_receiver receiver_1 { }, receiver_2 { }; - Signal_transmitter transmitter_1 { receiver_1.manage(&context_1) }; - Signal_transmitter transmitter_2 { receiver_2.manage(&context_2) }; + Signal_transmitter transmitter_1 { receiver_1.manage(context_1) }; + Signal_transmitter transmitter_2 { receiver_2.manage(context_2) }; Lazy_receivers_test(Env &, int id) : Signal_test(id, brief) { @@ -291,7 +291,7 @@ struct Context_management_test : Signal_test Timer::Connection timer { env }; Signal_context context { }; Signal_receiver receiver { }; - Signal_context_capability context_cap { receiver.manage(&context) }; + Signal_context_capability context_cap { receiver.manage(context) }; Sender sender { env, context_cap, 500, true }; Context_management_test(Env &env, int id) : Signal_test(id, brief), env(env) @@ -306,7 +306,7 @@ struct Context_management_test : Signal_test Signal signal = receiver.wait_for_signal(); log("got ", signal.num(), " signal(s) from ", signal.context()); } - receiver.dissolve(&context); + receiver.dissolve(context); /* let sender spin for some time */ log("resume sender"); @@ -330,12 +330,12 @@ struct Synchronized_destruction_test : private Signal_test, Thread Heap heap { env.ram(), env.rm() }; Signal_context &context { *new (heap) Signal_context }; Signal_receiver receiver { }; - Signal_transmitter transmitter { receiver.manage(&context) }; + Signal_transmitter transmitter { receiver.manage(context) }; bool destroyed { false }; void entry() override { - receiver.dissolve(&context); + receiver.dissolve(context); log("dissolve finished"); destroyed = true; destroy(heap, &context); @@ -380,11 +380,11 @@ struct Many_contexts_test : Signal_test Signal_receiver receiver; for (unsigned i = 0; i < nr_of_contexts; i++) { - if (!receiver.manage(new (heap) Registered(contexts)).valid()) { + if (!receiver.manage(*new (heap) Registered(contexts)).valid()) { throw Manage_failed(); } } contexts.for_each([&] (Registered &context) { - receiver.dissolve(&context); + receiver.dissolve(context); destroy(heap, &context); }); } diff --git a/repos/os/src/test/terminal_crosslink/main.cc b/repos/os/src/test/terminal_crosslink/main.cc index a1157013e7..1c790948d5 100644 --- a/repos/os/src/test/terminal_crosslink/main.cc +++ b/repos/os/src/test/terminal_crosslink/main.cc @@ -81,7 +81,7 @@ class Test_terminal_crosslink::Partner : public Thread : Thread(env, name, STACK_SIZE), _terminal(env) { - _terminal.read_avail_sigh(_sig_rec.manage(&_sig_ctx)); + _terminal.read_avail_sigh(_sig_rec.manage(_sig_ctx)); } };