From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, MAILING_LIST_MULTI autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 6140 invoked from network); 26 Sep 2023 00:45:04 -0000 Received: from alyss.skarnet.org (95.142.172.232) by inbox.vuxu.org with ESMTPUTF8; 26 Sep 2023 00:45:04 -0000 Received: (qmail 54289 invoked by uid 89); 26 Sep 2023 00:45:29 -0000 Mailing-List: contact supervision-help@list.skarnet.org; run by ezmlm Sender: Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Received: (qmail 54282 invoked from network); 26 Sep 2023 00:45:29 -0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=westernsemico.com; s=default; h=Content-Transfer-Encoding:MIME-Version: Message-ID:Date:Subject:Cc:To:From:Sender:Reply-To:Content-Type:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:In-Reply-To:References:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=WwfCgBncd71v9rHRzGo6g9Ni/BrOBeibCrKgmM+oOvc=; b=CZ2e9tptA00P7oRRzRCZVHb3Ew 1nRmvvHnJHOrEsFNQWB/3o1ksjKwafn48fMuXbFLXXswqYYvjbLC0ex1FEsnkbgS6sWY7ze47SyIN YlJ9sqnPSBVydLzeJbS2sM8KfwLvD2B+QQTfo5Hw1mcyqHOBBepeOH7MgKUCqeDAkpwhF1ETTUeIT jxognwXX+U2AuhaTTC/qMoUuN9BsAisBMUKRaWJ/cRDW/oj3/mOYZpYj8WuhVkeQbCt3u3LhXqhDn UjKhvKliID4ODbNU6Z4Ek0u8XzkblKvKeIoXTL7eAtXQstzWpioy5YAl06Q50gBwTQ3porU1rX0AK z64+8mWw==; From: Adam Joseph To: supervision@list.skarnet.org Cc: Adam Joseph Subject: [PATCH 1/3] s6-rc-update.c: add #define constants for bitflags Date: Mon, 25 Sep 2023 17:44:16 -0700 Message-ID: <20230926004418.24990-1-adam@westernsemico.com> X-Mailer: git-send-email 2.41.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - server220.web-hosting.com X-AntiAbuse: Original Domain - list.skarnet.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - westernsemico.com X-Get-Message-Sender-Via: server220.web-hosting.com: authenticated_id: westwhdn/from_h X-Authenticated-Sender: server220.web-hosting.com: adam@westernsemico.com X-Source: X-Source-Args: X-Source-Dir: X-From-Rewrite: unmodified, already matched The documentation for s6-rc-update says "Live upgrading a service database is not easy, and no fully automated system can get it right in all cases." This is certainly true! I reviewed the source code in order to be sure I fully understood the algorithm. I found it much easier to do this after replacing all of the "Conversions and transitions" bitflags with descriptive #define identifiers. For example this routine, which had no comment, was quite difficult to understand: if (oldstate[i] & 1 && (oldstate[i] & 4 || !(oldstate[i] & 8))) oldstate[i] |= 34 ; else oldstate[i] &= 221 ; Here's what these two lines of code do: if a service was up in the old state and either shall be restarted or else converts to a non-singleton bundle, set the two flags 'included in the down transition' and 'wanted down or restarted'; otherwise clear those two flags. When the integers above are replaced by symbolic identifiers, the code now conveys exactly what it does: if (oldstate[i] & OLDSTATE_WAS_UP && (oldstate[i] & OLDSTATE_RESTART || !(oldstate[i] & OLDSTATE_CONVERTS_TO_ATOMIC_OR_SINGLETON_BUNDLE) )) oldstate[i] |= OLDSTATE_WANT_DOWN_OR_RESTART | OLDSTATE_INCLUDE_IN_DOWN_TRANSITION; else oldstate[i] &= ~(OLDSTATE_WANT_DOWN_OR_RESTART | OLDSTATE_INCLUDE_IN_DOWN_TRANSITION); This commit has been written to do nothing other than add the descriptive identifiers and replace integers with their corresponding identifier-combinations. Therefore it should be straightforward to locally verify the correctness of each change in this commit. Signed-off-by: Adam Joseph --- src/s6-rc/s6-rc-update.c | 167 +++++++++++++++++++++++---------------- 1 file changed, 100 insertions(+), 67 deletions(-) diff --git a/src/s6-rc/s6-rc-update.c b/src/s6-rc/s6-rc-update.c index 617cdfb..c1074de 100644 --- a/src/s6-rc/s6-rc-update.c +++ b/src/s6-rc/s6-rc-update.c @@ -48,26 +48,24 @@ static unsigned int verbosity = 1 ; /* Conversions and transitions */ - - /* - oldstate flags: - 1 -> is up - 2 -> wanted down - 4 -> restart - 8 -> converts to atomic or singleton - 16 -> has a new name - 32 -> wanted down after closure - 64 -> appears in convfile - - newstate flags: - 1 -> is up (converted from old up) - 2 -> wanted up - 4 -> is a bijective conversion target - 8 -> is a conversion target - 16 -> changed names - 32 -> is up after closure (i.e. includes new deps) - 128 -> depends on a new service, has to be restarted - */ +#define OLDSTATE_WAS_UP 1 +#define OLDSTATE_INCLUDE_IN_DOWN_TRANSITION 2 +#define OLDSTATE_RESTART 4 +#define OLDSTATE_CONVERTS_TO_ATOMIC_OR_SINGLETON_BUNDLE 8 +#define OLDSTATE_HAS_NEW_NAME 16 +#define OLDSTATE_WANT_DOWN_OR_RESTART 32 +#define OLDSTATE_WANT_DOWN_OR_RESTART_BITNO 5 +#define OLDSTATE_APPEARS_IN_CONVFILE 64 + +#define NEWSTATE_WAS_UP 1 +#define NEWSTATE_INCLUDE_IN_UP_TRANSITION 2 +#define NEWSTATE_IS_BIJECTIVE_CONVERSION_TARGET 4 +#define NEWSTATE_IS_CONVERSION_TARGET 8 +#define NEWSTATE_CHANGED_NAMES 16 +#define NEWSTATE_WANT_UP_AFTER_CLOSURE_BITNO 5 +#define NEWSTATE_WANT_UP_AFTER_CLOSURE 32 +#define NEWSTATE_DEPENDS_ON_A_NEW_SERVICE_BITNO 7 +#define NEWSTATE_DEPENDS_ON_A_NEW_SERVICE 128U static inline void parse_line (stralloc *sa, char const *s, size_t slen, unsigned int *newnames, unsigned char *oldstate, cdb const *oldc, s6rc_db_t const *olddb) { @@ -96,22 +94,22 @@ static inline void parse_line (stralloc *sa, char const *s, size_t slen, unsigne if (data.len != 4) strerr_dief5x(5, "identifier ", sa->s + base + slen, " does not represent an atomic service in ", live, "/compiled") ; uint32_unpack_big(data.s, &x) ; if (x >= oldn) strerr_dief3x(4, "invalid database in ", live, "/compiled") ; - if (oldstate[x] & 64) + if (oldstate[x] & OLDSTATE_APPEARS_IN_CONVFILE) strerr_dief3x(6, "service ", olddb->string + olddb->services[x].name, " appears more than once in conversion file") ; - oldstate[x] |= 64 ; + oldstate[x] |= OLDSTATE_APPEARS_IN_CONVFILE ; cur = base + slen + strlen(sa->s + base + slen) + 1 ; if (n >= 2 && !strcmp(sa->s + cur, "->")) { size_t newnamelen = strlen(sa->s + cur + 3) ; memcpy(sa->s + sa->len, sa->s + cur + 3, newnamelen + 1) ; - newnames[x] = sa->len ; oldstate[x] |= 16 ; + newnames[x] = sa->len ; oldstate[x] |= OLDSTATE_HAS_NEW_NAME ; sa->len += newnamelen + 1 ; cur += newnamelen + 4 ; n -= 2 ; } while (n--) { - if (!strcmp(sa->s + cur, "restart")) oldstate[x] |= 4 ; + if (!strcmp(sa->s + cur, "restart")) oldstate[x] |= OLDSTATE_RESTART ; else strerr_dief2x(100, "unknown keyword in conversion file: ", sa->s + cur) ; cur += strlen(sa->s + cur) + 1 ; @@ -161,37 +159,39 @@ static inline void fill_convtable_and_flags (unsigned char *conversion_table, un while (i--) { - char const *newname = oldstate[i] & 16 ? namedata + oldindex[i] : olddb->string + olddb->services[i].name ; + char const *newname = oldstate[i] & OLDSTATE_HAS_NEW_NAME ? namedata + oldindex[i] : olddb->string + olddb->services[i].name ; cdb_data data ; int r = cdb_find(newc, &data, newname, strlen(newname)) ; if (r < 0) strerr_dief3x(111, "invalid cdb in ", newfn, "/resolve.cdb") ; if (!r) { - if (oldstate[i] & 16) + if (oldstate[i] & OLDSTATE_HAS_NEW_NAME) strerr_dief4x(4, "bad conversion file: new service ", newname, " is undefined in database ", newfn) ; - oldstate[i] |= 34 ; /* disappeared */ + oldstate[i] |= OLDSTATE_WANT_DOWN_OR_RESTART | OLDSTATE_INCLUDE_IN_DOWN_TRANSITION ; /* disappeared */ continue ; } if (data.len & 3) strerr_dief3x(4, "invalid resolve database in ", newfn, "/resolve.cdb") ; if (data.len >> 2 > newn) strerr_dief3x(4, "invalid resolve database in ", newfn, "/resolve.cdb") ; - if (data.len == 4) oldstate[i] |= 8 ; + if (data.len == 4) oldstate[i] |= OLDSTATE_CONVERTS_TO_ATOMIC_OR_SINGLETON_BUNDLE ; while (data.len) { uint32_t x ; uint32_unpack_big(data.s, &x) ; data.s += 4 ; data.len -= 4 ; if (x >= newn) strerr_dief3x(4, "invalid resolve database in ", newfn, "/resolve.cdb") ; - if (newstate[x] & 8) + 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) ; - newstate[x] |= 8 ; + + newstate[x] |= NEWSTATE_IS_CONVERSION_TARGET ; invimage[x] = i ; - if (oldstate[i] & 16) newstate[x] |= 16 ; + if (oldstate[i] & OLDSTATE_HAS_NEW_NAME) newstate[x] |= NEWSTATE_CHANGED_NAMES ; bitarray_set(conversion_table + i * bitarray_div8(newn), x) ; - if (oldstate[i] & 8) + if (oldstate[i] & OLDSTATE_CONVERTS_TO_ATOMIC_OR_SINGLETON_BUNDLE) { - newstate[x] |= 4 ; - if ((i < olddb->nlong) != (x < newdb->nlong)) oldstate[i] |= 4 ; + newstate[x] |= NEWSTATE_IS_BIJECTIVE_CONVERSION_TARGET ; + + if ((i < olddb->nlong) != (x < newdb->nlong)) oldstate[i] |= OLDSTATE_RESTART ; } } } @@ -222,7 +222,11 @@ static void compute_transitions (char const *convfile, unsigned char *oldstate, { int done = 1 ; unsigned int i = newn ; - while (i--) newstate[i] &= 28 ; + while (i--) + newstate[i] &= + ( NEWSTATE_IS_BIJECTIVE_CONVERSION_TARGET | + NEWSTATE_IS_CONVERSION_TARGET | + NEWSTATE_CHANGED_NAMES); /* If an old service needs to restart, mark it wanted down, as well @@ -232,11 +236,22 @@ static void compute_transitions (char const *convfile, unsigned char *oldstate, i = oldn ; while (i--) { - if (oldstate[i] & 1 && (oldstate[i] & 4 || !(oldstate[i] & 8))) - oldstate[i] |= 34 ; - else oldstate[i] &= 221 ; + if (oldstate[i] & OLDSTATE_WAS_UP && + (oldstate[i] & OLDSTATE_RESTART || + !(oldstate[i] & OLDSTATE_CONVERTS_TO_ATOMIC_OR_SINGLETON_BUNDLE) + )) + oldstate[i] |= + OLDSTATE_WANT_DOWN_OR_RESTART | + OLDSTATE_INCLUDE_IN_DOWN_TRANSITION; + else + oldstate[i] &= + ~(OLDSTATE_WANT_DOWN_OR_RESTART | + OLDSTATE_INCLUDE_IN_DOWN_TRANSITION); } - s6rc_graph_closure(olddb, oldstate, 5, 0) ; + s6rc_graph_closure(olddb, + oldstate, + OLDSTATE_WANT_DOWN_OR_RESTART_BITNO, + 0) ; /* @@ -247,12 +262,16 @@ static void compute_transitions (char const *convfile, unsigned char *oldstate, */ i = oldn ; - while (i--) if (oldstate[i] & 1) - { - unsigned int j = newn ; - while (j--) if (bitarray_peek(conversion_table + i * newm, j)) - newstate[j] |= (oldstate[i] & 32) ? 2 : 33 ; - } + while (i--) + if (oldstate[i] & OLDSTATE_WAS_UP) { + unsigned int j = newn ; + while (j--) + if (bitarray_peek(conversion_table + i * newm, j)) + newstate[j] |= + (oldstate[i] & OLDSTATE_WANT_DOWN_OR_RESTART) + ? NEWSTATE_INCLUDE_IN_UP_TRANSITION + : (NEWSTATE_WAS_UP | NEWSTATE_WANT_UP_AFTER_CLOSURE) ; + } /* @@ -261,17 +280,23 @@ static void compute_transitions (char const *convfile, unsigned char *oldstate, restart and loop until there are no new dependencies. */ - s6rc_graph_closure(newdb, newstate, 5, 1) ; + s6rc_graph_closure(newdb, newstate, NEWSTATE_WANT_UP_AFTER_CLOSURE_BITNO, 1) ; i = newn ; - while (i--) if ((newstate[i] & 33) == 32) - { - done = 0 ; - newstate[i] |= 128U ; - } + while (i--) + if ( (newstate[i] & NEWSTATE_WANT_UP_AFTER_CLOSURE) && + !(newstate[i] & NEWSTATE_WAS_UP)) { + done = 0 ; + newstate[i] |= NEWSTATE_DEPENDS_ON_A_NEW_SERVICE ; + } if (done) break ; - s6rc_graph_closure(newdb, newstate, 7, 0) ; + + s6rc_graph_closure(newdb, newstate, NEWSTATE_DEPENDS_ON_A_NEW_SERVICE_BITNO, 0) ; + i = newn ; - while (i--) if ((newstate[i] & 129U) == 129U) oldstate[invimage[i]] |= 4 ; + while (i--) + if ((newstate[i] & NEWSTATE_WAS_UP) && + (newstate[i] & NEWSTATE_DEPENDS_ON_A_NEW_SERVICE)) + oldstate[invimage[i]] |= OLDSTATE_RESTART ; } } @@ -291,9 +316,9 @@ static inline void rollback_servicedirs (char const *newlive, unsigned char cons memcpy(newfn, newlive, newllen) ; memcpy(newfn + newllen, "/servicedirs/", 13) ; memcpy(newfn + newllen + 13, newdb->string + newdb->services[i].name, newnamelen + 1) ; - if (newstate[i] & 1) + if (newstate[i] & NEWSTATE_WAS_UP) { - char const *oldname = newstate[i] & 8 ? olddb->string + olddb->services[invimage[i]].name : newdb->string + newdb->services[i].name ; + char const *oldname = newstate[i] & NEWSTATE_IS_CONVERSION_TARGET ? olddb->string + olddb->services[invimage[i]].name : newdb->string + newdb->services[i].name ; size_t oldnamelen = strlen(oldname) ; char oldfn[livelen + 23 + oldnamelen] ; memcpy(oldfn, live, livelen) ; @@ -327,7 +352,7 @@ static inline void make_new_livedir (unsigned char const *oldstate, s6rc_db_t co if (r < 0) strerr_diefu2sys(111, "readlink ", sdlink) ; if (r >= SKALIBS_PATH_MAX - 1) strerr_dief3x(100, "target for ", sdlink, " is too long") ; sdtarget[r] = 0 ; - while (i--) tmpstate[i] = newstate[i] & 1 ; + while (i--) tmpstate[i] = newstate[i] & NEWSTATE_WAS_UP ; if (!s6rc_livedir_create(sa, live, PROG, sdtarget, prefix, newcompiled, tmpstate, newdb->nlong + newdb->nshort, &dirlen)) strerr_diefu1sys(111, "create new livedir") ; } @@ -345,9 +370,9 @@ static inline void make_new_livedir (unsigned char const *oldstate, s6rc_db_t co memcpy(newfn + newclen + 13, newdb->string + newdb->services[i].name, newnamelen + 1) ; sa->len = sdlen ; if (!stralloc_cats(sa, newdb->string + newdb->services[i].name) || !stralloc_0(sa)) goto rollback ; - if (newstate[i] & 1) + if (newstate[i] & NEWSTATE_WAS_UP) { - char const *oldname = newstate[i] & 8 ? olddb->string + olddb->services[invimage[i]].name : newdb->string + newdb->services[i].name ; + char const *oldname = newstate[i] & NEWSTATE_IS_CONVERSION_TARGET ? olddb->string + olddb->services[invimage[i]].name : newdb->string + newdb->services[i].name ; size_t oldnamelen = strlen(oldname) ; char oldfn[livelen + 14 + oldnamelen] ; memcpy(oldfn, live, livelen) ; @@ -381,7 +406,11 @@ static inline void make_new_livedir (unsigned char const *oldstate, s6rc_db_t co /* scandir cleanup, then old livedir cleanup */ i = olddb->nlong ; while (i--) - s6rc_servicedir_unsupervise(sa->s, prefix, olddb->string + olddb->services[i].name, (oldstate[i] & 33) == 1) ; + s6rc_servicedir_unsupervise(sa->s, + prefix, + olddb->string + olddb->services[i].name, + (oldstate[i] & OLDSTATE_WAS_UP) && + !(oldstate[i] & OLDSTATE_WANT_DOWN_OR_RESTART)) ; rm_rf_in_tmp(sa, 0) ; sa->len = 0 ; return ; @@ -405,7 +434,7 @@ static inline int delete_unused_pipes (s6_fdholder_t *a, s6rc_db_t const *olddb, { unsigned int i = olddb->nlong ; while (i--) - if (!(oldstate[i] & 8) && olddb->services[i].x.longrun.nproducers) + if (!(oldstate[i] & OLDSTATE_CONVERTS_TO_ATOMIC_OR_SINGLETON_BUNDLE) && olddb->services[i].x.longrun.nproducers) { size_t len = strlen(olddb->string + olddb->services[i].name) ; char pipename[len + 13] ; @@ -426,7 +455,9 @@ static inline int rename_pipes (s6_fdholder_t *a, s6rc_db_t const *olddb, s6rc_d unsigned int i = newdb->nlong ; while (i--) { - if ((newstate[i] & 20) == 20 && newdb->services[i].x.longrun.nproducers) + if ((newstate[i] & NEWSTATE_CHANGED_NAMES) && + (newstate[i] & NEWSTATE_IS_BIJECTIVE_CONVERSION_TARGET) && + newdb->services[i].x.longrun.nproducers) { int fd ; size_t oldlen = strlen(olddb->string + olddb->services[invimage[i]].name) ; @@ -472,7 +503,8 @@ static inline int create_new_pipes (s6_fdholder_t *a, s6rc_db_t const *newdb, un nano1.nano = 1 ; while (i--) { - if (!(newstate[i] & 4) && newdb->services[i].x.longrun.nproducers) + if (!(newstate[i] & NEWSTATE_IS_BIJECTIVE_CONVERSION_TARGET) && + newdb->services[i].x.longrun.nproducers) { int p[2] ; size_t len = strlen(newdb->string + newdb->services[i].name) ; @@ -516,7 +548,7 @@ static inline void update_fdholder (s6rc_db_t const *olddb, unsigned char const { s6_fdholder_t a = S6_FDHOLDER_ZERO ; char fnsocket[livelen + sizeof("/servicedirs/" S6RC_FDHOLDER "/s")] ; - if (!(newstate[1] & 1)) return ; + if (!(newstate[1] & NEWSTATE_WAS_UP)) return ; memcpy(fnsocket, live, livelen) ; memcpy(fnsocket + livelen, "/servicedirs/" S6RC_FDHOLDER "/s", sizeof("/servicedirs/" S6RC_FDHOLDER "/s")) ; if (!s6_fdholder_start_g(&a, fnsocket, deadline)) goto hammer ; @@ -692,7 +724,7 @@ int main (int argc, char const *const *argv, char const *const *envp) if (rr < oldn) strerr_diefu2x(4, "read valid db in ", dbfn) ; } r = oldn ; - while (r--) oldstate[r] &= 1 ; + while (r--) oldstate[r] &= OLDSTATE_WAS_UP ; memset(newstate, 0, newn) ; r = newn ; while (r--) invimage[r] = olddb.nlong + olddb.nshort ; @@ -735,8 +767,9 @@ int main (int argc, char const *const *argv, char const *const *envp) newargv[m++] = "-d" ; newargv[m++] = "--" ; newargv[m++] = "change" ; - while (i--) if (oldstate[i] & 2) - newargv[m++] = olddb.string + olddb.services[i].name ; + while (i--) + if (oldstate[i] & OLDSTATE_INCLUDE_IN_DOWN_TRANSITION) + newargv[m++] = olddb.string + olddb.services[i].name ; newargv[m++] = 0 ; if (verbosity >= 2) strerr_warni1x("stopping services in the old database") ; @@ -819,7 +852,7 @@ int main (int argc, char const *const *argv, char const *const *envp) newargv[m++] = "-u" ; newargv[m++] = "--" ; newargv[m++] = "change" ; - while (i--) if (newstate[i] & 2) + while (i--) if (newstate[i] & NEWSTATE_INCLUDE_IN_UP_TRANSITION) newargv[m++] = newdb.string + newdb.services[i].name ; newargv[m++] = 0 ; if (verbosity >= 2) -- 2.41.0