From b058f99cb8a272a29d52f272dd8154f9511b5b4f Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 21 Feb 2023 12:21:32 -0800 Subject: [PATCH] remove references to obsolete isc_task/timer functions removed references in code comments, doc/dev documentation, etc, to isc_task, isc_timer_reset(), and isc_timertype_inactive. also removed a coccinelle patch related to isc_timer_reset() that was no longer needed. --- cocci/isc_timer_reset.spatch | 6 --- doc/dev/dev.md | 84 ++---------------------------------- doc/dev/loopmgr.md | 41 ++++++------------ doc/dev/results | 2 +- doc/dev/style.md | 26 +++++------ lib/dns/include/dns/view.h | 4 +- lib/dns/include/dns/zone.h | 12 +++--- lib/dns/zone.c | 2 +- lib/isc/include/isc/netmgr.h | 14 +++--- 9 files changed, 43 insertions(+), 148 deletions(-) delete mode 100644 cocci/isc_timer_reset.spatch diff --git a/cocci/isc_timer_reset.spatch b/cocci/isc_timer_reset.spatch deleted file mode 100644 index b89907f46c..0000000000 --- a/cocci/isc_timer_reset.spatch +++ /dev/null @@ -1,6 +0,0 @@ -@@ -expression E1, E2, E3, E4; -@@ - -- isc_timer_reset(E1, E2, NULL, E3, E4) -+ isc_timer_reset(E1, E2, E3, E4) diff --git a/doc/dev/dev.md b/doc/dev/dev.md index 3dbc90618b..22cc05eaa0 100644 --- a/doc/dev/dev.md +++ b/doc/dev/dev.md @@ -32,7 +32,7 @@ information regarding copyright ownership. * [Iterators](#iterators) * [Logging](#logging) * [Adding a new RR type](#rrtype) - * [Task and timer model](#tasks) + * [Asynchronous operations](#async) ### The code review process @@ -1366,86 +1366,10 @@ In nearly all cases, this is simply a wrapper around the `compare()` function, except where DNSSEC comparisons are specified as case-sensitive. Unknown RR types are always compared case-sensitively. -#### Task and timer model +#### Asynchronous operations -The BIND task/timer management system can be thought of as comparable to a -simple non-preemptive multitasking operating system. - -In this model, what BIND calls a "task" (or an `isc_task_t` object) is the -equivalent of what is usually called a "process" in an operating system: -a persistent execution context in which functions run when action is -required. When the action is complete, the task goes to sleep, and -wakes up again when more work arrives. A "worker thread" is comparable -to an operating system's CPU; when a task is ready to run, a worker -thread will run it. By default, BIND creates one worker thread for each -system CPU. - -An "event" object can be associated with a task, and triggered when a a -specific condition occurs. Each event object contains a pointer to a -specific function and arguments to be passed to it. When the event -triggers, the task is placed onto a "ready queue" in the task manager. As -each running task finishes, the worker threads pull new tasks off the ready -queue. When the task associated with a given event reaches the head of the -queue, the specified function will be called. - -Examples: - -A timer is set for a specified time in the future, and the event will -be triggered at that time. - - /* - * Function to handle a timeout - */ - static void - timeout(isc_task_t *task, isc_event_t *event) { - /* do things */ - } - - ... - - /* - * Set up a timer in timer manager 'timermgr', to run - * once, with a NULL expiration time, after 'interval' - * has passed; it will run the function 'timeout' with - * 'arg' as its argument in task 'task'. - */ - isc_timer_t *timer = NULL; - result = isc_timer_create(timermgr, task, timeout, arg, &timer); - result = isc_timer_reset(timermgr, isc_timertype_once, NULL, - interval, false); - -An event can also be explicitly triggered via `isc_task_send()`. - - static void - do_things(isc_task_t *task, isc_event_t *event) { - /* this function does things */ - } - - ... - - /* - * Allocate an event that calls 'do_things' with a - * NULL argument, using 'myself' as ev_sender. - * - * DNS_EVENT_DOTHINGS must be defined in . - * - * (Note that 'size' must be specified because there are - * event objects that inherit from isc_event_t, incorporating - * common members via the ISC_EVENT_COMMON member and then - * following them with other members.) - */ - isc_event_t *event; - event = isc_event_allocate(mctx, myself, DNS_EVENT_DOTHINGS, - do_things, NULL, sizeof(isc_event_t)); - if (event == NULL) - return (ISC_R_NOMEMORY); - - ... - - /* - * Send the allocated event to task 'task' - */ - isc_task_send(task, event); +Asynchronous operations are processed using the event loop manager; +see the [Loop Manager](loopmgr.md) document for details. #### More... diff --git a/doc/dev/loopmgr.md b/doc/dev/loopmgr.md index 15831e6236..8f6599b544 100644 --- a/doc/dev/loopmgr.md +++ b/doc/dev/loopmgr.md @@ -79,38 +79,25 @@ need to run the event on a different thread. manager, uses locked list to collect new jobs and uv_async() primitive to enqueue the collected jobs onto the event loop. -## Tasks - -The ``isc_task`` API has been modified to run the tasks directly on the loop -manager. The new ``isc_job`` and ``isc_async`` APIs are preferred for simple -events; the ``isc_task`` API is provided for backward-compatibility purposes -and thus is also thread-safe because it uses locking and uv_async() to enqueue -events onto the event loop. - ## Timers -The ``isc_timer`` API is now built on top of the ``uv_timer_t`` object. It has -been changed to support only ``ticker`` and ``once`` timers, and now uses -``isc_timer_start()`` and ``isc_timer_stop()`` instead of changing the timer -type to ``inactive``. The ``isc_timer_t`` object is not thread-safe. +The ``isc_timer`` API is built on top of the ``uv_timer_t`` object. +It supports ``ticker`` and ``once`` timers, and uses ``isc_timer_start()`` +and ``isc_timer_stop()`` to start and stop timers. The ``isc_timer_t`` +object is not thread-safe. ## Network Manager -The network manager has been changed to use the loop manager event loops -instead of managing its own event loops. - -The new network manager calls are not thread-safe; all connect/read/write -functions MUST be called from the thread that created the network manager -socket. +The network manager is built on top loop manager event loops rather than +managing its own event loops. Network manager calls are not thread-safe: +all connect/read/write functions MUST be called from the thread that created +the network manager socket. The ``isc_nm_listen*()`` functions MUST be called from the ``main`` loop. -The general design of Network Manager is based on callbacks. An extra care must -be taken when implementing new functions because the callbacks MUST be called -asynchronously because the caller might be inside a lock and the same lock must -be acquired in the callback. This doesn't mean that the callback must be always -called asynchronously, because sometimes we are already in the libuv callback -and thus we can just call the callback directly, but in other places, especially -when returning an error, the control hasn't been returned to the caller yet and -in such case, the callback must be scheduled onto the event loop instead of -executing it directly. +The general design of Network Manager is based on callbacks. Callback +functions should always be called asynchronously by the network manager, +because the caller might be holding a lock, and the callback may try to +acquire the same lock. So even if a network manager function is able to +determine a result immediately, the callback must be scheduled onto the +event loop instead of executed directly. diff --git a/doc/dev/results b/doc/dev/results index 38a2d38f9a..2bdeb29a8e 100644 --- a/doc/dev/results +++ b/doc/dev/results @@ -46,7 +46,7 @@ result information as the return value of the function. E.g. isc_result_t result; - result = isc_task_send(task, &event); + result = isc_stdio_open(filename, 0644, &fp); Note that an explicit result type is used, instead of mixing the error result type with the normal result type. E.g. the C library routine getc() can diff --git a/doc/dev/style.md b/doc/dev/style.md index f4b79c5af5..5537f4c480 100644 --- a/doc/dev/style.md +++ b/doc/dev/style.md @@ -764,24 +764,18 @@ described for the public interfaces, except `{library}` and `{module}` are separated by a double-underscore. This indicates that the name is internal, its API is not as formal as the public API, and thus it might change without any sort of notice. Examples of this usage include -`dns__zone_loadpending()` and `isc__taskmgr_ready()`. +`dns__zone_loadpending()` and `isc__mem_printallactive()`. -In many cases, a public interface is instantiated by a private back-end -implementation. The double-underscore naming style is sometimes used in -that situation; for example, `isc_task_attach()` calls the `attach` -function provided by a task API implementation; in BIND 9, this function -is provided by `isc__task_attach()`. - -Other times, private interface implementations are static functions -that are pointed to by "method" tables. For example, the `dns_db` -interface is implemented in several places, including `lib/dns/rbtdb.c` -(the red-black tree database used for internal storage of zones and -cache data) and `lib/dns/sdlz.c` (an interface to DLZ modules). +In some cases, a public interface is instantiated by a private back-end +implementation. The private interface implementations are typically +static functions that are pointed to by "method" tables. For example, +the `dns_db` interface is implemented in several places, including +`lib/dns/rbtdb.c` (the red-black tree database used for internal storage of +zones and cache data) and `lib/dns/sdlz.c` (an interface to DLZ modules). An object of type `dns_dbmethods_t` is created for each of these, -containing function pointers to the local implementations of each -of the `dns_db` API functions. The `dns_db_findnode()` function -is provided by static functions called `findnode()` in each file, -and so on. +containing function pointers to the local implementations of each of the +`dns_db` API functions. The `dns_db_findnode()` function is provided by +static functions called `findnode()` in each file, and so on. #### Initialization diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index 30d1a174da..e38a18dd72 100644 --- a/lib/dns/include/dns/view.h +++ b/lib/dns/include/dns/view.h @@ -530,8 +530,8 @@ void dns_view_thaw(dns_view_t *view); /*%< * Thaw view. This allows zones to be added or removed at runtime. This is - * NOT thread-safe; the caller MUST have run isc_task_exclusive() prior to - * thawing the view. + * NOT thread-safe; the caller MUST have paused the loopmgr prior to thawing + * the view. * * Requires: * diff --git a/lib/dns/include/dns/zone.h b/lib/dns/include/dns/zone.h index 4969c91085..f9f2fb582b 100644 --- a/lib/dns/include/dns/zone.h +++ b/lib/dns/include/dns/zone.h @@ -1133,17 +1133,17 @@ dns_zone_clearxfracl(dns_zone_t *zone); bool dns_zone_getupdatedisabled(dns_zone_t *zone); /*%< - * Return update disabled. - * Transient unless called when running in isc_task_exclusive() mode. + * Return true if updates are disabled. */ void dns_zone_setupdatedisabled(dns_zone_t *zone, bool state); /*%< - * Set update disabled. - * Should only be called only when running in isc_task_exclusive() mode. - * Failure to do so may result in updates being committed after the - * call has been made. + * Enable or disable updates. + * + * This should only be called when running in exclusive mode; + * otherwise, updates that were already in progress could be + * committed after disabling. */ bool diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 59237daf8d..e60b7547d2 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -14583,7 +14583,7 @@ zone_timer(void *arg) { static void zone_timer_stop(dns_zone_t *zone) { - zone_debuglog(zone, __func__, 10, "settimer inactive"); + zone_debuglog(zone, __func__, 10, "stop zone timer"); if (zone->timer != NULL) { isc_timer_stop(zone->timer); } diff --git a/lib/isc/include/isc/netmgr.h b/lib/isc/include/isc/netmgr.h index 2221897a9a..9201dee880 100644 --- a/lib/isc/include/isc/netmgr.h +++ b/lib/isc/include/isc/netmgr.h @@ -170,16 +170,12 @@ isc__nmhandle_attach(isc_nmhandle_t *handle, isc_nmhandle_t **dest FLARG); void isc__nmhandle_detach(isc_nmhandle_t **handlep FLARG); /*%< - * Increment/decrement the reference counter in a netmgr handle, - * but (unlike the attach/detach functions) do not change the pointer - * value. If reference counters drop to zero, the handle can be - * marked inactive, possibly triggering deletion of its associated - * socket. + * Increment/decrement the reference counter in a netmgr handle. * - * (This will be used to prevent a client from being cleaned up when - * it's passed to an isc_task event handler. The libuv code would not - * otherwise know that the handle was in use and might free it, along - * with the client.) + * When the detach function is called on a thread other than the one that + * created the handle, it is scheduled to asynchronously by the handle's + * event loop. When references go to zero, the associated socket will be + * closed and deleted. */ #undef FLARG