Re: hostctl: Change from fixed length to variable length
YASUOKA Masahiko wrote: > Currently the value on VMware may be truncated silently. It's simply > broken. I think we should fix it by having a way to know if the value > is reached the limit. > > Also I think we should be able to pass larger size of data. Since at > least on VMware, people is useing for parameters when deployment > through OVF tamplate. Sometimes the parameter includes large data > like X.509 certificate. > > https://docs.vmware.com/en/VMware-vSphere/7.0/com.vmware.vsphere.vm_admin.doc/GUID-D0F9E3B9-B77B-4DEF-A982-49B9F6358FF3.html > > What do you think? > > > Prepare a variable like kern.maxpvbus and default it to > > 4096. Futhermore, how about free() after copyout() to user space? > > I suppose we can use the space prepared by the userland directly. An example of this mechanism is SIOCGIFCONF. The ioctl passes a pointer to a struct containing length & pointer to data. See net/if.c ifconf() There are other similar schemes, but they all come down to asking the kernel for the size and then doing a 2nd ioctl. Or a 3rd or more calls, in case the value has changed in the meantime and grown even further, but userland can realloc() the storage until it wins.
Re: sparc64: ofwboot: no incremental builds due to broken libsa/libz dep
On Wed, 12 Oct 2022 03:01:04 +0400, Klemens Nanni wrote: > sparc64 also has its own under stand/, > or do you mean that the alpha one is worse than the sparc64? Oh yes, the alpha one is needlessly complicated. I suppose this is not surprising considering the source. - todd
ps.1: control terminal -> controlling terminal
Index: ps.1 === RCS file: /cvs/src/bin/ps/ps.1,v retrieving revision 1.128 diff -u -p -r1.128 ps.1 --- ps.13 Sep 2022 15:59:04 - 1.128 +++ ps.111 Oct 2022 23:20:34 - @@ -443,7 +443,8 @@ information: .Pp .Bl -tag -width indent -compact .It + -The process is in the foreground process group of its control terminal. +The process is in the foreground process group of its controlling +terminal. .It \*(Lt The process has a raised CPU scheduling priority (see @@ -508,7 +509,7 @@ Saved GID from a setgid executable. .It Cm svuid Saved UID from a setuid executable. .It Cm tdev -Control terminal device number. +Controlling terminal device number. .It Cm tid Thread ID. Used together with @@ -518,11 +519,11 @@ Alias: .Cm cputime . Accumulated CPU time, user + system. .It Cm tpgid -Control terminal process group ID. +Controlling terminal process group ID. .\".It trss .\"Text resident set size, in Kilobytes. .It Cm tsess -Control terminal session pointer. +Controlling terminal session pointer. .It Cm tsiz Text size, in Kilobytes. .It Cm tt @@ -536,7 +537,7 @@ This is followed by a if the process can no longer reach that controlling terminal (i.e. it has been revoked). .It Cm tty -Full name of control terminal. +Full name of controlling terminal. .It Cm ucomm Alias: .Cm comm .
Re: sparc64: ofwboot: no incremental builds due to broken libsa/libz dep
12 Oct 2022 02:22:15 Todd C. Miller : > This problem is not confined to sparc64 is it? At least macppc > seems to have the same issue. Looks like it, I have not yet tested other architectures. > Some, like amd64 and i386, cheat and > just pull in the 4 source files from the libz directory. Yes, that's what I meant with pxeboot. > It looks like alpha has its own libz and libsa directories which > seems even worse. sparc64 also has its own under stand/, or do you mean that the alpha one is worse than the sparc64?
Re: hostctl: Change from fixed length to variable length
Hello, On Wed, 05 Oct 2022 13:37:35 +0900 (JST) Masato Asou wrote: > From: "Theo de Raadt" > Date: Tue, 04 Oct 2022 21:58:13 -0600 >> Userland may not ask the kernel to allocate such a huge object. The >> kernel address space is quite small. First off, huge allocations will >> fail. >> >> But it is worse -- the small kernel address space is shared for many >> purposes, so large allocations will harm the other subsystems. >> >> As written, this diff is too dangerous. Arbitrary allocation inside >> the kernel is not reasonable. object sizes requested by userland must >> be small, or the operations must be cut up, which does create impact >> on atomicity or other things. > > As you pointed out, it is not a good idea to allocate large spaces > in kernel. > > Would it be better to keep the current fixed length? Currently the value on VMware may be truncated silently. It's simply broken. I think we should fix it by having a way to know if the value is reached the limit. Also I think we should be able to pass larger size of data. Since at least on VMware, people is useing for parameters when deployment through OVF tamplate. Sometimes the parameter includes large data like X.509 certificate. https://docs.vmware.com/en/VMware-vSphere/7.0/com.vmware.vsphere.vm_admin.doc/GUID-D0F9E3B9-B77B-4DEF-A982-49B9F6358FF3.html What do you think? > Prepare a variable like kern.maxpvbus and default it to > 4096. Futhermore, how about free() after copyout() to user space? I suppose we can use the space prepared by the userland directly.
Re: sparc64: ofwboot: no incremental builds due to broken libsa/libz dep
This problem is not confined to sparc64 is it? At least macppc seems to have the same issue. Some, like amd64 and i386, cheat and just pull in the 4 source files from the libz directory. It looks like alpha has its own libz and libsa directories which seems even worse. - todd
sparc64: ofwboot: no incremental builds due to broken libsa/libz dep
Testing the libz size fix in ofwboot.net showed that rebuilding ofwboot
after updating libz sources did not relink ofwboot as it should, imho:
$ cd sys/arch/sparc64/stand
$ make
...
$ touch ../../../lib/libz/crc32.c
$ make
===> bootblk
===> libsa
===> libz
cc -O2 -pipe -g -fno-stack-protector -msoft-float -ffreestanding -MD
-MP -D_LP64 -g -Wa,-Av9a -I/usr/src/sys/arch/sparc64/stand/libz/../../../..
-fno-pie -DSLOW -DSMALL -DNOBYFOUR -DNO_GZIP -I. -D_STANDALONE -DSUN4U -I.
-DDYNAMIC_CRC_TABLE -DBUILDFIXED -c
/usr/src/sys/arch/sparc64/stand/libz/../../../../lib/libz/crc32.c -o crc32.o
building standard z library
ranlib libz.a
===> ofwboot
===> ofwboot.net
===> ofwbootfd
ofwboot/Makefile defines LIBSA and LIBZ as prerequisite for PROG, but the
two former variables are not defined until which includes
"../Makefile.inc" which defines them.
It still works because a) libsa and libz are always built before ofwboot,
thus the intended but effectively missing dependency is always there, and
b) make(1) variables inside commands are evaluated lazily at run-time,
while those in targets and prerequisites are evaluated at parse-time:
$ make -C ofwboot -p | grep -C1 ^ofwboot
ofwboot : /usr/lib/crt0.o srt0.o Locore.o alloc.o boot.o
elf64_exec.o arc4.o net.o netif_of.o ofdev.o vers.o diskprobe.o
softraid_sparc64.o strlcpy.o strcmp.o strlcat.o strlen.o ffs.o aes_xts.o
bcrypt_pbkdf.o blowfish.o explicit_bzero.o hmac_sha1.o pkcs5_pbkdf2.o
rijndael.o sha1.o sha2.o softraid.o /usr/lib/libc.a /usr/lib/crtbegin.o
/usr/lib/crtend.o
${LD} -N -Ttext ${RELOC} -e ${ENTRY} -o ${PROG} -nopie
-znorelro ${OBJS} -L${LIBSADIR} ${LIBSA} -L${LIBZDIR} ${LIBZ}
Diff below fixes this, but I doubt this is the way to go:
# make -C ofwboot -p | grep -C1 ^ofwboot
# parent targets: all
ofwboot :
/usr/src/sys/arch/sparc64/stand/ofwboot/../libsa//libsa.a
/usr/src/sys/arch/sparc64/stand/ofwboot/../libz//libz.a /usr/lib/crt0.o srt0.o
Locore.o alloc.o boot.o elf64_exec.o arc4.o net.o netif_of.o ofdev.o vers.o
diskprobe.o softraid_sparc64.o strlcpy.o strcmp.o strlcat.o strlen.o ffs.o
aes_xts.o bcrypt_pbkdf.o blowfish.o explicit_bzero.o hmac_sha1.o pkcs5_pbkdf2.o
rijndael.o sha1.o sha2.o softraid.o /usr/lib/libc.a /usr/lib/crtbegin.o
/usr/lib/crtend.o
${LD} -N -Ttext ${RELOC} -e ${ENTRY} -o ${PROG} -nopie
-znorelro ${OBJS} -L${LIBSADIR} ${LIBSA} -L${LIBZDIR} ${LIBZ}
# touch ../../../lib/libz/crc32.c
# make
===> bootblk
===> libsa
===> libz
cc -O2 -pipe -g -fno-stack-protector -msoft-float -ffreestanding -MD
-MP -D_LP64 -g -Wa,-Av9a -I/usr/src/sys/arch/sparc64/stand/libz/../../../..
-fno-pie -DSLOW -DSMALL -DNOBYFOUR -DNO_GZIP -I. -D_STANDALONE -DSUN4U -I.
-DDYNAMIC_CRC_TABLE -DBUILDFIXED -c
/usr/src/sys/arch/sparc64/stand/libz/../../../../lib/libz/crc32.c -o crc32.o
building standard z library
ranlib libz.a
===> ofwboot
Warning: target
/usr/src/sys/arch/sparc64/stand/ofwboot/../libsa//libsa.a (prerequisite of:
ofwboot) does not have any command (BUG)
Warning: target /usr/src/sys/arch/sparc64/stand/ofwboot/../libz//libz.a
(prerequisite of: ofwboot) does not have any command (BUG)
ld -N -Ttext 10 -e _start -o ofwboot -nopie -znorelro srt0.o
Locore.o alloc.o boot.o elf64_exec.o arc4.o net.o netif_of.o ofdev.o vers.o
diskprobe.o softraid_sparc64.o strlcpy.o strcmp.o strlcat.o strlen.o ffs.o
aes_xts.o bcrypt_pbkdf.o blowfish.o explicit_bzero.o hmac_sha1.o pkcs5_pbkdf2.o
rijndael.o sha1.o sha2.o softraid.o
-L/usr/src/sys/arch/sparc64/stand/ofwboot/../libsa
/usr/src/sys/arch/sparc64/stand/ofwboot/../libsa/obj/libsa.a
-L/usr/src/sys/arch/sparc64/stand/ofwboot/../libz
/usr/src/sys/arch/sparc64/stand/ofwboot/../libz/obj/libz.a
===> ofwboot.net
Warning: target
/usr/src/sys/arch/sparc64/stand/ofwboot.net/../libsa//libsa.a (prerequisite of:
ofwboot.net) does not have any command (BUG)
Warning: target
/usr/src/sys/arch/sparc64/stand/ofwboot.net/../libz//libz.a (prerequisite of:
ofwboot.net) does not have any command (BUG)
ld -N -Ttext 10 -e _start -o ofwboot.net -nopie -znorelro srt0.o
Locore.o alloc.o boot.o elf64_exec.o arc4.o net.o netif_of.o ofdev.o vers.o
strlcpy.o strcmp.o strlcat.o strlen.o ffs.o
-L/usr/src/sys/arch/sparc64/stand/ofwboot.net/../libsa
/usr/src/sys/arch/sparc64/stand/ofwboot.net/../libsa/obj/libsa.a
-L/usr/src/sys/arch/sparc64/stand/ofwboot.net/../libz
/usr/src/sys/arch/sparc64/stand/ofwboot.net/../libz/obj/libz.a
===> ofwbootfd
Warning: target
/usr/src/sys/arch/sparc64/stand/ofwbootfd/../libsa//libsa.a (prerequisite of:
ofwbootfd) does not have any command (BUG)
Warning: target
vmd: allow agentx to reconnect closed components
This builds on top of the previous two diffs. Easiest way to test is to add the following line to vm.conf in combination with snmpd(8): agentx context foo This results in: [fd:6 sess:4009008336 ctx:foo]: region .1.3.6.1.2.1.236: opening [fd:6 sess:4009008336 ctx:foo]: region .1.3.6.1.2.1.236: Unsupported context If you then remove the "context foo" part and run vmctl reload vmd should properly register: [fd:6 sess:4009008336 ctx:]: region .1.3.6.1.2.1.236: opening [fd:6 sess:4009008336 ctx:]: region .1.3.6.1.2.1.236: open OK? martijn@ Index: vm_agentx.c === RCS file: /cvs/src/usr.sbin/vmd/vm_agentx.c,v retrieving revision 1.1 diff -u -p -r1.1 vm_agentx.c --- vm_agentx.c 13 Sep 2022 10:28:19 - 1.1 +++ vm_agentx.c 11 Oct 2022 18:52:45 - @@ -321,6 +321,7 @@ vm_agentx_configure(struct vmd_agentx *e changed = 1; } + agentx_retry(conn->agentx); if (!changed) return;
libagentx: Allow not enabled components to retry
This one is on top of the don't reset errors diff.
This diff includes a minor bump. Don't know if we're still to close
after unlock.
If certain components (session, agentcaps, region, index, object) are
in a closed state while they are expected to be open, there is no way
to retry them without destroying the entire structure and rebuilding
it from scratch.
This diff adds agentx_retry, which walks the tree and retries all in
state AX_CSTATE_CLOSE.
This function could be triggered by the admin, e.g. on a daemon
reload.
OK? If so, now?
martijn@
diff --git a/Symbols.list b/Symbols.list
index 6eda2be..a5c8bf7 100644
--- a/Symbols.list
+++ b/Symbols.list
@@ -4,6 +4,7 @@ agentx_log_info
agentx_log_debug
agentx
agentx_connect
+agentx_retry
agentx_read
agentx_write
agentx_wantwrite
diff --git a/agentx.3 b/agentx.3
index d45a3ae..81322c6 100644
--- a/agentx.3
+++ b/agentx.3
@@ -24,6 +24,7 @@
.Nm agentx_log_debug ,
.Nm agentx ,
.Nm agentx_connect ,
+.Nm agentx_retry ,
.Nm agentx_read ,
.Nm agentx_write ,
.Nm agentx_wantwrite ,
@@ -95,6 +96,8 @@
.Ft void
.Fn agentx_connect "struct agentx *sa" "int fd"
.Ft void
+.Fn agentx_retry "struct agentx *sa"
+.Ft void
.Fn agentx_read "struct agentx *sa"
.Ft void
.Fn agentx_write "struct agentx *sa"
@@ -367,6 +370,12 @@ is ready for a write, the function
.Fn agentx_write
should be called.
.Pp
+If any of the session, agentcaps, region, index, or objects failed to enable
+correctly
+.Pq as can be seen by the admin through the logs
+they can be retried through
+.Fn agentx_retry.
+.Pp
.Fa sa
can be freed via
.Fn agentx_free .
diff --git a/agentx.c b/agentx.c
index 8df1032..5f4f382 100644
--- a/agentx.c
+++ b/agentx.c
@@ -133,6 +133,7 @@ void (*agentx_wantwrite)(struct agentx *, int) =
agentx_wantwritenow;
static void agentx_reset(struct agentx *);
static void agentx_free_finalize(struct agentx *);
+static int agentx_session_retry(struct agentx_session *);
static int agentx_session_start(struct agentx_session *);
static int agentx_session_finalize(struct ax_pdu *, void *);
static int agentx_session_close(struct agentx_session *,
@@ -140,6 +141,7 @@ static int agentx_session_close(struct agentx_session *,
static int agentx_session_close_finalize(struct ax_pdu *, void *);
static void agentx_session_free_finalize(struct agentx_session *);
static void agentx_session_reset(struct agentx_session *);
+static int agentx_context_retry(struct agentx_context *);
static void agentx_context_start(struct agentx_context *);
static void agentx_context_free_finalize(struct agentx_context *);
static void agentx_context_reset(struct agentx_context *);
@@ -149,6 +151,7 @@ static int agentx_agentcaps_close(struct agentx_agentcaps
*);
static int agentx_agentcaps_close_finalize(struct ax_pdu *, void *);
static void agentx_agentcaps_free_finalize(struct agentx_agentcaps *);
static void agentx_agentcaps_reset(struct agentx_agentcaps *);
+static int agentx_region_retry(struct agentx_region *);
static int agentx_region_start(struct agentx_region *);
static int agentx_region_finalize(struct ax_pdu *, void *);
static int agentx_region_close(struct agentx_region *);
@@ -229,6 +232,25 @@ agentx_connect(struct agentx *ax, int fd)
agentx_finalize(ax, fd);
}
+void
+agentx_retry(struct agentx *ax)
+{
+ struct agentx_session *axs;
+
+ if (ax->ax_fd == -1)
+ return;
+
+ TAILQ_FOREACH(axs, &(ax->ax_sessions), axs_ax_sessions) {
+ if (axs->axs_cstate == AX_CSTATE_OPEN) {
+ if (agentx_session_retry(axs) == -1)
+ return;
+ } else if (axs->axs_cstate == AX_CSTATE_CLOSE) {
+ if (agentx_session_start(axs) == -1)
+ return;
+ }
+ }
+}
+
static void
agentx_start(struct agentx *ax)
{
@@ -401,6 +423,26 @@ agentx_session(struct agentx *ax, uint32_t oid[],
return axs;
}
+static int
+agentx_session_retry(struct agentx_session *axs)
+{
+ struct agentx_context *axc;
+
+#ifdef AX_DEBUG
+ if (axs->axs_cstate != AX_CSTATE_OPEN)
+ agentx_log_axs_fatalx(axs, "%s: unexpected retry", __func__);
+#endif
+
+ TAILQ_FOREACH(axc, &(axs->axs_contexts), axc_axs_contexts) {
+ if (axc->axc_cstate == AX_CSTATE_OPEN) {
+ if (agentx_context_retry(axc) == -1)
+ return -1;
+ } else if (axc->axc_cstate == AX_CSTATE_CLOSE)
+ agentx_context_start(axc);
+ }
+ return 0;
+}
+
static int
agentx_session_start(struct agentx_session *axs)
{
@@ -628,6 +670,36 @@ agentx_context(struct agentx_session *axs, const char
*name)
return axc;
}
+static int
+agentx_context_retry(struct agentx_context *axc)
+{
+ struct agentx_agentcaps *axa;
+ struct agentx_region *axr;
+
+#ifdef AX_DEBUG
+ if (axc->axc_cstate != AX_CSTATE_OPEN)
+
libagentx: don't reset on server errors
There's a couple of cases in libagentx where we call agentx_reset on
error cases returned by the server, which in turn results in the
connection being closed and retried.
There's no reason to expect the server to return another response on
the next try. I think it's better to just keep the different structs
in a closed state and just miss that particular part of the
functionality until it's fixed.
Found by explicitly setting vmd's agentx context, which is supported
by net-snmpd, but not by snmpd(8), which just returns an error and
creates an infinite loop.
OK?
martijn
diff --git a/agentx.c b/agentx.c
index 3ee05e6..8df1032 100644
--- a/agentx.c
+++ b/agentx.c
@@ -444,7 +444,7 @@ agentx_session_finalize(struct ax_pdu *pdu, void *cookie)
if (pdu->ap_payload.ap_response.ap_error != AX_PDU_ERROR_NOERROR) {
agentx_log_ax_warnx(ax, "failed to open session: %s",
ax_error2string(pdu->ap_payload.ap_response.ap_error));
- agentx_reset(ax);
+ axs->axs_cstate = AX_CSTATE_CLOSE;
return -1;
}
@@ -1112,8 +1112,6 @@ agentx_region_finalize(struct ax_pdu *pdu, void *cookie)
{
struct agentx_region *axr = cookie;
struct agentx_context *axc = axr->axr_axc;
- struct agentx_session *axs = axc->axc_axs;
- struct agentx *ax = axs->axs_ax;
struct agentx_index *axi;
struct agentx_object *axo;
@@ -1140,22 +1138,11 @@ agentx_region_finalize(struct ax_pdu *pdu, void *cookie)
agentx_log_axc_info(axc, "region %s: duplicate, can't "
"reduce priority, ignoring",
ax_oid2string(&(axr->axr_oid)));
- } else if (pdu->ap_payload.ap_response.ap_error ==
- AX_PDU_ERROR_REQUESTDENIED) {
+ } else {
axr->axr_cstate = AX_CSTATE_CLOSE;
agentx_log_axc_warnx(axc, "region %s: %s",
ax_oid2string(&(axr->axr_oid)),
ax_error2string(pdu->ap_payload.ap_response.ap_error));
- /*
-* If we can't register a region, related objects are useless.
-* But no need to retry.
-*/
- return 0;
- } else {
- agentx_log_axc_info(axc, "region %s: %s",
- ax_oid2string(&(axr->axr_oid)),
- ax_error2string(pdu->ap_payload.ap_response.ap_error));
- agentx_reset(ax);
return -1;
}
@@ -1648,8 +1635,6 @@ agentx_index_finalize(struct ax_pdu *pdu, void *cookie)
struct agentx_index *axi = cookie;
struct agentx_region *axr = axi->axi_axr;
struct agentx_context *axc = axr->axr_axc;
- struct agentx_session *axs = axc->axc_axs;
- struct agentx *ax = axs->axs_ax;
struct ax_pdu_response *resp;
size_t i;
@@ -1675,20 +1660,20 @@ agentx_index_finalize(struct ax_pdu *pdu, void *cookie)
if (resp->ap_nvarbind != 1) {
agentx_log_axc_warnx(axc, "index %s: unexpected number of "
"indices", ax_oid2string(&(axr->axr_oid)));
- agentx_reset(ax);
+ axi->axi_cstate = AX_CSTATE_CLOSE;
return -1;
}
if (resp->ap_varbindlist[0].avb_type != axi->axi_vb.avb_type) {
agentx_log_axc_warnx(axc, "index %s: unexpected index type",
ax_oid2string(&(axr->axr_oid)));
- agentx_reset(ax);
+ axi->axi_cstate = AX_CSTATE_CLOSE;
return -1;
}
if (ax_oid_cmp(&(resp->ap_varbindlist[0].avb_oid),
&(axi->axi_vb.avb_oid)) != 0) {
agentx_log_axc_warnx(axc, "index %s: unexpected oid",
ax_oid2string(&(axr->axr_oid)));
- agentx_reset(ax);
+ axi->axi_cstate = AX_CSTATE_CLOSE;
return -1;
}
@@ -1702,7 +1687,7 @@ agentx_index_finalize(struct ax_pdu *pdu, void *cookie)
resp->ap_varbindlist[0].avb_data.avb_int32) {
agentx_log_axc_warnx(axc, "index %s: unexpected "
"index value", ax_oid2string(&(axr->axr_oid)));
- agentx_reset(ax);
+ axi->axi_cstate = AX_CSTATE_CLOSE;
return -1;
}
agentx_log_axc_info(axc, "index %s: allocated '%d'",
sparc64: miniroot: add mount_nfs(8) to fetch sets over NFS
sparc64 RAMDISK already has NFSCLIENT enabled and miniroot's FSSIZE still fits after linking mount_nfs: /dev/vnd0a6312 5592 72089% 3 59 5% /mnt This boots fine on T4-2 guest domains and I can upgrade via NFS: Location of sets? (disk http nfs or 'done') [http] nfs Server IP address or hostname? 192.168.42.1 Filesystem on server to mount? /home/snapshots Use TCP transport? (requires TCP-capable NFS server) [no] Pathname to the sets? (or 'done') [7.2/sparc64] / Select sets by entering a set name, a file name pattern or 'all'. De-select sets by prepending a '-', e.g.: '-game*'. Selected sets are labelled '[X]'. [X] bsd [X] base72.tgz[X] game72.tgz[X] xfont72.tgz [X] bsd.mp[X] comp72.tgz[X] xbase72.tgz [X] xserv72.tgz [X] bsd.rd[X] man72.tgz [X] xshare72.tgz Set name(s)? (or 'abort' or 'done') [done] ... CONGRATULATIONS! Your OpenBSD upgrade has been successfully completed! Feedback? OK? Index: distrib/sparc64/miniroot/list === RCS file: /cvs/src/distrib/sparc64/miniroot/list,v retrieving revision 1.10 diff -u -p -r1.10 list --- distrib/sparc64/miniroot/list 18 Jul 2021 15:18:49 - 1.10 +++ distrib/sparc64/miniroot/list 11 Oct 2022 17:36:16 - @@ -38,6 +38,7 @@ LINK instbin sbin/mount LINK instbin sbin/mount_cd9660 LINK instbin sbin/mount_ffs LINK instbin sbin/mount_msdos +LINK instbin sbin/mount_nfs LINK instbin sbin/newfs LINK instbin sbin/ping sbin/ping6 LINK instbin sbin/reboot sbin/halt
Re: em(4) IPv4, TCP, UDP checksum offloading
On 11.10.2022. 17:16, Stuart Henderson wrote: Hi, > I tried this on my laptop which has I219-V em (I run it in a trunk > with iwm). It breaks tx (packets don't show up on the other side). > rx seems ok. > > There is also a "em0: watchdog: head 111 tail 20 TDH 20 TDT 111" this em log can be triggered with or without em offload diff¸with sh /etc/netstart, but it seems that with this diff it's little easier to trigger it ... > but that was after ~10 mins uptime so may have occurred after > I switched the active port on the trunk(4) over to the iwm, or > back again.
Re: em(4) IPv4, TCP, UDP checksum offloading
On 11.10.2022. 15:03, Moritz Buhl wrote: > Here is a new diff for checksum offloading (ipv4, udp, tcp) for em(4). > > The previous diff didn't implement hardware vlan tagging for >em82578 > which should result in variable ethernet header lengths and thus > wrong checksums inserted at wrong places. > > The diff below addresses this. > I would appreciate further testing reports with different controllers. Hi, what would be the best thing to do to test this diff? I have box with em0 at pci7 dev 0 function 0 "Intel 82576" rev 0x01: msi, address 00:1b:21:61:8a:94 em1 at pci7 dev 0 function 1 "Intel 82576" rev 0x01: msi, address 00:1b:21:61:8a:95 em2 at pci8 dev 0 function 0 "Intel I210" rev 0x03: msi, address 00:25:90:5d:c9:98 em3 at pci9 dev 0 function 0 "Intel I210" rev 0x03: msi, address 00:25:90:5d:c9:99 em4 at pci12 dev 0 function 0 "Intel I350" rev 0x01: msi, address 00:25:90:5d:c9:9a em5 at pci12 dev 0 function 1 "Intel I350" rev 0x01: msi, address 00:25:90:5d:c9:9b em6 at pci12 dev 0 function 2 "Intel I350" rev 0x01: msi, address 00:25:90:5d:c9:9c em7 at pci12 dev 0 function 3 "Intel I350" rev 0x01: msi, address 00:25:90:5d:c9:9d after this diff I'm seeing em0: flags=8843 mtu 1500 hwfeatures=1b7 hardmtu 9216 lladdr 00:1b:21:61:8a:94 em1: flags=8843 mtu 1500 hwfeatures=1b7 hardmtu 9216 lladdr 00:1b:21:61:8a:95 em2: flags=8843 mtu 1500 hwfeatures=1b7 hardmtu 9216 lladdr 00:25:90:5d:c9:98 em3: flags=8843 mtu 1500 hwfeatures=1b7 hardmtu 9216 lladdr 00:25:90:5d:c9:99 em4: flags=8843 mtu 1500 hwfeatures=1b7 hardmtu 9216 lladdr 00:25:90:5d:c9:9a em5: flags=8843 mtu 1500 hwfeatures=1b7 hardmtu 9216 lladdr 00:25:90:5d:c9:9b em6: flags=8843 mtu 1500 hwfeatures=1b7 hardmtu 9216 lladdr 00:25:90:5d:c9:9c em7: flags=8802 mtu 1500 hwfeatures=1b7 hardmtu 9216 lladdr 00:25:90:5d:c9:9d and basic tcp/udp iperf is working as expected over plain interface.
Re: sysupgrade: exit 1 instead of exit 0 when ending early
On Tue, Oct 11, 2022 at 01:32:43PM +0200, Janne Johansson wrote: > > On Mon, Oct 10, 2022 at 11:17:32AM -0600, Theo de Raadt wrote: > > > It's been explained a few times that being up-to-date is not an error. > > > It's a good thing, and no action is neccessary when up-to-date. > > > Any non-zero value indicates an error, that would include 2. You are > > > marking this as an error, when it isn't. > > > > It's been said that being up-to-date is not an error, but if it's been > > explained, I've failed to find an explanation. > > > > Usually, when a utility fails to perform its intended task, it gives an > > error. > > I don't personally care what exit code it throws, I only use the tool > > manually, I'd just like to know the rationale if anyone cares to > > elaborate. > > Simplest explanation is probably: > > ./some-program && do something on success > > If you understand this part of shell scripting, then you would see why > returning > 2 (or 1 or 3 or 102444) will throw off the logic, especially if 1 > means some kind > of error. > > As for "intended task", sysupgrade might be viewed as a tool to make > sure you have > the latest it knows about. If no upgrade is needed or the mirror is > old, then that task is done. > > Someone else might think it is a tool to stress the network and disk > by always downloading > things from the internet, and that it is a grave error if this can't > be done, but those people > would have a view that differs from the obsd devs idea of what to use > sysupgrade for. > > Also, if you replace your "mkdir /foo with missing /foo" with "install > -d /foo", you'd see it can > run twice without throwing errors. It makes sure a dir named foo is > created if not exists. > That is its task when run with -d. And you can && a command behind it > so it catches errors > if a file/pipe/socket is there instead. Or add -p to mkdir to get the > same behaviour. > > -- > May the most significant bit of your life be positive. Aha. I didn't know install worked that way (though mkdir -p does too, now that I think of it). I guess my actual confusion was perhaps due to this behavior being implied whereas other utilities are rather explicit about it, at least in terms of man files. Thanks!
Re: ksh: errexit executes ERR trap twice
On Mon, 10 Oct 2022 20:58:29 -, Klemens Nanni wrote: > The ERR trap is supposed to be run once, regardless of errexit: > > $ ksh -c 'trap "echo ERR" ERR; false' > ERR > $ ksh -c 'trap "echo ERR" ERR; false' -e > ERR > ERR > > This duplication is a regression of the following bin/ksh/main.c commit: > > revision 1.52 > date: 2013/06/15 17:25:19; author: millert; state: Exp; lines: +5 -1 > ; > Run any pending traps before calling the EXIT or ERR traps when -e > is set. Fixes a bug where we would not run the signal trap if, > for example, ^C was pressed and -e was set. OK espie@ > > Revert it and add a new test, matching bash(1) behaviour: > > $ ./obj/ksh -c 'trap "echo ERR" ERR; false' > ERR > $ ./obj/ksh -c 'trap "echo ERR" ERR; false' -e > ERR > > After that, fix signal handlers (e.g. SIGINT/^C traps) properly. OK. - todd
Re: em(4) IPv4, TCP, UDP checksum offloading
On 2022/10/11 15:03, Moritz Buhl wrote: > Here is a new diff for checksum offloading (ipv4, udp, tcp) for em(4). > > The previous diff didn't implement hardware vlan tagging for >em82578 > which should result in variable ethernet header lengths and thus > wrong checksums inserted at wrong places. > > The diff below addresses this. > I would appreciate further testing reports with different controllers. > > mbuhl I tried this on my laptop which has I219-V em (I run it in a trunk with iwm). It breaks tx (packets don't show up on the other side). rx seems ok. There is also a "em0: watchdog: head 111 tail 20 TDH 20 TDT 111" but that was after ~10 mins uptime so may have occurred after I switched the active port on the trunk(4) over to the iwm, or back again. OpenBSD 7.2-current (GENERIC.MP) #64: Tue Oct 11 14:30:52 BST 2022 st...@bamboo.spacehopper.org:/sys/arch/amd64/compile/GENERIC.MP real mem = 16926281728 (16142MB) avail mem = 16395878400 (15636MB) random: good seed from bootblocks mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 3.1 @ 0x77d49000 (64 entries) bios0: vendor LENOVO version "N2HET63W (1.46 )" date 06/01/2021 bios0: LENOVO 20QF00B2UK acpi0 at bios0: ACPI 6.1 acpi0: sleep states S0 S3 S4 S5 acpi0: tables DSDT FACP SSDT SSDT SSDT SSDT SSDT TPM2 UEFI SSDT HPET APIC MCFG ECDT SSDT SSDT SSDT BOOT SLIC SSDT LPIT WSMT SSDT DBGP DBG2 MSDM BATB NHLT FPDT UEFI acpi0: wakeup devices GLAN(S4) XHC_(S3) XDCI(S4) HDAS(S4) RP01(S4) PXSX(S4) RP02(S4) PXSX(S4) PXSX(S4) RP04(S4) PXSX(S4) RP05(S4) PXSX(S4) RP06(S4) PXSX(S4) RP07(S4) [...] acpitimer0 at acpi0: 3579545 Hz, 24 bits acpihpet0 at acpi0: 2399 Hz acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz, 1795.82 MHz, 06-8e-0c cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,TSC_ADJUST,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,SRBDS_CTRL,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,XSAVEC,XGETBV1,XSAVES cpu0: 32KB 64b/line 8-way D-cache, 32KB 64b/line 8-way I-cache, 256KB 64b/line 4-way L2 cache, 8MB 64b/line 16-way L3 cache cpu0: smt 0, core 0, package 0 mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges cpu0: apic clock running at 24MHz cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4.1.1.1, IBE cpu1 at mainbus0: apid 2 (application processor) cpu1: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz, 1795.82 MHz, 06-8e-0c cpu1: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,TSC_ADJUST,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,SRBDS_CTRL,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,XSAVEC,XGETBV1,XSAVES cpu1: 32KB 64b/line 8-way D-cache, 32KB 64b/line 8-way I-cache, 256KB 64b/line 4-way L2 cache, 8MB 64b/line 16-way L3 cache cpu1: smt 0, core 1, package 0 cpu2 at mainbus0: apid 4 (application processor) cpu2: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz, 1795.82 MHz, 06-8e-0c cpu2: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,TSC_ADJUST,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,SRBDS_CTRL,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,XSAVEC,XGETBV1,XSAVES cpu2: 32KB 64b/line 8-way D-cache, 32KB 64b/line 8-way I-cache, 256KB 64b/line 4-way L2 cache, 8MB 64b/line 16-way L3 cache cpu2: smt 0, core 2, package 0 cpu3 at mainbus0: apid 6 (application processor) cpu3: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz, 1698.73 MHz, 06-8e-0c cpu3: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,TSC_ADJUST,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,SRBDS_CTRL,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,XSAVEC,XGETBV1,XSAVES cpu3: 32KB 64b/line 8-way D-cache, 32KB 64b/line 8-way I-cache, 256KB 64b/line 4-way L2 cache, 8MB 64b/line 16-way L3 cache cpu3: smt 0, core
em(4) IPv4, TCP, UDP checksum offloading
Here is a new diff for checksum offloading (ipv4, udp, tcp) for em(4).
The previous diff didn't implement hardware vlan tagging for >em82578
which should result in variable ethernet header lengths and thus
wrong checksums inserted at wrong places.
The diff below addresses this.
I would appreciate further testing reports with different controllers.
mbuhl
Index: dev/pci/if_em.c
===
RCS file: /mount/openbsd/cvs/src/sys/dev/pci/if_em.c,v
retrieving revision 1.362
diff -u -p -r1.362 if_em.c
--- dev/pci/if_em.c 23 Jun 2022 09:38:28 - 1.362
+++ dev/pci/if_em.c 10 Oct 2022 18:01:19 -
@@ -37,6 +37,8 @@ POSSIBILITY OF SUCH DAMAGE.
#include
#include
+#include
+
/*
* Driver version
*/
@@ -278,6 +280,8 @@ void em_receive_checksum(struct em_softc
struct mbuf *);
u_int em_transmit_checksum_setup(struct em_queue *, struct mbuf *, u_int,
u_int32_t *, u_int32_t *);
+u_int em_tx_ctx_setup(struct em_queue *, struct mbuf *, u_int, u_int32_t *,
+ u_int32_t *);
void em_iff(struct em_softc *);
void em_update_link_status(struct em_softc *);
int em_get_buf(struct em_queue *, int);
@@ -1220,10 +1224,9 @@ em_encap(struct em_queue *que, struct mb
BUS_DMASYNC_POSTREAD | BUS_DMASYNC_POSTWRITE);
}
- if (sc->hw.mac_type >= em_82543 && sc->hw.mac_type != em_82575 &&
- sc->hw.mac_type != em_82576 &&
- sc->hw.mac_type != em_82580 && sc->hw.mac_type != em_i210 &&
- sc->hw.mac_type != em_i350) {
+ if (sc->hw.mac_type >= em_82575) {
+ used += em_tx_ctx_setup(que, m, head, _upper, _lower);
+ } else if (sc->hw.mac_type >= em_82543) {
used += em_transmit_checksum_setup(que, m, head,
_upper, _lower);
} else {
@@ -1278,7 +1281,7 @@ em_encap(struct em_queue *que, struct mb
#if NVLAN > 0
/* Find out if we are in VLAN mode */
- if (m->m_flags & M_VLANTAG) {
+ if (m->m_flags & M_VLANTAG && sc->hw.mac_type < em_82575) {
/* Set the VLAN id */
desc->upper.fields.special = htole16(m->m_pkthdr.ether_vtag);
@@ -1964,17 +1967,14 @@ em_setup_interface(struct em_softc *sc)
ifp->if_capabilities = IFCAP_VLAN_MTU;
#if NVLAN > 0
- if (sc->hw.mac_type != em_82575 && sc->hw.mac_type != em_82580 &&
- sc->hw.mac_type != em_82576 &&
- sc->hw.mac_type != em_i210 && sc->hw.mac_type != em_i350)
- ifp->if_capabilities |= IFCAP_VLAN_HWTAGGING;
+ ifp->if_capabilities |= IFCAP_VLAN_HWTAGGING;
#endif
- if (sc->hw.mac_type >= em_82543 && sc->hw.mac_type != em_82575 &&
- sc->hw.mac_type != em_82576 &&
- sc->hw.mac_type != em_82580 && sc->hw.mac_type != em_i210 &&
- sc->hw.mac_type != em_i350)
+ if (sc->hw.mac_type >= em_82543) {
+ ifp->if_capabilities |= IFCAP_CSUM_IPv4;
ifp->if_capabilities |= IFCAP_CSUM_TCPv4 | IFCAP_CSUM_UDPv4;
+ ifp->if_capabilities |= IFCAP_CSUM_TCPv6 | IFCAP_CSUM_UDPv6;
+ }
/*
* Specify the media types supported by this adapter and register
@@ -2391,6 +2391,108 @@ em_free_transmit_structures(struct em_so
}
}
+u_int
+em_tx_ctx_setup(struct em_queue *que, struct mbuf *mp, u_int head,
+u_int32_t *olinfo_status, u_int32_t *cmd_type_len)
+{
+ struct e1000_adv_tx_context_desc *TD;
+ struct ether_header *eh = mtod(mp, struct ether_header *);
+ struct mbuf *m;
+ uint32_t vlan_macip_lens = 0, type_tucmd_mlhl = 0, mss_l4len_idx = 0;
+ int off = 0, hoff;
+ uint8_t ipproto, iphlen;
+
+ *olinfo_status = 0;
+ *cmd_type_len = 0;
+ TD = (struct e1000_adv_tx_context_desc *)>tx.sc_tx_desc_ring[head];
+
+#if NVLAN > 0
+ if (ISSET(mp->m_flags, M_VLANTAG)) {
+ uint16_t vtag = htole16(mp->m_pkthdr.ether_vtag);
+ vlan_macip_lens |= vtag << E1000_ADVTXD_VLAN_SHIFT;
+ *cmd_type_len |= E1000_ADVTXD_DCMD_VLE;
+ off = 1;
+ }
+#endif
+
+ vlan_macip_lens |= (sizeof(*eh) << E1000_ADVTXD_MACLEN_SHIFT);
+
+ switch (ntohs(eh->ether_type)) {
+ case ETHERTYPE_IP: {
+ struct ip *ip;
+
+ m = m_getptr(mp, sizeof(*eh), );
+ ip = (struct ip *)(mtod(m, caddr_t) + hoff);
+
+ iphlen = ip->ip_hl << 2;
+ ipproto = ip->ip_p;
+
+ type_tucmd_mlhl |= E1000_ADVTXD_TUCMD_IPV4;
+ if (ISSET(mp->m_pkthdr.csum_flags, M_IPV4_CSUM_OUT)) {
+ *olinfo_status |= E1000_TXD_POPTS_IXSM << 8;
+ off = 1;
+ }
+
+ break;
+ }
+#ifdef INET6
+
Re: sysupgrade: exit 1 instead of exit 0 when ending early
> After reading the script, I'd nitpick that it merely advances the > version by one step rather than ensures the system is up-to-date, but I Then again, this seem to match exactly what the manpage says it should be doing: sysupgrade is a utility to upgrade OpenBSD to the next release or a new snapshot if available. -- May the most significant bit of your life be positive.
Re: sysupgrade: exit 1 instead of exit 0 when ending early
On Tue, Oct 11, 2022 at 01:34:08PM +0200, Renaud Allard wrote: > > > On 10/11/22 13:10, bug wrote: > > On Mon, Oct 10, 2022 at 11:17:32AM -0600, Theo de Raadt wrote: > > > It's been explained a few times that being up-to-date is not an error. > > > It's a good thing, and no action is neccessary when up-to-date. > > > > > > Any non-zero value indicates an error, that would include 2. You are > > > marking this as an error, when it isn't. > > > > It's been said that being up-to-date is not an error, but if it's been > > explained, I've failed to find an explanation. > > It didn't fail, your system is up-to-date as requested. So, it's successful. > > > > > Usually, when a utility fails to perform its intended task, it gives an > > error. This includes when the task is not necessary, e.g. using rm to > > remove a file which doesn't exist, using mkdir to make a directory which > > already exists, or using gzip to compress a file that wouldn't benefit > > from compression (unless you tell it to do so anyway, of course). > > > > I'm not an expert on sysupgrade, but it seems to me like it could in > > fact fail incorrectly if one's system is pointed to a mirror that, for > > whatever reason, is itself outdated. In a macabre sense, this is > > inevitable if one maintains an older copy of OpenBSD that outlives > > OpenBSD itself, as URLs are not permanent. > > And how is it supposed to know your mirror is out of date? Giving an error > there won't help as if your mirror is outdated, it will also tell you that > your system is up to date as of this mirror. > Besides, for some testing purposes, you might need an outdated repo. > > > > > Given all this, I don't understand why it's a "good thing" if sysupgrade > > decides partway through that it doesn't need to do anything after all. > > It did things, it verified your system was up-to-date as you asked. > > > > > I don't personally care what exit code it throws, I only use the tool > > manually, I'd just like to know the rationale if anyone cares to > > elaborate. > > After reading the script, I'd nitpick that it merely advances the version by one step rather than ensures the system is up-to-date, but I suppose one could then argue that somehow advancing beyond the latest version would be incorrect (nevermind impossible), thus leaving "doing nothing" as the only correct action. In any case, thank you for the explanation!
Re: sysupgrade: exit 1 instead of exit 0 when ending early
On 10/11/22 13:10, bug wrote: On Mon, Oct 10, 2022 at 11:17:32AM -0600, Theo de Raadt wrote: It's been explained a few times that being up-to-date is not an error. It's a good thing, and no action is neccessary when up-to-date. Any non-zero value indicates an error, that would include 2. You are marking this as an error, when it isn't. It's been said that being up-to-date is not an error, but if it's been explained, I've failed to find an explanation. It didn't fail, your system is up-to-date as requested. So, it's successful. Usually, when a utility fails to perform its intended task, it gives an error. This includes when the task is not necessary, e.g. using rm to remove a file which doesn't exist, using mkdir to make a directory which already exists, or using gzip to compress a file that wouldn't benefit from compression (unless you tell it to do so anyway, of course). I'm not an expert on sysupgrade, but it seems to me like it could in fact fail incorrectly if one's system is pointed to a mirror that, for whatever reason, is itself outdated. In a macabre sense, this is inevitable if one maintains an older copy of OpenBSD that outlives OpenBSD itself, as URLs are not permanent. And how is it supposed to know your mirror is out of date? Giving an error there won't help as if your mirror is outdated, it will also tell you that your system is up to date as of this mirror. Besides, for some testing purposes, you might need an outdated repo. Given all this, I don't understand why it's a "good thing" if sysupgrade decides partway through that it doesn't need to do anything after all. It did things, it verified your system was up-to-date as you asked. I don't personally care what exit code it throws, I only use the tool manually, I'd just like to know the rationale if anyone cares to elaborate. smime.p7s Description: S/MIME Cryptographic Signature
Re: sysupgrade: exit 1 instead of exit 0 when ending early
> On Mon, Oct 10, 2022 at 11:17:32AM -0600, Theo de Raadt wrote: > > It's been explained a few times that being up-to-date is not an error. > > It's a good thing, and no action is neccessary when up-to-date. > > Any non-zero value indicates an error, that would include 2. You are > > marking this as an error, when it isn't. > > It's been said that being up-to-date is not an error, but if it's been > explained, I've failed to find an explanation. > > Usually, when a utility fails to perform its intended task, it gives an > error. > I don't personally care what exit code it throws, I only use the tool > manually, I'd just like to know the rationale if anyone cares to > elaborate. Simplest explanation is probably: ./some-program && do something on success If you understand this part of shell scripting, then you would see why returning 2 (or 1 or 3 or 102444) will throw off the logic, especially if 1 means some kind of error. As for "intended task", sysupgrade might be viewed as a tool to make sure you have the latest it knows about. If no upgrade is needed or the mirror is old, then that task is done. Someone else might think it is a tool to stress the network and disk by always downloading things from the internet, and that it is a grave error if this can't be done, but those people would have a view that differs from the obsd devs idea of what to use sysupgrade for. Also, if you replace your "mkdir /foo with missing /foo" with "install -d /foo", you'd see it can run twice without throwing errors. It makes sure a dir named foo is created if not exists. That is its task when run with -d. And you can && a command behind it so it catches errors if a file/pipe/socket is there instead. Or add -p to mkdir to get the same behaviour. -- May the most significant bit of your life be positive.
Re: sysupgrade: exit 1 instead of exit 0 when ending early
On Mon, Oct 10, 2022 at 11:17:32AM -0600, Theo de Raadt wrote: > It's been explained a few times that being up-to-date is not an error. > It's a good thing, and no action is neccessary when up-to-date. > > Any non-zero value indicates an error, that would include 2. You are > marking this as an error, when it isn't. It's been said that being up-to-date is not an error, but if it's been explained, I've failed to find an explanation. Usually, when a utility fails to perform its intended task, it gives an error. This includes when the task is not necessary, e.g. using rm to remove a file which doesn't exist, using mkdir to make a directory which already exists, or using gzip to compress a file that wouldn't benefit from compression (unless you tell it to do so anyway, of course). I'm not an expert on sysupgrade, but it seems to me like it could in fact fail incorrectly if one's system is pointed to a mirror that, for whatever reason, is itself outdated. In a macabre sense, this is inevitable if one maintains an older copy of OpenBSD that outlives OpenBSD itself, as URLs are not permanent. Given all this, I don't understand why it's a "good thing" if sysupgrade decides partway through that it doesn't need to do anything after all. I don't personally care what exit code it throws, I only use the tool manually, I'd just like to know the rationale if anyone cares to elaborate.
Re: sysupgrade: exit 1 instead of exit 0 when ending early
On 2022/10/11 03:44, Mikolaj Kucharski wrote: > On Mon, Oct 10, 2022 at 11:17:32AM -0600, Theo de Raadt wrote: > > > Any non-zero value indicates an error, that would include 2. You are > > marking this as an error, when it isn't. > > > > You think this will help your scripting. Do you not realize the proposed > > changes will break someone else's scripting? > > I don't insist on this approach. I just propose a patch. > > I do recognize that it is going to break someone else's workflow. And relying on it also means that your own scripts won't work on older versions, whereas if you just check for /bsd.upgrade then they'll work in both cases (and won't break things for anyone else who uses "set -e" in their sysupgrade wrapper scripts).
