supervision - discussion about system services, daemon supervision, init, runlevel management, and tools such as s6 and runit
 help / color / mirror / Atom feed
From: Adam Joseph <adam@westernsemico.com>
To: supervision@list.skarnet.org
Cc: Adam Joseph <adam@westernsemico.com>
Subject: [PATCH 3/3] [WIP] s6-rc-update.c: add additional comments
Date: Mon, 25 Sep 2023 17:44:18 -0700	[thread overview]
Message-ID: <20230926004418.24990-3-adam@westernsemico.com> (raw)
In-Reply-To: <20230926004418.24990-1-adam@westernsemico.com>

This commit adds additional explanatory comments for parts of
s6-rc-update.c that did not seem immediately obvious to me.

It is probably not appropriate to apply this commit verbatim in its current
form.  Feel free to pick changes from it selectively.

Signed-off-by: Adam Joseph <adam@westernsemico.com>
---
 src/s6-rc/s6-rc-update.c | 43 ++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/src/s6-rc/s6-rc-update.c b/src/s6-rc/s6-rc-update.c
index e0726a2..240a256 100644
--- a/src/s6-rc/s6-rc-update.c
+++ b/src/s6-rc/s6-rc-update.c
@@ -51,6 +51,7 @@ static unsigned int verbosity = 1 ;
 #define OLDSTATE_WAS_UP 1
 #define OLDSTATE_INCLUDE_IN_DOWN_TRANSITION 2
 #define OLDSTATE_RESTART 4
+/* or, equivalently, "converts to a multi-element bundle" */
 #define OLDSTATE_CONVERTS_TO_ATOMIC_OR_SINGLETON_BUNDLE 8
 #define OLDSTATE_HAS_NEW_NAME 16
 #define OLDSTATE_WANT_DOWN_OR_RESTART 32
@@ -60,6 +61,11 @@ static unsigned int verbosity = 1 ;
 #define NEWSTATE_WAS_UP 1
 #define NEWSTATE_INCLUDE_IN_UP_TRANSITION 2
 #define NEWSTATE_IS_BIJECTIVE_CONVERSION_TARGET 4
+/*
+  this flag is set for:
+  - anything which is the target of a rename in the convfile
+  - the contents of bundles which are the targets of renames
+*/
 #define NEWSTATE_IS_CONVERSION_TARGET 8
 #define NEWSTATE_CHANGED_NAMES 16
 #define NEWSTATE_WANT_UP_AFTER_CLOSURE_BITNO 5
