Re: PATCH: a bit of introspection in make
On Mon, Aug 07, 2023 at 10:05:37PM -0400, Thomas Frohwein wrote:
> I looked through the file and it seems that varname_list_changed is
> never set to anything but true. Is there a bit missing, like down lower
> in varname_list_retrieve()?
Yep, I removed it at some point because it looked like some extra
unneeded work, and then I did change my mind because actual patterns
require it several times. So, yeah, it should be set to false there.
As for patterns, well, it will allow to write loops like
.for v in ${.VARIABLES:MMASTER_SITES*}
instead of having to look at MASTER_SITES0...9 manually, just as a for
instance.
agtimer(4/arm64): simplify agtimer_delay()
The agtimer(4/arm64) delay(9) implementation is quite complicated.
This patch simplifies it.
Am I missing something here? There's no reason to implement the
busy-loop like this, right?
ok?
Index: agtimer.c
===
RCS file: /cvs/src/sys/arch/arm64/dev/agtimer.c,v
retrieving revision 1.23
diff -u -p -r1.23 agtimer.c
--- agtimer.c 25 Jul 2023 18:16:19 - 1.23
+++ agtimer.c 8 Aug 2023 02:24:57 -
@@ -323,32 +323,12 @@ agtimer_cpu_initclocks(void)
void
agtimer_delay(u_int usecs)
{
- uint64_tclock, oclock, delta, delaycnt;
- uint64_tcsec, usec;
- volatile intj;
+ uint64_t cycles, start;
- if (usecs > (0x8000 / agtimer_frequency)) {
- csec = usecs / 1;
- usec = usecs % 1;
-
- delaycnt = (agtimer_frequency / 100) * csec +
- (agtimer_frequency / 100) * usec / 1;
- } else {
- delaycnt = agtimer_frequency * usecs / 100;
- }
- if (delaycnt <= 1)
- for (j = 100; j > 0; j--)
- ;
-
- oclock = agtimer_readcnt64();
- while (1) {
- for (j = 100; j > 0; j--)
- ;
- clock = agtimer_readcnt64();
- delta = clock - oclock;
- if (delta > delaycnt)
- break;
- }
+ start = agtimer_readcnt64();
+ cycles = (uint64_t)usecs * agtimer_frequency / 100;
+ while (agtimer_readcnt64() - start < cycles)
+ CPU_BUSY_CYCLE();
}
void
Re: PATCH: a bit of introspection in make
On Mon, Aug 07, 2023 at 04:42:54PM +0200, Marc Espie wrote:
> I think it could be occasionally useful to know which variables have
> been defined in make.
>
> Incidentally, this DOES exist in GNU make, so I've reused the same name
> for basically the same functionality.
>
> I haven't checked whether NetBSD/FreeBSD introduced something similar.
>
> This is a fairly straightforward patch, introduces .VARIABLES corresponding
> to the full list of global variables (from the command line and the Makefile)
> that have been defined.
>
> (^ says the guy who had to remember a few details from his own(!) var.c
> implementation from a few years ago)
>
> I just took var_get_value offline from the old macro, for readability,
> even though it's likely the compiler may still decide to inline it.
>
> For efficiency, that list is only computed as needed, since it is
> somewhat long.
>
> For debugging purposes, this can come in fairly handy, and I see at
> least another application in ports.
>
> Comments and nits welcome.
>
> Note that the list is completely unsorted. This could be sorted through
> since we already have the code for dumping purposes, but it would be even
> more expensive (the order will be "random", as per the hash)
I can't judge all the implications of this, but can speak to that I
compiled it and that I get a very long list of variables with:
$ make show=.VARIABLES
> Index: var.c
> ===
> RCS file: /cvs/src/usr.bin/make/var.c,v
> retrieving revision 1.104
> diff -u -p -r1.104 var.c
> --- var.c 9 Jun 2022 13:13:14 - 1.104
> +++ var.c 7 Aug 2023 14:33:42 -
> @@ -104,6 +104,8 @@ static char varNoError[] = "";
> bool errorIsOkay;
> static bool checkEnvFirst; /* true if environment should be searched for
>* variables before the global context */
> + /* do we need to recompute varname_list */
> +static bool varname_list_changed = true;
I assume the comment /* do we need to recompute varname_list */ is not
meant to be contiguous with the 2 comment lines above (/* true if...
* variables before...). I suggest using whitespace (newline?) to make
that clearer.
I looked through the file and it seems that varname_list_changed is
never set to anything but true. Is there a bit missing, like down lower
in varname_list_retrieve()?
>
> void
> Var_setCheckEnvFirst(bool yes)
> @@ -228,9 +230,12 @@ typedef struct Var_ {
> */
> #define POISONS (POISON_NORMAL | POISON_EMPTY | POISON_NOT_DEFINED)
> /* Defined in var.h */
> +#define VAR_IS_NAMES 1024/* Very expensive, only defined when needed */
> char name[1]; /* the variable's name */
> } Var;
>
> +/* for GNU make compatibility */
> +#define VARNAME_LIST ".VARIABLES"
>
> static struct ohash_info var_info = {
> offsetof(Var, name),
> @@ -245,10 +250,11 @@ static void fill_from_env(Var *);
> static Var *create_var(const char *, const char *);
> static void var_set_initial_value(Var *, const char *);
> static void var_set_value(Var *, const char *);
> -#define var_get_value(v) ((v)->flags & VAR_EXEC_LATER ? \
> - var_exec_cmd(v) : \
> - Buf_Retrieve(&((v)->val)))
> -static char *var_exec_cmd(Var *);
> +static char *var_get_value(Var *);
> +static void var_exec_cmd(Var *);
> +static void varname_list_retrieve(Var *);
> +
> +
> static void var_append_value(Var *, const char *);
> static void poison_check(Var *);
> static void var_set_append(const char *, const char *, const char *, int,
> bool);
> @@ -423,6 +429,7 @@ var_set_initial_value(Var *v, const char
> len = strlen(val);
> Buf_Init(&(v->val), len+1);
> Buf_AddChars(&(v->val), len, val);
> + varname_list_changed = true;
> }
>
> /* Normal version of var_set_value(), to be called after variable is fully
> @@ -440,6 +447,16 @@ var_set_value(Var *v, const char *val)
> }
> }
>
> +static char *
> +var_get_value(Var *v)
> +{
> + if (v->flags & VAR_IS_NAMES)
> + varname_list_retrieve(v);
> + else if (v->flags & VAR_EXEC_LATER)
> + var_exec_cmd(v);
> + return Buf_Retrieve(&(v->val));
> +}
> +
> /* Add to a variable, insert a separating space if the variable was already
> * defined.
> */
> @@ -628,6 +645,7 @@ Var_Deletei(const char *name, const char
>
> ohash_remove(&global_variables, slot);
> delete_var(v);
> + varname_list_changed = true;
> }
>
> /* Set or add a global variable, either to VAR_CMD or VAR_GLOBAL.
> @@ -687,7 +705,7 @@ Var_Appendi_with_ctxt(const char *name,
> var_set_append(name, ename, val, ctxt, true);
> }
>
> -static char *
> +static void
> var_exec_cmd(Var *v)
> {
> char *arg = Buf_Retrieve(&(v->val));
> @@ -699,7 +717,27 @@ var_exec_cmd(Var *v)
> var_set_value(v, res1);
> free(res1);
> v->flags &= ~VAR_EXEC_LATER;
> -
Re: sec(4): route based ipsec vpns
On Mon, Aug 07, 2023 at 05:36:27PM +0200, Tobias Heider wrote:
> On Mon, Aug 07, 2023 at 02:22:23PM +1000, David Gwynne wrote:
> > tobhe@ wrote the iked bits, so he'll commit them when he's ready.
> >
> > your config looks pretty much the same as mine except you specify a lot
> > more stuff around lifetimes and crypto than i do. maybe try without "tunnel
> > esp"?
> >
> > dlg
>
> The config in the previous mail looks ok. You will need the "from any to any"
> for now but I'm planning to adjust the default.
>
> Below is the latest state of my iked diff. The previous version had a small
> bug in pfkey preventing SAs from getting deleted properly.
this is working well in my testing. ok by me.
>
> diff cfdc039572fed1d8c9081b6d9557ce9b85e89697
> a2b839af1fa9d47730731e392e3c8dd0e9f7dd1e
> commit - cfdc039572fed1d8c9081b6d9557ce9b85e89697
> commit + a2b839af1fa9d47730731e392e3c8dd0e9f7dd1e
> blob - 2c7fbe14af3dad6fe38af607e9f870324e2be55c
> blob + 6449f3cc2d423daee6294b71ca098247a296886a
> --- sbin/iked/iked.h
> +++ sbin/iked/iked.h
> @@ -260,6 +260,7 @@ struct iked_policy {
> #define IKED_POLICY_SKIP 0x10
> #define IKED_POLICY_IPCOMP0x20
> #define IKED_POLICY_TRANSPORT 0x40
> +#define IKED_POLICY_ROUTING 0x80
>
> int pol_refcnt;
>
> blob - bf6bf0fb0d43ef17ebe71368dfbc4bf28628a1f8
> blob + 25e5098dd29b03426aa2d9a03c324f13dc8cdecf
> --- sbin/iked/ikev2.c
> +++ sbin/iked/ikev2.c
> @@ -6532,63 +6532,65 @@ ikev2_childsa_enable(struct iked *env, struct iked_sa
> peer_changed = (memcmp(&sa->sa_peer_loaded, &sa->sa_peer,
> sizeof(sa->sa_peer_loaded)) != 0);
>
> - TAILQ_FOREACH(flow, &sa->sa_flows, flow_entry) {
> - /* re-load the flow if the peer for the flow has changed */
> - reload = 0;
> - if (flow->flow_loaded) {
> - if (!peer_changed) {
> - log_debug("%s: flow already loaded %p",
> - __func__, flow);
> - continue;
> + if (!(sa->sa_policy->pol_flags & IKED_POLICY_ROUTING)) {
> + TAILQ_FOREACH(flow, &sa->sa_flows, flow_entry) {
> + /* re-load the flow if the peer for the flow has
> changed */
> + reload = 0;
> + if (flow->flow_loaded) {
> + if (!peer_changed) {
> + log_debug("%s: flow already loaded %p",
> + __func__, flow);
> + continue;
> + }
> + RB_REMOVE(iked_flows, &env->sc_activeflows,
> flow);
> + (void)pfkey_flow_delete(env, flow);
> + flow->flow_loaded = 0; /* we did RB_REMOVE */
> + reload = 1;
> }
> - RB_REMOVE(iked_flows, &env->sc_activeflows, flow);
> - (void)pfkey_flow_delete(env, flow);
> - flow->flow_loaded = 0; /* we did RB_REMOVE */
> - reload = 1;
> - }
>
> - if (pfkey_flow_add(env, flow) != 0) {
> - log_debug("%s: failed to load flow", __func__);
> - goto done;
> - }
> + if (pfkey_flow_add(env, flow) != 0) {
> + log_debug("%s: failed to load flow", __func__);
> + goto done;
> + }
>
> - if ((oflow = RB_FIND(iked_flows, &env->sc_activeflows, flow))
> - != NULL) {
> - log_debug("%s: replaced old flow %p with %p",
> - __func__, oflow, flow);
> - oflow->flow_loaded = 0;
> - RB_REMOVE(iked_flows, &env->sc_activeflows, oflow);
> - }
> + if ((oflow = RB_FIND(iked_flows, &env->sc_activeflows,
> flow))
> + != NULL) {
> + log_debug("%s: replaced old flow %p with %p",
> + __func__, oflow, flow);
> + oflow->flow_loaded = 0;
> + RB_REMOVE(iked_flows, &env->sc_activeflows,
> oflow);
> + }
>
> - RB_INSERT(iked_flows, &env->sc_activeflows, flow);
> + RB_INSERT(iked_flows, &env->sc_activeflows, flow);
>
> - log_debug("%s: %sloaded flow %p", __func__,
> - reload ? "re" : "", flow);
> + log_debug("%s: %sloaded flow %p", __func__,
> + reload ? "re" : "", flow);
>
> - /* append flow to log buffer */
> - if (flow->flow_dir == IPSP_DIRECTION_OUT &&
> - flow->flow_prenat.addr_af != 0)
>
Re: standardize and simplify GitHub submodule handling in ports?
On 2023/08/07 14:53, Thomas Frohwein wrote: > On Mon, Aug 07, 2023 at 06:59:15PM +0100, Stuart Henderson wrote: > [...] > > > I haven't looked at other ports, but asterisk, vim and vmm-firmware do > > not use git submodules. > > With vim, it's the way colorschemes are pulled in that *could* be > reworked using GH_SUBMODULES syntax. The old way continues to work, so > for any of the ports listed, there is no need to change anything. It does feel like a bit of a hack to use something named after submodules for something else. (And submodules thenselves are a total hack which I don't think shoukd be used at all, especially when projects expect to build from a checkkut rather than a proper distfile, but I digress ;) I think maybe I'd prefer to have some variable that could be used *instead* of the existing GH_* variables rather than in conjunction with them (so they can be used for all GH archive ports, rather than have them a special case for multi-distfile ports). If that's the standard way to do things we can have a sweep of the tree converting other ports (or at least the ones that don't use go.port.mk ;) It would be kind-of helpful if it could support more than just github too (gitlab.com, sr.ht, ..). While that could be done with different variables (GH_xx, GL_xx, SRHT_xx etc) they're all a similar enough layout to each other that making the site part of the variable itself rather than the name would be simpler and easier to add more sites (plus it covers the case where you have some port using one file from github and one from gitlab, etc). Playing with syntax ideas, maybe something like this would be easy to use for pprts not needing a rename - SOMEVAR+= github vim vim refs/tags/v9.0.1677 SOMEVAR+= github vim colorschemes 22986fa2a3d2f7229efd4019fcbca411caa6afbb or with some auto-renaming (and specifying more of the path to avoid the extra GH_WRKSRC which I think might not be enough in some cases anyway - a port may have several distfiles that need to go into different base dirs) - SOMEVAR+= github fortran-lang fpm refs/tags/v0.7.0 OTHERVAR+= github toml-f toml-f e49f5523e4ee67db6628618864504448fb8c8939 vendor/toml-f OTHERVAR+= github urbanjost M_CLI2 90a1a146e19c8ad37b0469b8cbd04bc28eb67a50 vendor/M_CLI2 (no idea what to use as real names instead of SOMEVAR/OTHERVAR though!) How does that sort of thing seem to you? (i.e. using the same basic idea as you have for submodules, but making it the standard for all gh distfiles)?
Re: Bringing in OpenBSD
On Mon, Aug 7, 2023 at 9:48 AM Theo de Raadt wrote: > > enh wrote: > > > (fwiw, Android has someone working on adding this header to upstream > > LLVM right now, so hopefully it should come "for free" in llvm 18. > > what, if anything, to _do_ with it is another question though :-) ) > > Will stdckdint.h come into common usage before or after 32-bit time_t > wraps? yeah, i'm not expecting to see much use any time soon. it feels like it would have been a good idea a few decades ago, but (especially with the awful names --- who missed out the 'h' that everyone types when they try to spell this?!) there's little to recommend this over just using the builtins directly. "the destination comes first" seems to be about it? i can't say i'm a huge fan of either, and given that it'll need to be done per-libc rather than just per-compiler i do wonder whether it will be another "annex k" [https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1967.htm]. oh well, at least we finally got 0b literals and %b formatting, a mere 40 years late... (i'll grant them their first decade of "octal should be good enough for every PDP-11 programmer!" :-) ) > That's what I want to know. >
Re: standardize and simplify GitHub submodule handling in ports?
On Mon, Aug 07, 2023 at 06:59:15PM +0100, Stuart Henderson wrote: [...] > I haven't looked at other ports, but asterisk, vim and vmm-firmware do > not use git submodules. With vim, it's the way colorschemes are pulled in that *could* be reworked using GH_SUBMODULES syntax. The old way continues to work, so for any of the ports listed, there is no need to change anything.
Re: standardize and simplify GitHub submodule handling in ports?
On 8/7/2023 1:59 PM, Stuart Henderson wrote: > On 2023/08/07 12:44, Thomas Frohwein wrote: >> I tested this with the about 30 ports I could identify that use GitHub >> submodules, by adjusting the Makefile to use GH_SUBMODULES. Here a few >> points from what I've observed: > .. >> >> The full table of what I tested and the result up to if the port still >> packages is here: https://thfr.info/tmp/github-submodule-ports.txt > > I haven't looked at other ports, but asterisk, vim and vmm-firmware do > not use git submodules. > I don't want to change the DMD build process. It matches what upstream does, and has been helpful for debugging the process with them in the past. ~Brian
Re: standardize and simplify GitHub submodule handling in ports?
On 2023/08/07 12:44, Thomas Frohwein wrote: > I tested this with the about 30 ports I could identify that use GitHub > submodules, by adjusting the Makefile to use GH_SUBMODULES. Here a few > points from what I've observed: .. > > The full table of what I tested and the result up to if the port still > packages is here: https://thfr.info/tmp/github-submodule-ports.txt I haven't looked at other ports, but asterisk, vim and vmm-firmware do not use git submodules.
Re: Bringing in OpenBSD
enh wrote: > (fwiw, Android has someone working on adding this header to upstream > LLVM right now, so hopefully it should come "for free" in llvm 18. > what, if anything, to _do_ with it is another question though :-) ) Will stdckdint.h come into common usage before or after 32-bit time_t wraps? That's what I want to know.
Re: Bringing in OpenBSD
Lucian Popescu wrote: > I noticed that some parts of OpenBSD use awkward techniques to detect > undefined behavior in arithmetic operations, for example: ... > The snippet was taken from lib/libexpat/lib/xmlparse.c libexpat is outside source, which we incorporate We have no influence over what the upstream libexpat does, but I can *ASSURE YOU* there is zero change they will be switching to C23-only support tomorrow, and breaking compatibility with the entire universe. So that is such a terrible example. You couldn't find a better example?
Re: standardize and simplify GitHub submodule handling in ports?
On Sun, Aug 06, 2023 at 07:00:49PM +0200, Marc Espie wrote:
[...]
> > > I'm also wondering if keeping the main GH_* stuff in bsd.port.mk makes a
> > > lot
> > > of sense, instead of grouping everything in github.port.mk
> >
> > I'm for it, maybe as a second step after the module for just the
> > submodule handling is done because there would be a lot of ports churn
> > with moving the main GH_* stuff out of bsd.port.mk.
>
> Probably not. We have a few "big" modules that don't depend on explicitly
> adding to modules, but on some variable triggering it (historic imake stuff
> or configure), having github.port.mk brought in from one of the GH_* variables
> (probably don't need to test them all) would be acceptable.
I'm sharing a new version after some testing, this time as a diff
because it turns out the handling of GH_*-related DISTNAME juggling
needs to happen before setting up the DISTFILES in the module. So this
diff now effectively moves all the GH_* bits from bsd.port.mk into
github.port.mk. The module is hooked up by defining GH_ACCOUNT or
GH_SUBMODULES.
Some rework was needed while testing to handle the different situations
for the target directory of the submodule. This ended up with a test if
the directory exists, and depending on the result either rmdir'ing the
(presumed empty) directory, or mkdir'ing the parent directory.
I tested this with the about 30 ports I could identify that use GitHub
submodules, by adjusting the Makefile to use GH_SUBMODULES. Here a few
points from what I've observed:
1. It's well possible to mix and match with the old way of defining
MASTER_SITES0 etc and DISTFILES=filename.ext:0. An example is
devel/fpm, where I kept this line:
MASTER_SITES0 =
https://github.com/fortran-lang/fpm/releases/download/v${V}/
while replacing MASTER_SITES{1,2} with the new GH_SUBMODULES way.
2. Setting GH_WRKSRC allows keeping the diff from the current Makefile
small, for example in devel/fpm:
+GH_WRKSRC =${WRKSRC}/vendor
and in editors/neovim:
+GH_WRKSRC =${STATIC_DEPS_WRKSRC}
3. It's possible to either use the commit hash or a tagname, like here
for lang/jruby:
+GH_SUBMODULES+=jnr jffi refs/tags/jffi-1.3.10 jffi
4. When using EXTRACT_ONLY, the name to use is more complicated now,
but can be identified with `$ make show=CHECKSUMFILES`. Here is what I
had to change it to with print/lilypond:
-EXTRACT_ONLY= ${DISTNAME}.tar.gz urw-base35-fonts-${URW_V}.tar.gz
+EXTRACT_ONLY= ${DISTNAME}.tar.gz \
+
gh-submodules/ArtifexSoftware-urw-base35-fonts-${URW_V}.tar.gz
The full table of what I tested and the result up to if the port still
packages is here: https://thfr.info/tmp/github-submodule-ports.txt
I also tested megaglest as a port that uses GH_ACCOUNT etc without using any
submodules. Tested also with dxx-rebirth as an example of a port not
using any GH_*.
This draft hijacks MASTER_SITES8 for the modules purposes - not
MASTER_SITES9, as I noticed that is used by some modules like cargo and
go. I grep'd through the ports tree and couldn't identify any explicit
use of MASTER_SITES8.
I think this is ready for more general testing:
$ cd /usr/ports/infrastructure/mk; patch < /path/to/github-submodules.diff
Index: bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1595
diff -u -p -r1.1595 bsd.port.mk
--- bsd.port.mk 8 Jul 2023 10:20:16 - 1.1595
+++ bsd.port.mk 7 Aug 2023 16:42:28 -
@@ -136,8 +136,8 @@ _ALL_VARIABLES += BROKEN COMES_WITH \
CONFIGURE_STYLE USE_LIBTOOL SEPARATE_BUILD \
SHARED_LIBS TARGETS PSEUDO_FLAVOR \
AUTOCONF_VERSION AUTOMAKE_VERSION CONFIGURE_ARGS \
- GH_ACCOUNT GH_COMMIT GH_PROJECT GH_TAGNAME MAKEFILE_LIST \
- USE_LLD USE_NOEXECONLY USE_WXNEEDED USE_NOBTCFI \
+ GH_ACCOUNT GH_COMMIT GH_PROJECT GH_SUBMODULES GH_TAGNAME \
+ MAKEFILE_LIST USE_LLD USE_NOEXECONLY USE_WXNEEDED USE_NOBTCFI \
COMPILER COMPILER_LANGS COMPILER_LINKS \
SUBST_VARS UPDATE_PLIST_ARGS \
PKGPATHS DEBUG_PACKAGES DEBUG_CONFIGURE_ARGS \
@@ -313,6 +313,10 @@ MODULES += ${_i}
. endif
.endfor
+.if defined(GH_ACCOUNT) || defined(GH_SUBMODULES)
+MODULES += github
+.endif
+
MODULES ?=
.if !empty(MODULES) || !empty(COMPILER)
_MODULES_DONE =
@@ -611,18 +615,6 @@ BUILD_DEPENDS += textproc/groff>=1.21
_PKG_ARGS += -DUSE_GROFF=1
.endif
-# github related variables
-GH_TAGNAME ?=
-GH_COMMIT ?=
-GH_ACCOUNT ?=
-GH_PROJECT ?=
-
-.if !empty(GH_PROJECT) && !empty(GH_TAGNAME)
-_GH_TAG_DIST = ${GH_TAGNAME:C/^(v|V|ver|[Rr]el|[Rr]elease)[-._]?([0-9])/\2/}
-DISTNAME ?= ${GH_PROJECT}-${_GH_TAG_DIST}
-GH_DISTFILE = ${GH_PROJECT}-${_GH_TAG_DIST}${EXTRACT_SUFX}
-.endif
-
PKGNAME ?= ${DISTNAME}
FULLPKGNAME ?= ${PKGNAME}${FLAVOR_EXT}
_MASTER ?=
@@ -871,11 +863,7 @@ _WRKDIRS = ${WRKOBJDIR_${PKGPATH}}/${_WR
_WRKDIRS += ${WRKOBJDIR}/${_WRKDIR_STEM}
_WRKDIRS += ${WRKOBJ
Re: Bringing in OpenBSD
(fwiw, Android has someone working on adding this header to upstream
LLVM right now, so hopefully it should come "for free" in llvm 18.
what, if anything, to _do_ with it is another question though :-) )
On Mon, Aug 7, 2023 at 9:09 AM Lucian Popescu
wrote:
>
> Hi,
>
> I noticed that some parts of OpenBSD use awkward techniques to detect
> undefined behavior in arithmetic operations, for example:
>
> > static size_t
> > poolBytesToAllocateFor(int blockSize) {
> > /* Unprotected math would be:
> > ** return offsetof(BLOCK, s) + blockSize * sizeof(XML_Char);
> > **
> > ** Detect overflow, avoiding _signed_ overflow undefined behavior
> > ** For a + b * c we check b * c in isolation first, so that addition of
> a
> > ** on top has no chance of making us accept a small non-negative number
> > */
> > const size_t stretch = sizeof(XML_Char); /* can be 4 bytes */
> >
> > if (blockSize <= 0)
> > return 0;
> >
> > if (blockSize > (int)(INT_MAX / stretch))
> > return 0;
> >
> > {
> > const int stretchedBlockSize = blockSize * (int)stretch;
> > const int bytesToAllocate
> > = (int)(offsetof(BLOCK, s) + (unsigned)stretchedBlockSize);
> > if (bytesToAllocate < 0)
> > return 0;
> >
> > return (size_t)bytesToAllocate;
> > }
> > }
>
> The snippet was taken from lib/libexpat/lib/xmlparse.c
>
> This does not fully protect the code because bytesToAllocate might
> overflow in the unsigned addition resulting in bytesToAllocate > 0. Note
> that unsigned overflow is not undefined behavior but leads to unexpected
> results.
>
> C23 solves this problem using [1] which transforms the
> above function into:
>
> > static size_t
> > poolBytesToAllocateFor(int blockSize) {
> > size_t add, mul;
> > if (ckd_mul(&mul, blockSize, sizeof(XML_Char)))
> > return 0;
> > if (ckd_add(&add, mul, offsetof(BLOCK, s)))
> > return 0;
> > return add;
> > }
>
> Is it worth bringing this functionality in OpenBSD? As a starting point
> I was thinking that we can make use of __builtin_add_overflow et al.
> when compiling with Clang. GCC is trickier to solve as the builtins were
> implemented from version 5.
>
> Best Regards,
> Lucian Popescu
>
> [1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2681.pdf
Bringing in OpenBSD
Hi,
I noticed that some parts of OpenBSD use awkward techniques to detect
undefined behavior in arithmetic operations, for example:
> static size_t
> poolBytesToAllocateFor(int blockSize) {
> /* Unprotected math would be:
> ** return offsetof(BLOCK, s) + blockSize * sizeof(XML_Char);
> **
> ** Detect overflow, avoiding _signed_ overflow undefined behavior
> ** For a + b * c we check b * c in isolation first, so that addition of
a
> ** on top has no chance of making us accept a small non-negative number
> */
> const size_t stretch = sizeof(XML_Char); /* can be 4 bytes */
>
> if (blockSize <= 0)
> return 0;
>
> if (blockSize > (int)(INT_MAX / stretch))
> return 0;
>
> {
> const int stretchedBlockSize = blockSize * (int)stretch;
> const int bytesToAllocate
> = (int)(offsetof(BLOCK, s) + (unsigned)stretchedBlockSize);
> if (bytesToAllocate < 0)
> return 0;
>
> return (size_t)bytesToAllocate;
> }
> }
The snippet was taken from lib/libexpat/lib/xmlparse.c
This does not fully protect the code because bytesToAllocate might
overflow in the unsigned addition resulting in bytesToAllocate > 0. Note
that unsigned overflow is not undefined behavior but leads to unexpected
results.
C23 solves this problem using [1] which transforms the
above function into:
> static size_t
> poolBytesToAllocateFor(int blockSize) {
> size_t add, mul;
> if (ckd_mul(&mul, blockSize, sizeof(XML_Char)))
> return 0;
> if (ckd_add(&add, mul, offsetof(BLOCK, s)))
> return 0;
> return add;
> }
Is it worth bringing this functionality in OpenBSD? As a starting point
I was thinking that we can make use of __builtin_add_overflow et al.
when compiling with Clang. GCC is trickier to solve as the builtins were
implemented from version 5.
Best Regards,
Lucian Popescu
[1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2681.pdf
Re: sec(4): route based ipsec vpns
On Mon, Aug 07, 2023 at 02:22:23PM +1000, David Gwynne wrote:
> tobhe@ wrote the iked bits, so he'll commit them when he's ready.
>
> your config looks pretty much the same as mine except you specify a lot
> more stuff around lifetimes and crypto than i do. maybe try without "tunnel
> esp"?
>
> dlg
The config in the previous mail looks ok. You will need the "from any to any"
for now but I'm planning to adjust the default.
Below is the latest state of my iked diff. The previous version had a small
bug in pfkey preventing SAs from getting deleted properly.
diff cfdc039572fed1d8c9081b6d9557ce9b85e89697
a2b839af1fa9d47730731e392e3c8dd0e9f7dd1e
commit - cfdc039572fed1d8c9081b6d9557ce9b85e89697
commit + a2b839af1fa9d47730731e392e3c8dd0e9f7dd1e
blob - 2c7fbe14af3dad6fe38af607e9f870324e2be55c
blob + 6449f3cc2d423daee6294b71ca098247a296886a
--- sbin/iked/iked.h
+++ sbin/iked/iked.h
@@ -260,6 +260,7 @@ struct iked_policy {
#define IKED_POLICY_SKIP0x10
#define IKED_POLICY_IPCOMP 0x20
#define IKED_POLICY_TRANSPORT 0x40
+#define IKED_POLICY_ROUTING 0x80
int pol_refcnt;
blob - bf6bf0fb0d43ef17ebe71368dfbc4bf28628a1f8
blob + 25e5098dd29b03426aa2d9a03c324f13dc8cdecf
--- sbin/iked/ikev2.c
+++ sbin/iked/ikev2.c
@@ -6532,63 +6532,65 @@ ikev2_childsa_enable(struct iked *env, struct iked_sa
peer_changed = (memcmp(&sa->sa_peer_loaded, &sa->sa_peer,
sizeof(sa->sa_peer_loaded)) != 0);
- TAILQ_FOREACH(flow, &sa->sa_flows, flow_entry) {
- /* re-load the flow if the peer for the flow has changed */
- reload = 0;
- if (flow->flow_loaded) {
- if (!peer_changed) {
- log_debug("%s: flow already loaded %p",
- __func__, flow);
- continue;
+ if (!(sa->sa_policy->pol_flags & IKED_POLICY_ROUTING)) {
+ TAILQ_FOREACH(flow, &sa->sa_flows, flow_entry) {
+ /* re-load the flow if the peer for the flow has
changed */
+ reload = 0;
+ if (flow->flow_loaded) {
+ if (!peer_changed) {
+ log_debug("%s: flow already loaded %p",
+ __func__, flow);
+ continue;
+ }
+ RB_REMOVE(iked_flows, &env->sc_activeflows,
flow);
+ (void)pfkey_flow_delete(env, flow);
+ flow->flow_loaded = 0; /* we did RB_REMOVE */
+ reload = 1;
}
- RB_REMOVE(iked_flows, &env->sc_activeflows, flow);
- (void)pfkey_flow_delete(env, flow);
- flow->flow_loaded = 0; /* we did RB_REMOVE */
- reload = 1;
- }
- if (pfkey_flow_add(env, flow) != 0) {
- log_debug("%s: failed to load flow", __func__);
- goto done;
- }
+ if (pfkey_flow_add(env, flow) != 0) {
+ log_debug("%s: failed to load flow", __func__);
+ goto done;
+ }
- if ((oflow = RB_FIND(iked_flows, &env->sc_activeflows, flow))
- != NULL) {
- log_debug("%s: replaced old flow %p with %p",
- __func__, oflow, flow);
- oflow->flow_loaded = 0;
- RB_REMOVE(iked_flows, &env->sc_activeflows, oflow);
- }
+ if ((oflow = RB_FIND(iked_flows, &env->sc_activeflows,
flow))
+ != NULL) {
+ log_debug("%s: replaced old flow %p with %p",
+ __func__, oflow, flow);
+ oflow->flow_loaded = 0;
+ RB_REMOVE(iked_flows, &env->sc_activeflows,
oflow);
+ }
- RB_INSERT(iked_flows, &env->sc_activeflows, flow);
+ RB_INSERT(iked_flows, &env->sc_activeflows, flow);
- log_debug("%s: %sloaded flow %p", __func__,
- reload ? "re" : "", flow);
+ log_debug("%s: %sloaded flow %p", __func__,
+ reload ? "re" : "", flow);
- /* append flow to log buffer */
- if (flow->flow_dir == IPSP_DIRECTION_OUT &&
- flow->flow_prenat.addr_af != 0)
- snprintf(prenat_mask, sizeof(prenat_mask), "%d",
- flow->flow_prenat.addr_mask);
- else
- prenat_mask[0] = '\0';
-
PATCH: a bit of introspection in make
I think it could be occasionally useful to know which variables have
been defined in make.
Incidentally, this DOES exist in GNU make, so I've reused the same name
for basically the same functionality.
I haven't checked whether NetBSD/FreeBSD introduced something similar.
This is a fairly straightforward patch, introduces .VARIABLES corresponding
to the full list of global variables (from the command line and the Makefile)
that have been defined.
(^ says the guy who had to remember a few details from his own(!) var.c
implementation from a few years ago)
I just took var_get_value offline from the old macro, for readability,
even though it's likely the compiler may still decide to inline it.
For efficiency, that list is only computed as needed, since it is
somewhat long.
For debugging purposes, this can come in fairly handy, and I see at
least another application in ports.
Comments and nits welcome.
Note that the list is completely unsorted. This could be sorted through
since we already have the code for dumping purposes, but it would be even
more expensive (the order will be "random", as per the hash)
Index: var.c
===
RCS file: /cvs/src/usr.bin/make/var.c,v
retrieving revision 1.104
diff -u -p -r1.104 var.c
--- var.c 9 Jun 2022 13:13:14 - 1.104
+++ var.c 7 Aug 2023 14:33:42 -
@@ -104,6 +104,8 @@ static char varNoError[] = "";
bool errorIsOkay;
static boolcheckEnvFirst; /* true if environment should be searched for
* variables before the global context */
+ /* do we need to recompute varname_list */
+static boolvarname_list_changed = true;
void
Var_setCheckEnvFirst(bool yes)
@@ -228,9 +230,12 @@ typedef struct Var_ {
*/
#define POISONS (POISON_NORMAL | POISON_EMPTY | POISON_NOT_DEFINED)
/* Defined in var.h */
+#define VAR_IS_NAMES 1024/* Very expensive, only defined when needed */
char name[1]; /* the variable's name */
} Var;
+/* for GNU make compatibility */
+#define VARNAME_LIST ".VARIABLES"
static struct ohash_info var_info = {
offsetof(Var, name),
@@ -245,10 +250,11 @@ static void fill_from_env(Var *);
static Var *create_var(const char *, const char *);
static void var_set_initial_value(Var *, const char *);
static void var_set_value(Var *, const char *);
-#define var_get_value(v) ((v)->flags & VAR_EXEC_LATER ? \
- var_exec_cmd(v) : \
- Buf_Retrieve(&((v)->val)))
-static char *var_exec_cmd(Var *);
+static char *var_get_value(Var *);
+static void var_exec_cmd(Var *);
+static void varname_list_retrieve(Var *);
+
+
static void var_append_value(Var *, const char *);
static void poison_check(Var *);
static void var_set_append(const char *, const char *, const char *, int,
bool);
@@ -423,6 +429,7 @@ var_set_initial_value(Var *v, const char
len = strlen(val);
Buf_Init(&(v->val), len+1);
Buf_AddChars(&(v->val), len, val);
+ varname_list_changed = true;
}
/* Normal version of var_set_value(), to be called after variable is fully
@@ -440,6 +447,16 @@ var_set_value(Var *v, const char *val)
}
}
+static char *
+var_get_value(Var *v)
+{
+ if (v->flags & VAR_IS_NAMES)
+ varname_list_retrieve(v);
+ else if (v->flags & VAR_EXEC_LATER)
+ var_exec_cmd(v);
+ return Buf_Retrieve(&(v->val));
+}
+
/* Add to a variable, insert a separating space if the variable was already
* defined.
*/
@@ -628,6 +645,7 @@ Var_Deletei(const char *name, const char
ohash_remove(&global_variables, slot);
delete_var(v);
+ varname_list_changed = true;
}
/* Set or add a global variable, either to VAR_CMD or VAR_GLOBAL.
@@ -687,7 +705,7 @@ Var_Appendi_with_ctxt(const char *name,
var_set_append(name, ename, val, ctxt, true);
}
-static char *
+static void
var_exec_cmd(Var *v)
{
char *arg = Buf_Retrieve(&(v->val));
@@ -699,7 +717,27 @@ var_exec_cmd(Var *v)
var_set_value(v, res1);
free(res1);
v->flags &= ~VAR_EXEC_LATER;
- return Buf_Retrieve(&(v->val));
+}
+
+static void
+varname_list_retrieve(Var *v)
+{
+ unsigned int i;
+ void *e;
+ bool first = true;
+
+ for (e = ohash_first(&global_variables, &i); e != NULL;
+ e = ohash_next(&global_variables, &i)) {
+ Var *v2 = e;
+ if (v2->flags & VAR_DUMMY)
+ continue;
+
+ if (first)
+ var_set_value(v, v2->name);
+ else
+ var_append_value(v, v2->name);
+ first = false;
+ }
}
/* XXX different semantics for Var_Valuei() and Var_Definedi():
@@ -1339,6 +1377,19 @@ set_magic_shell_variable()
v->flags = VAR_IS_SHELL | VAR_SEEN_ENV;
}
+static void
+set_magic_name_list_varia
Re: ldomctl: status: Make stopped ldom utilization appear as zero
On Sat, Apr 22, 2023 at 12:00:44AM +, Klemens Nanni wrote:
> On Fri, Apr 21, 2023 at 11:29:11PM +, Koakuma wrote:
> > I noticed that when using `ldomctl status` the utilization value of
> > stopped ldoms is always a copy of the previous entry's value,
> > which is probably incorrect?
Right, it sticks to the value of the last running guest, that's wrong.
> >
> > Zeroing utilization value in `ldomctl status` at the start of the loop
> > makes it so that stopped ldoms' utilization appear as a zero, which
> > I believe is a more appropriate value in this context.
> >
> > Before:
> > primary -running OpenBSD running
> > 11%
> > gentoo ttyV0running Linux running
> > 0%
> > openbsd ttyV1running OpenBSD running
> > 5%
> > solaris ttyV2stopped -
> > 5%
> >
> > After:
> > primary -running OpenBSD running
> > 1%
> > gentoo ttyV0running Linux running
> > 0%
> > openbsd ttyV1running OpenBSD running
> > 8%
> > solaris ttyV2stopped -
> > 0%
> >
> > Any comments?
>
> That explains why I saw >0% after i stopped a guest that caused problems...
> thought it was a bug nastier than this, but didn't look and forgot.
>
> Makes sense, I'll test in a few days and come back.
Works as expected, thanks!
>
> >
> > diff --git usr.sbin/ldomctl/ldomctl.c usr.sbin/ldomctl/ldomctl.c
> > index e48a560f7db..3a1b47cc1dc 100644
> > --- usr.sbin/ldomctl/ldomctl.c
> > +++ usr.sbin/ldomctl/ldomctl.c
> > @@ -574,6 +574,8 @@ guest_status(int argc, char **argv)
> > if (gid != -1 && guest->gid != gid)
> > continue;
> >
> > + utilisation = 0.0;
> > +
> > /*
> > * Request status.
> > */
I'd reset after the comment and struct msg setup,
closer to the switch block which may set the value.
Feedback? OK?
> > @@ -644,8 +646,6 @@ guest_status(int argc, char **argv)
> > if (yielded_cycles <= total_cycles)
> > utilisation = (100.0 * (total_cycles
> > - yielded_cycles)) / total_cycles;
> > - else
> > - utilisation = 0.0;
> >
> > break;
> > case GUEST_STATE_SUSPENDED:
> >
>
Index: ldomctl.c
===
RCS file: /cvs/src/usr.sbin/ldomctl/ldomctl.c,v
retrieving revision 1.40
diff -u -p -r1.40 ldomctl.c
--- ldomctl.c 24 Oct 2021 21:24:18 - 1.40
+++ ldomctl.c 7 Aug 2023 14:15:08 -
@@ -592,6 +592,8 @@ guest_status(int argc, char **argv)
if (nbytes != sizeof(msg))
err(1, "read");
+ utilisation = 0.0;
+
memcpy(&state, msg.msg.resstat.data, sizeof(state));
switch (state.state) {
case GUEST_STATE_STOPPED:
@@ -644,8 +646,6 @@ guest_status(int argc, char **argv)
if (yielded_cycles <= total_cycles)
utilisation = (100.0 * (total_cycles
- yielded_cycles)) / total_cycles;
- else
- utilisation = 0.0;
break;
case GUEST_STATE_SUSPENDED:
Re: installer: always create new softraid volume
Klemens Nanni: > If the root disk contains a valid CRYPTO volume, bioctl(8) by default > unlocks that instead of creating a new one. > > Use `-C force' to prevent reuse of old volumes, Yes, I like this. During testing I ran into the case where it re-used an existing crypto volume and I thought to myself, "hmm, this is probably not what a user would expect". -- Christian "naddy" Weisgerber na...@mips.inka.de