@@ -183,6 +189,12 @@ static inline void fill_convtable_and_flags (unsigned char *conversion_table, un
       if (newstate[x] & NEWSTATE_IS_CONVERSION_TARGET)
         strerr_diefu4x(6, "convert database: new service ", newdb->string + newdb->services[x].name, " is a target for more than one conversion, including old service ", olddb->string + olddb->services[i].name) ;
 
+      /*
+        Note: the following will fail if you have a singleton bundle
+        and try to rename both the bundle and the thing it contains
+        in the same s6-rc-update invocation.  Parts of this loop
+        should probably be skipped for singleton bundles.
+      */
       newstate[x] |= NEWSTATE_IS_CONVERSION_TARGET ;
       invimage[x] = i ;
       if (oldstate[i] & OLDSTATE_HAS_NEW_NAME) newstate[x] |= NEWSTATE_CHANGED_NAMES ;
@@ -191,6 +203,8 @@ static inline void fill_convtable_and_flags (unsigned char *conversion_table, un
       {
         newstate[x] |= NEWSTATE_IS_BIJECTIVE_CONVERSION_TARGET ;
 
+        /* The following line forces a restart when a oneshot
+           converts to a longrun or vice versa. */
         if ((i < olddb->nlong) != (x < newdb->nlong)) oldstate[i] |= OLDSTATE_RESTART ;
       }
     }
@@ -229,8 +243,9 @@ static void compute_transitions (char const *convfile, unsigned char *oldstate,
           NEWSTATE_CHANGED_NAMES);
 
    /*
-     If an old service needs to restart, mark it wanted down, as well
-     as everything that depends on it.
+     If an old service was up and either needs to restart or
+     converts to an atomic or converts to a singleton, then: mark it
+     wanted down, as well as everything that depends on it.
    */
 
     i = oldn ;
@@ -238,6 +253,11 @@ static void compute_transitions (char const *convfile, unsigned char *oldstate,
     {
       if (oldstate[i] & OLDSTATE_WAS_UP &&
           (oldstate[i] & OLDSTATE_RESTART ||
+           /*
+             The following clause deals with the situation where
+             you convert an atomic or singleton bundle into a
+             multi-element bundle
+           */
            !(oldstate[i] & OLDSTATE_CONVERTS_TO_ATOMIC_OR_SINGLETON_BUNDLE)
            ))
         oldstate[i] |=
@@ -255,7 +275,7 @@ static void compute_transitions (char const *convfile, unsigned char *oldstate,
 
 
    /*
-      Convert the old state to the new state: if an old service is up,
+      Convert the old state to the new state: if an old service was up,
       the new service will be either up or wanted up.
    */
 
@@ -277,6 +297,7 @@ static void compute_transitions (char const *convfile, unsigned char *oldstate,
      restart and loop until there are no new dependencies.
    */
 
+    /* For every service wanted up, propagate that marking to its dependencies. */
     s6rc_graph_closure(newdb, newstate, NEWSTATE_WANT_UP_AFTER_CLOSURE_BITNO, 1) ;
     i = newn ;
     while (i--)
@@ -287,8 +308,12 @@ static void compute_transitions (char const *convfile, unsigned char *oldstate,
       }
     if (done) break ;
 
+    /* For every service marked "depends on a new service", mark
+       services which depend upon the same way */
     s6rc_graph_closure(newdb, newstate, NEWSTATE_DEPENDS_ON_A_NEW_SERVICE_BITNO, 0) ;
 
+    /* For every service which acquires a new dependency (one which
+       did not previously exist), force it to restart. */
     i = newn ;
     while (i--)
       if ((newstate[i] & NEWSTATE_WAS_UP) &&
@@ -401,13 +426,19 @@ static inline void make_new_livedir (unsigned char const *oldstate, s6rc_db_t co
   if (verbosity >= 2) strerr_warni1x("successfully switched to new database") ;
 
  /* scandir cleanup, then old livedir cleanup */
+
   i = olddb->nlong ;
-  while (i--)
+  while (i--) {
+    /* If an old service survived without needing a restart, do not
+       kill its supervisor */
+    unsigned int keepsupervisor =
+      (oldstate[i] & OLDSTATE_WAS_UP) &&
+      !(oldstate[i] & OLDSTATE_WANT_DOWN_OR_RESTART);
     s6rc_servicedir_unsupervise(sa->s,
                                 prefix,
                                 olddb->string + olddb->services[i].name,
-                                (oldstate[i] & OLDSTATE_WAS_UP) &&
-                                !(oldstate[i] & OLDSTATE_WANT_DOWN_OR_RESTART)) ;
+                                keepsupervisor);
+  }
   rm_rf_in_tmp(sa, 0) ;
   sa->len = 0 ;
   return ;
-- 
2.41.0


  parent reply	other threads:[~2023-09-26  0:45 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-26  0:44 [PATCH 1/3] s6-rc-update.c: add #define constants for bitflags Adam Joseph
2023-09-26  0:44 ` [PATCH 2/3] s6-rc-update.c: rewrite O(n^2) loop as O(n) complexity Adam Joseph
2023-09-26  0:57   ` Adam Joseph
2023-09-26  0:44 ` Adam Joseph [this message]
2023-09-26  0:59   ` [PATCH 3/3] [WIP] s6-rc-update.c: add additional comments Adam Joseph

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230926004418.24990-3-adam@westernsemico.com \
    --to=adam@westernsemico.com \
    --cc=supervision@list.skarnet.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).