Re: installer: zap fdisk.8.gz and disklabel.8.gz
Klemens Nanni wrote: > On Thu, Aug 25, 2022 at 07:07:27PM +, Miod Vallat wrote: > > > Well, something tells me the inclusion of the manual pages for fdisk > > > and disklabel is deliberate. Makes some sense as these are complex > > > utilities and their interactive use is documented in those pages. > > > > The ability to be able to read the manual pages from the binaries > > themselves, when running is interactive mode, is an intentional feature > > and the reason they embed a gzipped version of the formatted manpage. > > Even in the installer? > > From looking at how distrib works and Makefile.inc unconditionally sets > NOMAN=1, it seems to me that the odd-balls fdisk and disklabel were > overlooked here. > > The manual feature is handy, but I don't think it's worth having in > install media. > > But sure, if others want this in the installer, I'll just leave it. Whoosh.
Re: installer: zap fdisk.8.gz and disklabel.8.gz
Wow you have it so backwards.
So we will have embedded manuals for the case we don't need need the
embedded manual because you have manuals installed (type ^Z and run man)
but in the systems where you don't have manual pages, you won't have
the embedded manuals.
Very logical to forget why this was added...
Klemens Nanni wrote:
> On Thu, Aug 25, 2022 at 06:36:55PM +, Klemens Nanni wrote:
> > Turns out all install media ship full copies of those two manuals due to
> > what can be described like a makefile TOCTOU.
> >
> > In /usr/src/distrib/special, Makefile.inc sets NOMAN=1 but */Makefile
> > only includes it in the very end through .
> >
> > fdisk and disklabel have NOMAN logic before that include, so they don't
> > see it set yet and include the whole thing:
> >
> > $ make -C fdisk manual.o
> > mk -C fdisk manual.o
> > mandoc -Tascii
> > /usr/src/distrib/special/fdisk/../../../sbin/fdisk/fdisk.8 > fdisk.cat8
> > (echo 'const unsigned char manpage[] = {'; cat fdisk.cat8 | gzip -9c |
> > hexdump -ve '"0x" 1/1 "%02x,"'; echo '};'; echo 'const int manpage_sz =
> > sizeof(manpage);') > manual.c
> > cc -O2 -pipe -DHAS_MBR -fno-pie -Oz -fno-stack-protector
> > -fno-unwind-tables -fno-asynchronous-unwind-tables -MD -MP -c manual.c
> > $ size fdisk/obj/manual.o
> > textdatabss dec hex
> > 35520 0 3552de0
> >
> > Forcing it on the command line yields the desired behaviour:
> > $ make -C fdisk NOMAN=1 manual.o
> > (echo 'const unsigned char manpage[] = {'; echo 'no manual' | gzip -9c
> > | hexdump -ve '"0x" 1/1 "%02x,"'; echo '};'; echo 'const int manpage_sz =
> > sizeof(manpage);') > manual.c
> > cc -O2 -pipe -DHAS_MBR -fno-pie -Oz -fno-stack-protector
> > -fno-unwind-tables -fno-asynchronous-unwind-tables -MD -MP -c manual.c
> > $ size fdisk/obj/manual.o
> > textdatabss dec hex
> > 36 0 0 36 24
> >
> > Same for disklabel, size before/after:
> > textdatabss dec hex
> > 61600 0 61601810
> > 36 0 0 36 24
> >
> > I've confirmed this through an amd64 snapshots bsd.rd where I paged
> > through disklabel(8) via the 'M' command...
> >
> > Here's a minimal diff to highlight this non-obvious issue and avoid
> > reshuffling, i.e. cut down on differences with /usr/src/sbin/*/Makefile.
>
> Alternatively, here's a more invasive diff that rips out the compile
> logic around what I understand as purely man-install related NOMAN.
>
> Regular fdisk and disklabel always inlcude the manual and special ones
> just print "no manual." rather than gzipping the string, decompressing
> it at runtime and running a pager through it.
>
> I do like this a bit more as it makes the code simpler and just uses
> SMALL for this, as I'd expect; This saves even more bits.
>
> >
> > Haven't built ramdisks yet to see the final size decrease, but everyone
> > should be happy with this.
> >
> > OK?
>
>
> Index: sbin/fdisk/Makefile
> ===
> RCS file: /cvs/src/sbin/fdisk/Makefile,v
> retrieving revision 1.46
> diff -u -p -r1.46 Makefile
> --- sbin/fdisk/Makefile 23 May 2022 16:58:11 - 1.46
> +++ sbin/fdisk/Makefile 25 Aug 2022 18:47:14 -
> @@ -23,12 +23,6 @@ CLEANFILES += fdisk.cat8 manual.c
>
> .include
>
> -.ifdef NOMAN
> -manual.c:
> - (echo 'const unsigned char manpage[] = {'; \
> - echo 'no manual' | gzip -9c | hexdump -ve '"0x" 1/1 "%02x,"'; \
> - echo '};'; echo 'const int manpage_sz = sizeof(manpage);') > manual.c
> -.else
> fdisk.cat8: fdisk.8
> mandoc -Tascii ${.ALLSRC} > ${.TARGET}
>
> @@ -36,7 +30,6 @@ manual.c: fdisk.cat8
> (echo 'const unsigned char manpage[] = {'; \
> cat fdisk.cat8 | gzip -9c | hexdump -ve '"0x" 1/1 "%02x,"'; \
> echo '};'; echo 'const int manpage_sz = sizeof(manpage);') > manual.c
> -.endif
>
> MAN= fdisk.8
>
> Index: sbin/fdisk/cmd.c
> ===
> RCS file: /cvs/src/sbin/fdisk/cmd.c,v
> retrieving revision 1.164
> diff -u -p -r1.164 cmd.c
> --- sbin/fdisk/cmd.c 25 Jul 2022 17:45:16 - 1.164
> +++ sbin/fdisk/cmd.c 25 Aug 2022 18:51:02 -
> @@ -509,6 +509,9 @@ Xflag(const char *args, struct mbr *mbr)
> int
> Xmanual(const char *args, struct mbr *mbr)
> {
> +#ifdef SMALL
> + printf("no manual.\n");
> +#else
> char*pager = "/usr/bin/less";
> char*p;
> FILE*f;
> @@ -527,6 +530,7 @@ Xmanual(const char *args, struct mbr *mb
> }
>
> signal(SIGPIPE, opipe);
> +#endif /* !SMALL */
>
> return CMD_CONT;
> }
> Index: sbin/disklabel/Makefile
> ===
> RCS file: /cvs/src/sbin/disklabel/Makefile,v
> retrieving
Re: installer: zap fdisk.8.gz and disklabel.8.gz
I think you have this wrong.
If someone is operating in the install media, and manually adjusting
their disk, and they don't know the commands they need, where are they
going to find the instructions?
In 1997, we added the embedded manual pages to fdisk (inside the
'manual' command) and disklabel (the 'M' command) specifically for users
who encounter this problem. The '?' command shows you that you can see
the manual, and then you can use "M" to get a complete manual.
We don't know how much it is used, but it was added *SPECIFICALLY* for
the install media usage case.
Perhaps in the past we had an architecture that didn't fit, and this
NOMAN logic was added?? But it fits today. Why are you assuming it
should be deleted by default in the install media?
Like wow, the install media scenario is the ONE PLACE embedded manuals
are useful! If you aren't on the install media, you don't NEED the
embedded manuals, you can type ^Z man disklabel instead!
Klemens Nanni wrote:
> Turns out all install media ship full copies of those two manuals due to
> what can be described like a makefile TOCTOU.
>
> In /usr/src/distrib/special, Makefile.inc sets NOMAN=1 but */Makefile
> only includes it in the very end through .
>
> fdisk and disklabel have NOMAN logic before that include, so they don't
> see it set yet and include the whole thing:
>
> $ make -C fdisk manual.o
> mk -C fdisk manual.o
> mandoc -Tascii
> /usr/src/distrib/special/fdisk/../../../sbin/fdisk/fdisk.8 > fdisk.cat8
> (echo 'const unsigned char manpage[] = {'; cat fdisk.cat8 | gzip -9c |
> hexdump -ve '"0x" 1/1 "%02x,"'; echo '};'; echo 'const int manpage_sz =
> sizeof(manpage);') > manual.c
> cc -O2 -pipe -DHAS_MBR -fno-pie -Oz -fno-stack-protector
> -fno-unwind-tables -fno-asynchronous-unwind-tables -MD -MP -c manual.c
> $ size fdisk/obj/manual.o
> textdatabss dec hex
> 35520 0 3552de0
>
> Forcing it on the command line yields the desired behaviour:
> $ make -C fdisk NOMAN=1 manual.o
> (echo 'const unsigned char manpage[] = {'; echo 'no manual' | gzip -9c
> | hexdump -ve '"0x" 1/1 "%02x,"'; echo '};'; echo 'const int manpage_sz =
> sizeof(manpage);') > manual.c
> cc -O2 -pipe -DHAS_MBR -fno-pie -Oz -fno-stack-protector
> -fno-unwind-tables -fno-asynchronous-unwind-tables -MD -MP -c manual.c
> $ size fdisk/obj/manual.o
> textdatabss dec hex
> 36 0 0 36 24
>
> Same for disklabel, size before/after:
> textdatabss dec hex
> 61600 0 61601810
> 36 0 0 36 24
>
> I've confirmed this through an amd64 snapshots bsd.rd where I paged
> through disklabel(8) via the 'M' command...
>
> Here's a minimal diff to highlight this non-obvious issue and avoid
> reshuffling, i.e. cut down on differences with /usr/src/sbin/*/Makefile.
>
> Haven't built ramdisks yet to see the final size decrease, but everyone
> should be happy with this.
>
> OK?
>
>
> Index: disklabel/Makefile
> ===
> RCS file: /cvs/src/distrib/special/disklabel/Makefile,v
> retrieving revision 1.13
> diff -u -p -r1.13 Makefile
> --- disklabel/Makefile21 Sep 2021 18:36:09 - 1.13
> +++ disklabel/Makefile25 Aug 2022 18:34:31 -
> @@ -10,6 +10,8 @@ CLEANFILES += disklabel.cat8 manual.c
>
> .include
>
> +# XXX set in ../Makefile.inc already but only pulled in by
> +NOMAN = 1
> .ifdef NOMAN
> manual.c:
> (echo 'const unsigned char manpage[] = {'; \
> Index: fdisk/Makefile
> ===
> RCS file: /cvs/src/distrib/special/fdisk/Makefile,v
> retrieving revision 1.6
> diff -u -p -r1.6 Makefile
> --- fdisk/Makefile23 May 2022 16:58:11 - 1.6
> +++ fdisk/Makefile25 Aug 2022 18:34:15 -
> @@ -23,6 +23,8 @@ CLEANFILES += fdisk.cat8 manual.c
>
> .include
>
> +# XXX set in ../Makefile.inc already but only pulled in by
> +NOMAN = 1
> .ifdef NOMAN
> manual.c:
> (echo 'const unsigned char manpage[] = {'; \
>
Re: installboot: link dynamically
Klemens Nanni wrote: > Dynamic installboot would be nice but I don't have strong opinoins about > it, so best drop the diff and retain the chance to repair your system. These are the static binaries: ./libexec/ld.so/ldconfig obvious why ./sbin/dhcpleased ./sbin/iked ./sbin/isakmpd ./sbin/mountd ./sbin/nfsd ./sbin/pflogd ./sbin/resolvd ./sbin/slaacd ./sbin/unwind I have a diff (in snapshots) which makes some of these dynamic, for shared-libraries-more-secure reasons ./usr.bin/kdump ./usr.bin/ktrace when these are dynamic, ld.so development/debugging becomes very difficult ./usr.sbin/watchdogd to try to remain locked in memory. I'm not sure it works. ./usr.sbin/chroot strange reasons and the boot block related things: ./sys/arch/alpha/stand/installboot ./sys/arch/alpha/stand/setnetbootinfo ./sys/arch/hppa/stand/mkboot ./usr.sbin/installboot
Re: Race in disk_attach_callback?
On Thu, Aug 18, 2022 at 08:29:13AM +, Klemens Nanni wrote:
> On Wed, Aug 17, 2022 at 07:03:50AM +, Miod Vallat wrote:
> > > What is the result if root runs disklabel, and forces it to all zeros?
> >
> > If the root duid is all zeroes, then the only way to refer to the root
> > disk is to use its /dev/{s,w}d* device name, as zero duids are ignored.
>
> I like miod's second diff and it fixes the race for vnd(4).
> I never ran into the issue with softraid(4), but that should not happen
> anymore with it, either.
What's the status on this diff?
The problem still exists and I was reminded by the new installboot
regress suffering from it on an arm64 box.
>
> OK kn
>
> >
> > If you set a zero duid in disklabel(8), setdisklabel() in the kernel
> > will compute a new, non-zero value.
>
> Correct; same for real sd1 and fictitious vnd0.
>
> # disklabel -E sd1
> Label editor (enter '?' for help at any prompt)
>
> sd1> i
> The disklabel UID is currently: c766517084e5e5ce
> duid: []
>
> sd1*> l
> # /dev/rsd1c:
> type: SCSI
> disk: SCSI disk
> label: Block Device
> duid:
> flags:
> bytes/sector: 512
> sectors/track: 63
> tracks/cylinder: 16
> sectors/cylinder: 1008
> cylinders: 203
> total sectors: 204800
> boundstart: 32832
> boundend: 204767
> drivedata: 0
>
> sd1*> w
>
> sd1> l
> # /dev/rsd1c:
> type: SCSI
> disk: SCSI disk
> label: Block Device
> duid: 9ff85059e4960901
> flags:
> bytes/sector: 512
> sectors/track: 63
> tracks/cylinder: 16
> sectors/cylinder: 1008
> cylinders: 203
> total sectors: 204800
> boundstart: 32832
> boundend: 204767
> drivedata: 0
>
> sd1>
>
signify: move (unused) variables under !VERIFYONLY
See through
===> signify
/usr/src/distrib/special/signify/../../../usr.bin/signify/signify.c:754:34:
warning: variable 'seckeyfile' set but not used [-Wunused-but-set-variable]
const char *pubkeyfile = NULL, *seckeyfile = NULL, *msgfile =
NULL,
^
/usr/src/distrib/special/signify/../../../usr.bin/signify/signify.c:757:14:
warning: variable 'comment' set but not used [-Wunused-but-set-variable]
const char *comment = "signify";
^
/usr/src/distrib/special/signify/../../../usr.bin/signify/signify.c:760:6:
warning: variable 'none' set but not used [-Wunused-but-set-variable]
int none = 0;
^
3 warnings generated.
install media obviously does not use any of -cns.
OK?
Index: signify.c
===
RCS file: /cvs/src/usr.bin/signify/signify.c,v
retrieving revision 1.135
diff -u -p -r1.135 signify.c
--- signify.c 21 Jan 2020 12:13:21 - 1.135
+++ signify.c 25 Aug 2022 21:41:56 -
@@ -751,13 +751,14 @@ verifyzdata(uint8_t *zdata, unsigned lon
int
main(int argc, char **argv)
{
- const char *pubkeyfile = NULL, *seckeyfile = NULL, *msgfile = NULL,
- *sigfile = NULL;
+ const char *pubkeyfile = NULL, *msgfile = NULL, *sigfile = NULL;
char sigfilebuf[PATH_MAX];
- const char *comment = "signify";
char *keytype = NULL;
- int ch;
+#ifndef VERIFYONLY
+ const char *seckeyfile = NULL, *comment = "signify";
int none = 0;
+#endif
+ int ch;
int embedded = 0;
int quiet = 0;
int gzip = 0;
@@ -790,6 +791,15 @@ main(int argc, char **argv)
usage(NULL);
verb = SIGN;
break;
+ case 'c':
+ comment = optarg;
+ break;
+ case 'n':
+ none = 1;
+ break;
+ case 's':
+ seckeyfile = optarg;
+ break;
case 'z':
gzip = 1;
break;
@@ -799,26 +809,17 @@ main(int argc, char **argv)
usage(NULL);
verb = VERIFY;
break;
- case 'c':
- comment = optarg;
- break;
case 'e':
embedded = 1;
break;
case 'm':
msgfile = optarg;
break;
- case 'n':
- none = 1;
- break;
case 'p':
pubkeyfile = optarg;
break;
case 'q':
quiet = 1;
- break;
- case 's':
- seckeyfile = optarg;
break;
case 't':
keytype = optarg;
slaacd: move (unused) functions under !SMALL
Seen when building with -Wunused-function
===> slaacd
/usr/src/distrib/special/slaacd/../../../sbin/slaacd/engine.c:313:1:
warning: unused function 'if_state_name' [-Wunused-function]
if_state_name(enum if_state ifs)
^
/usr/src/distrib/special/slaacd/../../../sbin/slaacd/engine.c:323:1:
warning: unused function 'proposal_state_name' [-Wunused-function]
proposal_state_name(enum proposal_state ps)
^
2 warnings generated.
Those are used in log_debug() lines.
No object change but less noise.
OK?
Index: engine.c
===
RCS file: /cvs/src/sbin/slaacd/engine.c,v
retrieving revision 1.83
diff -u -p -r1.83 engine.c
--- engine.c23 Jul 2022 16:16:25 - 1.83
+++ engine.c25 Aug 2022 21:33:21 -
@@ -309,6 +309,7 @@ int64_t proposal_id;
#defineCASE(x) case x : return #x
+#ifndef SMALL
static const char*
if_state_name(enum if_state ifs)
{
@@ -332,6 +333,7 @@ proposal_state_name(enum proposal_state
CASE(PROPOSAL_STALE);
}
}
+#endif
void
engine_sig_handler(int sig, short event, void *arg)
bioctl: sync usage with manual, simplify option list
-l takes chunks as per the manual, not specials. I also think that comma separated lists are marked up overly confusing, so reduce it by one level, i.e. turn -l chunk[,chunk[,...]]] into -l chunk[,...]] Feedback? OK? Index: bioctl.8 === RCS file: /cvs/src/sbin/bioctl/bioctl.8,v retrieving revision 1.109 diff -u -p -r1.109 bioctl.8 --- bioctl.88 Feb 2021 11:20:03 - 1.109 +++ bioctl.825 Aug 2022 21:11:23 - @@ -42,10 +42,10 @@ .Pp .Nm bioctl .Op Fl dhiPqsv -.Op Fl C Ar flag Ns Op Pf , Ar flag Ns Op Pf , Ar ... +.Op Fl C Ar flag Ns Op Pf , Ar ... .Op Fl c Ar raidlevel .Op Fl k Ar keydisk -.Op Fl l Ar chunk Ns Op Pf , Ar chunk Ns Op Pf , Ar ... +.Op Fl l Ar chunk Ns Op Pf , Ar ... .Op Fl O Ar chunk | channel : Ns Ar target Ns Op Pf . Ar lun .Op Fl p Ar passfile .Op Fl R Ar chunk | channel : Ns Ar target Ns Op Pf . Ar lun Index: bioctl.c === RCS file: /cvs/src/sbin/bioctl/bioctl.c,v retrieving revision 1.148 diff -u -p -r1.148 bioctl.c --- bioctl.c19 Aug 2022 17:49:10 - 1.148 +++ bioctl.c25 Aug 2022 21:16:01 - @@ -290,8 +290,8 @@ usage(void) "[-u channel:target[.lun]] " "device\n" " %s [-dhiPqsv] " - "[-C flag[,flag,...]] [-c raidlevel] [-k keydisk]\n" - "\t[-l special[,special,...]] " + "[-C flag[,...]] [-c raidlevel] [-k keydisk]\n" + "\t[-l chunk[,...]] " "[-O device | channel:target[.lun]]\n" "\t[-p passfile] [-R chunk | channel:target[.lun]]\n" "\t[-r rounds] "
Re: installer: zap fdisk.8.gz and disklabel.8.gz
On Thu, Aug 25, 2022 at 07:32:16PM +, Miod Vallat wrote: > > > The ability to be able to read the manual pages from the binaries > > > themselves, when running is interactive mode, is an intentional feature > > > and the reason they embed a gzipped version of the formatted manpage. > > > > Even in the installer? > > Especially in the installer, because you might not have another machine > nearby to access documentation. (and there were no smartphones when this > was introduced close to 25 years ago) > > > The manual feature is handy, but I don't think it's worth having in > > install media. > > This feature is *especially* intended for installation media. > I might even go one step furter: the regular programs do not need the embedded man pages (as man(1) is available), but the installboot versions sure do. -Otto
Re: installer: zap fdisk.8.gz and disklabel.8.gz
> > The ability to be able to read the manual pages from the binaries > > themselves, when running is interactive mode, is an intentional feature > > and the reason they embed a gzipped version of the formatted manpage. > > Even in the installer? Especially in the installer, because you might not have another machine nearby to access documentation. (and there were no smartphones when this was introduced close to 25 years ago) > The manual feature is handy, but I don't think it's worth having in > install media. This feature is *especially* intended for installation media.
Re: installer: zap fdisk.8.gz and disklabel.8.gz
On Thu, Aug 25, 2022 at 07:07:27PM +, Miod Vallat wrote: > > Well, something tells me the inclusion of the manual pages for fdisk > > and disklabel is deliberate. Makes some sense as these are complex > > utilities and their interactive use is documented in those pages. > > The ability to be able to read the manual pages from the binaries > themselves, when running is interactive mode, is an intentional feature > and the reason they embed a gzipped version of the formatted manpage. Even in the installer? >From looking at how distrib works and Makefile.inc unconditionally sets NOMAN=1, it seems to me that the odd-balls fdisk and disklabel were overlooked here. The manual feature is handy, but I don't think it's worth having in install media. But sure, if others want this in the installer, I'll just leave it.
Re: installer: zap fdisk.8.gz and disklabel.8.gz
> Well, something tells me the inclusion of the manual pages for fdisk > and disklabel is deliberate. Makes some sense as these are complex > utilities and their interactive use is documented in those pages. The ability to be able to read the manual pages from the binaries themselves, when running is interactive mode, is an intentional feature and the reason they embed a gzipped version of the formatted manpage.
Re: installer: zap fdisk.8.gz and disklabel.8.gz
> Date: Thu, 25 Aug 2022 18:58:39 +
> From: Klemens Nanni
>
> On Thu, Aug 25, 2022 at 06:36:55PM +, Klemens Nanni wrote:
> > Turns out all install media ship full copies of those two manuals due to
> > what can be described like a makefile TOCTOU.
> >
> > In /usr/src/distrib/special, Makefile.inc sets NOMAN=1 but */Makefile
> > only includes it in the very end through .
> >
> > fdisk and disklabel have NOMAN logic before that include, so they don't
> > see it set yet and include the whole thing:
> >
> > $ make -C fdisk manual.o
> > mk -C fdisk manual.o
> > mandoc -Tascii
> > /usr/src/distrib/special/fdisk/../../../sbin/fdisk/fdisk.8 > fdisk.cat8
> > (echo 'const unsigned char manpage[] = {'; cat fdisk.cat8 | gzip -9c |
> > hexdump -ve '"0x" 1/1 "%02x,"'; echo '};'; echo 'const int manpage_sz =
> > sizeof(manpage);') > manual.c
> > cc -O2 -pipe -DHAS_MBR -fno-pie -Oz -fno-stack-protector
> > -fno-unwind-tables -fno-asynchronous-unwind-tables -MD -MP -c manual.c
> > $ size fdisk/obj/manual.o
> > textdatabss dec hex
> > 35520 0 3552de0
> >
> > Forcing it on the command line yields the desired behaviour:
> > $ make -C fdisk NOMAN=1 manual.o
> > (echo 'const unsigned char manpage[] = {'; echo 'no manual' | gzip -9c
> > | hexdump -ve '"0x" 1/1 "%02x,"'; echo '};'; echo 'const int manpage_sz =
> > sizeof(manpage);') > manual.c
> > cc -O2 -pipe -DHAS_MBR -fno-pie -Oz -fno-stack-protector
> > -fno-unwind-tables -fno-asynchronous-unwind-tables -MD -MP -c manual.c
> > $ size fdisk/obj/manual.o
> > textdatabss dec hex
> > 36 0 0 36 24
> >
> > Same for disklabel, size before/after:
> > textdatabss dec hex
> > 61600 0 61601810
> > 36 0 0 36 24
> >
> > I've confirmed this through an amd64 snapshots bsd.rd where I paged
> > through disklabel(8) via the 'M' command...
> >
> > Here's a minimal diff to highlight this non-obvious issue and avoid
> > reshuffling, i.e. cut down on differences with /usr/src/sbin/*/Makefile.
>
> Alternatively, here's a more invasive diff that rips out the compile
> logic around what I understand as purely man-install related NOMAN.
>
> Regular fdisk and disklabel always inlcude the manual and special ones
> just print "no manual." rather than gzipping the string, decompressing
> it at runtime and running a pager through it.
>
> I do like this a bit more as it makes the code simpler and just uses
> SMALL for this, as I'd expect; This saves even more bits.
>
> >
> > Haven't built ramdisks yet to see the final size decrease, but everyone
> > should be happy with this.
> >
> > OK?
Well, something tells me the inclusion of the manual pages for fdisk
and disklabel is deliberate. Makes some sense as these are complex
utilities and their interactive use is documented in those pages.
> Index: sbin/fdisk/Makefile
> ===
> RCS file: /cvs/src/sbin/fdisk/Makefile,v
> retrieving revision 1.46
> diff -u -p -r1.46 Makefile
> --- sbin/fdisk/Makefile 23 May 2022 16:58:11 - 1.46
> +++ sbin/fdisk/Makefile 25 Aug 2022 18:47:14 -
> @@ -23,12 +23,6 @@ CLEANFILES += fdisk.cat8 manual.c
>
> .include
>
> -.ifdef NOMAN
> -manual.c:
> - (echo 'const unsigned char manpage[] = {'; \
> - echo 'no manual' | gzip -9c | hexdump -ve '"0x" 1/1 "%02x,"'; \
> - echo '};'; echo 'const int manpage_sz = sizeof(manpage);') > manual.c
> -.else
> fdisk.cat8: fdisk.8
> mandoc -Tascii ${.ALLSRC} > ${.TARGET}
>
> @@ -36,7 +30,6 @@ manual.c: fdisk.cat8
> (echo 'const unsigned char manpage[] = {'; \
> cat fdisk.cat8 | gzip -9c | hexdump -ve '"0x" 1/1 "%02x,"'; \
> echo '};'; echo 'const int manpage_sz = sizeof(manpage);') > manual.c
> -.endif
>
> MAN= fdisk.8
>
> Index: sbin/fdisk/cmd.c
> ===
> RCS file: /cvs/src/sbin/fdisk/cmd.c,v
> retrieving revision 1.164
> diff -u -p -r1.164 cmd.c
> --- sbin/fdisk/cmd.c 25 Jul 2022 17:45:16 - 1.164
> +++ sbin/fdisk/cmd.c 25 Aug 2022 18:51:02 -
> @@ -509,6 +509,9 @@ Xflag(const char *args, struct mbr *mbr)
> int
> Xmanual(const char *args, struct mbr *mbr)
> {
> +#ifdef SMALL
> + printf("no manual.\n");
> +#else
> char*pager = "/usr/bin/less";
> char*p;
> FILE*f;
> @@ -527,6 +530,7 @@ Xmanual(const char *args, struct mbr *mb
> }
>
> signal(SIGPIPE, opipe);
> +#endif /* !SMALL */
>
> return CMD_CONT;
> }
> Index: sbin/disklabel/Makefile
> ===
> RCS file: /cvs/src/sbin/disklabel/Makefile,v
> retrieving revision 1.70
> diff -u -p -r1.70 Makefile
> --- sbin/disklabel/Makefile 20
Re: installer: zap fdisk.8.gz and disklabel.8.gz
On Thu, Aug 25, 2022 at 06:36:55PM +, Klemens Nanni wrote:
> Turns out all install media ship full copies of those two manuals due to
> what can be described like a makefile TOCTOU.
>
> In /usr/src/distrib/special, Makefile.inc sets NOMAN=1 but */Makefile
> only includes it in the very end through .
>
> fdisk and disklabel have NOMAN logic before that include, so they don't
> see it set yet and include the whole thing:
>
> $ make -C fdisk manual.o
> mk -C fdisk manual.o
> mandoc -Tascii
> /usr/src/distrib/special/fdisk/../../../sbin/fdisk/fdisk.8 > fdisk.cat8
> (echo 'const unsigned char manpage[] = {'; cat fdisk.cat8 | gzip -9c |
> hexdump -ve '"0x" 1/1 "%02x,"'; echo '};'; echo 'const int manpage_sz =
> sizeof(manpage);') > manual.c
> cc -O2 -pipe -DHAS_MBR -fno-pie -Oz -fno-stack-protector
> -fno-unwind-tables -fno-asynchronous-unwind-tables -MD -MP -c manual.c
> $ size fdisk/obj/manual.o
> textdatabss dec hex
> 35520 0 3552de0
>
> Forcing it on the command line yields the desired behaviour:
> $ make -C fdisk NOMAN=1 manual.o
> (echo 'const unsigned char manpage[] = {'; echo 'no manual' | gzip -9c
> | hexdump -ve '"0x" 1/1 "%02x,"'; echo '};'; echo 'const int manpage_sz =
> sizeof(manpage);') > manual.c
> cc -O2 -pipe -DHAS_MBR -fno-pie -Oz -fno-stack-protector
> -fno-unwind-tables -fno-asynchronous-unwind-tables -MD -MP -c manual.c
> $ size fdisk/obj/manual.o
> textdatabss dec hex
> 36 0 0 36 24
>
> Same for disklabel, size before/after:
> textdatabss dec hex
> 61600 0 61601810
> 36 0 0 36 24
>
> I've confirmed this through an amd64 snapshots bsd.rd where I paged
> through disklabel(8) via the 'M' command...
>
> Here's a minimal diff to highlight this non-obvious issue and avoid
> reshuffling, i.e. cut down on differences with /usr/src/sbin/*/Makefile.
Alternatively, here's a more invasive diff that rips out the compile
logic around what I understand as purely man-install related NOMAN.
Regular fdisk and disklabel always inlcude the manual and special ones
just print "no manual." rather than gzipping the string, decompressing
it at runtime and running a pager through it.
I do like this a bit more as it makes the code simpler and just uses
SMALL for this, as I'd expect; This saves even more bits.
>
> Haven't built ramdisks yet to see the final size decrease, but everyone
> should be happy with this.
>
> OK?
Index: sbin/fdisk/Makefile
===
RCS file: /cvs/src/sbin/fdisk/Makefile,v
retrieving revision 1.46
diff -u -p -r1.46 Makefile
--- sbin/fdisk/Makefile 23 May 2022 16:58:11 - 1.46
+++ sbin/fdisk/Makefile 25 Aug 2022 18:47:14 -
@@ -23,12 +23,6 @@ CLEANFILES += fdisk.cat8 manual.c
.include
-.ifdef NOMAN
-manual.c:
- (echo 'const unsigned char manpage[] = {'; \
- echo 'no manual' | gzip -9c | hexdump -ve '"0x" 1/1 "%02x,"'; \
- echo '};'; echo 'const int manpage_sz = sizeof(manpage);') > manual.c
-.else
fdisk.cat8:fdisk.8
mandoc -Tascii ${.ALLSRC} > ${.TARGET}
@@ -36,7 +30,6 @@ manual.c: fdisk.cat8
(echo 'const unsigned char manpage[] = {'; \
cat fdisk.cat8 | gzip -9c | hexdump -ve '"0x" 1/1 "%02x,"'; \
echo '};'; echo 'const int manpage_sz = sizeof(manpage);') > manual.c
-.endif
MAN= fdisk.8
Index: sbin/fdisk/cmd.c
===
RCS file: /cvs/src/sbin/fdisk/cmd.c,v
retrieving revision 1.164
diff -u -p -r1.164 cmd.c
--- sbin/fdisk/cmd.c25 Jul 2022 17:45:16 - 1.164
+++ sbin/fdisk/cmd.c25 Aug 2022 18:51:02 -
@@ -509,6 +509,9 @@ Xflag(const char *args, struct mbr *mbr)
int
Xmanual(const char *args, struct mbr *mbr)
{
+#ifdef SMALL
+ printf("no manual.\n");
+#else
char*pager = "/usr/bin/less";
char*p;
FILE*f;
@@ -527,6 +530,7 @@ Xmanual(const char *args, struct mbr *mb
}
signal(SIGPIPE, opipe);
+#endif /* !SMALL */
return CMD_CONT;
}
Index: sbin/disklabel/Makefile
===
RCS file: /cvs/src/sbin/disklabel/Makefile,v
retrieving revision 1.70
diff -u -p -r1.70 Makefile
--- sbin/disklabel/Makefile 20 Sep 2021 20:23:44 - 1.70
+++ sbin/disklabel/Makefile 25 Aug 2022 18:46:48 -
@@ -10,12 +10,6 @@ CLEANFILES += disklabel.cat8 manual.c
.include
-.ifdef NOMAN
-manual.c:
- (echo 'const unsigned char manpage[] = {'; \
- echo 'no manual' | gzip -9c | hexdump -ve '"0x" 1/1 "%02x,"'; \
- echo '};'; echo 'const int manpage_sz = sizeof(manpage);') > manual.c
-.else
disklabel.cat8:disklabel.8
mandoc
Re: move PRU_RCVD request to (*pru_rcvd)()
On Tue, Aug 23, 2022 at 11:47:30AM +0300, Vitaliy Makkoveev wrote:
> On Mon, Aug 22, 2022 at 11:08:07PM -0900, Philip Guenther wrote:
> > Since pru_rcvd() is only invoked if the protocol has the PR_WANTRCVD flag
> > set, there should be no need to test whether the callback is set: a
> > protocol without the callback MUST NOT have PR_WANTRCVD.
This may make sense. But we should have a look at all functions
to see if they should do the NULL check. The old behavior was to
return EOPNOTSUPP.
> > (I guess this could, alternatively, go the other direction and eliminate
> > PR_WANTRCVD and use the presence of the callback to decide whether the
> > protocol needs anything to be done.)
>
> I don't want this. At least the per buffer locking could require relock
> in the PRU_RCVD path, and I don't to do it within handler or pru_rcvd()
> wrapper.
>
> >
> > Side note: pru_rcvd() (and the pru_rcvd implementations) should have a
> > return type of void.
> >
Maybe. But we should look at all funtions to make a decision what
return type we want.
> Since the TCP socket could exist without PCB, and we want to return
> error in this case, all callbacks should return int. In other hand, we
> always do `so_pcb' NULL check before call pru_rcvd() and we don't
> interesting in the pru_rcvd() return value.
>
> Also PRU_RCVD is not the only request where return value type could
> be changed to void, at least PRU_SHUTDOWN handlers have no error path,
> except the same case for TCP sockets.
>
> Anyway, as I said in the one of preceding split diff for unix sockets
> case, this time I want only split existing (*pru_usrreq)() handlers, and
> leave possible refactoring to the future. The split diffs are mostly
> mechanical, but huge enough, and I don't want to make them harder.
>
> So I want to commit this as is.
Get this in as it is. First finish the conversion, then consider
guenther@'s comments. After everything has been converted, we will
have a better view what is needed.
bluhm
> > On Mon, Aug 22, 2022 at 1:40 PM Vitaliy Makkoveev wrote:
> >
> > > Another one.
> > >
> > > Since we never use `flags' arg within handlers, remove it from the
> > > pru_rcvd() args.
> > >
> > > Index: sys/sys/protosw.h
> > > ===
> > > RCS file: /cvs/src/sys/sys/protosw.h,v
> > > retrieving revision 1.43
> > > diff -u -p -r1.43 protosw.h
> > > --- sys/sys/protosw.h 22 Aug 2022 21:18:48 - 1.43
> > > +++ sys/sys/protosw.h 22 Aug 2022 22:27:08 -
> > > @@ -72,6 +72,7 @@ struct pr_usrreqs {
> > > int (*pru_accept)(struct socket *, struct mbuf *);
> > > int (*pru_disconnect)(struct socket *);
> > > int (*pru_shutdown)(struct socket *);
> > > + int (*pru_rcvd)(struct socket *);
> > > };
> > >
> > > struct protosw {
> > > @@ -314,10 +315,11 @@ pru_shutdown(struct socket *so)
> > > }
> > >
> > > static inline int
> > > -pru_rcvd(struct socket *so, int flags)
> > > +pru_rcvd(struct socket *so)
> > > {
> > > - return (*so->so_proto->pr_usrreqs->pru_usrreq)(so,
> > > - PRU_RCVD, NULL, (struct mbuf *)(long)flags, NULL, curproc);
> > > + if (so->so_proto->pr_usrreqs->pru_rcvd)
> > > + return (*so->so_proto->pr_usrreqs->pru_rcvd)(so);
> > > + return (EOPNOTSUPP);
> > > }
> > >
> > > static inline int
> > > Index: sys/kern/uipc_socket.c
> > > ===
> > > RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> > > retrieving revision 1.284
> > > diff -u -p -r1.284 uipc_socket.c
> > > --- sys/kern/uipc_socket.c 21 Aug 2022 16:22:17 - 1.284
> > > +++ sys/kern/uipc_socket.c 22 Aug 2022 22:27:08 -
> > > @@ -1156,7 +1156,7 @@ dontblock:
> > > SBLASTRECORDCHK(>so_rcv, "soreceive 4");
> > > SBLASTMBUFCHK(>so_rcv, "soreceive 4");
> > > if (pr->pr_flags & PR_WANTRCVD && so->so_pcb)
> > > - pru_rcvd(so, flags);
> > > + pru_rcvd(so);
> > > }
> > > if (orig_resid == uio->uio_resid && orig_resid &&
> > > (flags & MSG_EOR) == 0 && (so->so_state & SS_CANTRCVMORE) ==
> > > 0) {
> > > @@ -1521,7 +1521,7 @@ somove(struct socket *so, int wait)
> > > if (m == NULL) {
> > > sbdroprecord(so, >so_rcv);
> > > if (so->so_proto->pr_flags & PR_WANTRCVD && so->so_pcb)
> > > - pru_rcvd(so, 0);
> > > + pru_rcvd(so);
> > > goto nextpkt;
> > > }
> > >
> > > @@ -1627,7 +1627,7 @@ somove(struct socket *so, int wait)
> > >
> > > /* Send window update to source peer as receive buffer has
> > > changed. */
> > > if (so->so_proto->pr_flags & PR_WANTRCVD && so->so_pcb)
> > > - pru_rcvd(so, 0);
> > > + pru_rcvd(so);
> > >
> > > /* Receive buffer did shrink by len bytes,
installer: zap fdisk.8.gz and disklabel.8.gz
Turns out all install media ship full copies of those two manuals due to
what can be described like a makefile TOCTOU.
In /usr/src/distrib/special, Makefile.inc sets NOMAN=1 but */Makefile
only includes it in the very end through .
fdisk and disklabel have NOMAN logic before that include, so they don't
see it set yet and include the whole thing:
$ make -C fdisk manual.o
mk -C fdisk manual.o
mandoc -Tascii
/usr/src/distrib/special/fdisk/../../../sbin/fdisk/fdisk.8 > fdisk.cat8
(echo 'const unsigned char manpage[] = {'; cat fdisk.cat8 | gzip -9c |
hexdump -ve '"0x" 1/1 "%02x,"'; echo '};'; echo 'const int manpage_sz =
sizeof(manpage);') > manual.c
cc -O2 -pipe -DHAS_MBR -fno-pie -Oz -fno-stack-protector
-fno-unwind-tables -fno-asynchronous-unwind-tables -MD -MP -c manual.c
$ size fdisk/obj/manual.o
textdatabss dec hex
35520 0 3552de0
Forcing it on the command line yields the desired behaviour:
$ make -C fdisk NOMAN=1 manual.o
(echo 'const unsigned char manpage[] = {'; echo 'no manual' | gzip -9c
| hexdump -ve '"0x" 1/1 "%02x,"'; echo '};'; echo 'const int manpage_sz =
sizeof(manpage);') > manual.c
cc -O2 -pipe -DHAS_MBR -fno-pie -Oz -fno-stack-protector
-fno-unwind-tables -fno-asynchronous-unwind-tables -MD -MP -c manual.c
$ size fdisk/obj/manual.o
textdatabss dec hex
36 0 0 36 24
Same for disklabel, size before/after:
textdatabss dec hex
61600 0 61601810
36 0 0 36 24
I've confirmed this through an amd64 snapshots bsd.rd where I paged
through disklabel(8) via the 'M' command...
Here's a minimal diff to highlight this non-obvious issue and avoid
reshuffling, i.e. cut down on differences with /usr/src/sbin/*/Makefile.
Haven't built ramdisks yet to see the final size decrease, but everyone
should be happy with this.
OK?
Index: disklabel/Makefile
===
RCS file: /cvs/src/distrib/special/disklabel/Makefile,v
retrieving revision 1.13
diff -u -p -r1.13 Makefile
--- disklabel/Makefile 21 Sep 2021 18:36:09 - 1.13
+++ disklabel/Makefile 25 Aug 2022 18:34:31 -
@@ -10,6 +10,8 @@ CLEANFILES += disklabel.cat8 manual.c
.include
+# XXX set in ../Makefile.inc already but only pulled in by
+NOMAN = 1
.ifdef NOMAN
manual.c:
(echo 'const unsigned char manpage[] = {'; \
Index: fdisk/Makefile
===
RCS file: /cvs/src/distrib/special/fdisk/Makefile,v
retrieving revision 1.6
diff -u -p -r1.6 Makefile
--- fdisk/Makefile 23 May 2022 16:58:11 - 1.6
+++ fdisk/Makefile 25 Aug 2022 18:34:15 -
@@ -23,6 +23,8 @@ CLEANFILES += fdisk.cat8 manual.c
.include
+# XXX set in ../Makefile.inc already but only pulled in by
+NOMAN = 1
.ifdef NOMAN
manual.c:
(echo 'const unsigned char manpage[] = {'; \
acpihpet(4): use bus_space_{read,write}_8() where available
The HPET is a 64-bit counter. The spec permits both 32-bit and 64-bit
aligned access. We should use bus_space_read_8() in acpihpet_r()
where it is available to improve the accuracy of acpihpet_delay().
The math is obvious: one read is faster than two.
Switching acpihpet_w() to bus_space_read_write_8() is not strictly
necessary, but it does shrink the object file a bit and also keeps the
two functions symmetrical.
-current:
-rw-r--r-- 1 ssc wobj 53512 Aug 25 13:28 obj/acpihpet.o
patched:
-rw-r--r-- 1 ssc wobj 50040 Aug 25 13:29 obj/acpihpet.o
So we shave 3472 bytes off the module on amd64.
As suggested by jsg@ in the big ACPI delay thread, I am using __LP64__
to decide between 4-byte and 8-byte bus access.
ok?
Index: acpihpet.c
===
RCS file: /cvs/src/sys/dev/acpi/acpihpet.c,v
retrieving revision 1.28
diff -u -p -r1.28 acpihpet.c
--- acpihpet.c 25 Aug 2022 18:01:54 - 1.28
+++ acpihpet.c 25 Aug 2022 18:34:11 -
@@ -86,20 +86,28 @@ struct cfdriver acpihpet_cd = {
uint64_t
acpihpet_r(bus_space_tag_t iot, bus_space_handle_t ioh, bus_size_t ioa)
{
+#if defined(__LP64__)
+ return bus_space_read_8(iot, ioh, ioa);
+#else
uint64_t val;
val = bus_space_read_4(iot, ioh, ioa + 4);
val = val << 32;
val |= bus_space_read_4(iot, ioh, ioa);
return (val);
+#endif
}
void
acpihpet_w(bus_space_tag_t iot, bus_space_handle_t ioh, bus_size_t ioa,
uint64_t val)
{
+#if defined(__LP64__)
+ bus_space_write_8(iot, ioh, ioa, val);
+#else
bus_space_write_4(iot, ioh, ioa + 4, val >> 32);
bus_space_write_4(iot, ioh, ioa, val & 0x);
+#endif
}
int
Re: installboot: link dynamically
On Thu, Aug 25, 2022 at 12:16:56PM -0600, Theo de Raadt wrote: > This binary being static has nothing to do with "installer testing". > You've got that completely wrong. It has nothing to do with reacharounds > either, since instbin takes care of all that. Thanks. > > It has to do with people who may want to use it when their systems are > broken in some way, to repair their systems. Yes they need to mount /usr, > but their system may be too broken for dynamic linking to work, and there > may be multiple steps to repair it, including the ld.so cache etc etc This is also an argument for other programs we may need to fix a system but those are not linked statically. > > If you make this dynamic, I think someone will suffer. Dynamic installboot would be nice but I don't have strong opinoins about it, so best drop the diff and retain the chance to repair your system.
Re: installboot: link dynamically
This binary being static has nothing to do with "installer testing".
You've got that completely wrong. It has nothing to do with reacharounds
either, since instbin takes care of all that.
It has to do with people who may want to use it when their systems are
broken in some way, to repair their systems. Yes they need to mount /usr,
but their system may be too broken for dynamic linking to work, and there
may be multiple steps to repair it, including the ld.so cache etc etc
If you make this dynamic, I think someone will suffer.
Klemens Nanni wrote:
> Spotted through failed attempts to test libutil/opendev(3) changes
> with LD_LIBRARY_PATH.
That test procedure makes no sense, first because it is a static binary,
but secondly because that test procedure will always encounter other problems
so that is just not now one does actual testing.
> Regular installboot(8) has been linking statically since import in
> 2013, which probably stems from when early reach arounds of the
> installer to the installation's installboot binary.
[...]
> Index: Makefile
> ===
> RCS file: /cvs/src/usr.sbin/installboot/Makefile,v
> retrieving revision 1.25
> diff -u -p -r1.25 Makefile
> --- Makefile 15 Aug 2022 17:06:43 - 1.25
> +++ Makefile 25 Aug 2022 17:29:23 -
> @@ -8,8 +8,6 @@ CPPFLAGS= -I${.CURDIR}
> LDADD= -lutil
> DPADD= ${LIBUTIL}
>
> -LDSTATIC=${STATIC}
> -
> .if ${MACHINE} == "amd64" || ${MACHINE} == "i386"
> CFLAGS += -DSOFTRAID
> SRCS += i386_installboot.c
>
installboot: link dynamically
Spotted through failed attempts to test libutil/opendev(3) changes
with LD_LIBRARY_PATH.
Regular installboot(8) has been linking statically since import in
2013, which probably stems from when early reach arounds of the
installer to the installation's installboot binary.
These days all architectures except two use their own static
installboot build in install media as usual:
$ grep -L installboot /usr/src/distrib/*/*/list
/usr/src/distrib/alpha/miniroot/list
/usr/src/distrib/luna88k/ramdisk/list
alpha still uses its own sys/arch/alpha/stand/installboot as per
distrib/alpha/miniroot/install.md:
md_installboot() {
# Use cat to avoid holes created by cp(1)
cat /mnt/usr/mdec/boot > /mnt/boot
/mnt/usr/mdec/installboot /mnt/boot /mnt/usr/mdec/bootxx
/dev/r${1}c
}
luna88k does not use installboot at all in
distrib/alpha/miniroot/install.md:
md_installboot() {
cat /mnt/usr/mdec/boot > /mnt/boot
}
So none of our currently supported install media reach around;
all use their own static copy from distrib/special/installboot,
thus we can link regular installboot dynamically.
Thanks to moid for some alpha hints.
Feedback? Objection? OK?
Index: Makefile
===
RCS file: /cvs/src/usr.sbin/installboot/Makefile,v
retrieving revision 1.25
diff -u -p -r1.25 Makefile
--- Makefile15 Aug 2022 17:06:43 - 1.25
+++ Makefile25 Aug 2022 17:29:23 -
@@ -8,8 +8,6 @@ CPPFLAGS= -I${.CURDIR}
LDADD= -lutil
DPADD= ${LIBUTIL}
-LDSTATIC= ${STATIC}
-
.if ${MACHINE} == "amd64" || ${MACHINE} == "i386"
CFLAGS += -DSOFTRAID
SRCS += i386_installboot.c
Re: rpki-client: print info about encapsulated certs & PEM format in filemode
On Thu, Aug 25, 2022 at 05:06:33PM +, Job Snijders wrote:
> On Thu, Aug 25, 2022 at 06:38:36PM +0200, Theo Buehler wrote:
> > On Thu, Aug 25, 2022 at 04:04:27PM +, Job Snijders wrote:
> > > On Thu, Aug 25, 2022 at 03:38:45PM +0200, Claudio Jeker wrote:
> > > > I wonder why is PEM printing not part of -f? It seems to be something
> > > > that should be part of filemode.
> > >
> > > OK, how about this?
> >
> > That's a lot better. However X509_print() generates much visual noise
> > and hides the things I'm primarily interested in. I like the current
> > concise output of -f mode.
> >
> > Can we put the X509_print() behind the -v flag and keep the
> > PEM_write_bio_X509() behind -p?
>
> Sure.
>
> > More comments inline.
>
> Addressed.
Thanks.
>
> OK?
ok.
If you don't mind one last nit: could you put braces around the inner
ifs, please?
if (verbose) {
if (!X509_print_fp(...)
errx(...)
}
if (printpem) {
if (...)
...
}
Re: rpki-client: print info about encapsulated certs & PEM format in filemode
On Thu, Aug 25, 2022 at 06:38:36PM +0200, Theo Buehler wrote:
> On Thu, Aug 25, 2022 at 04:04:27PM +, Job Snijders wrote:
> > On Thu, Aug 25, 2022 at 03:38:45PM +0200, Claudio Jeker wrote:
> > > I wonder why is PEM printing not part of -f? It seems to be something
> > > that should be part of filemode.
> >
> > OK, how about this?
>
> That's a lot better. However X509_print() generates much visual noise
> and hides the things I'm primarily interested in. I like the current
> concise output of -f mode.
>
> Can we put the X509_print() behind the -v flag and keep the
> PEM_write_bio_X509() behind -p?
Sure.
> More comments inline.
Addressed.
OK?
Kind regards,
Job
Index: filemode.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
retrieving revision 1.9
diff -u -p -r1.9 filemode.c
--- filemode.c 25 Aug 2022 11:07:28 - 1.9
+++ filemode.c 25 Aug 2022 17:05:09 -
@@ -34,11 +34,15 @@
#include
#include
#include
+#include
#include
#include
#include "extern.h"
+extern int printpem;
+extern int verbose;
+
static X509_STORE_CTX *ctx;
static struct auth_tree auths = RB_INITIALIZER();
static struct crl_tree crlt = RB_INITIALIZER();
@@ -420,10 +424,25 @@ proc_parser_file(char *file, unsigned ch
}
if (outformats & FORMAT_JSON)
- printf("\"\n}");
- else
+ printf("\"\n}\n");
+ else {
printf("\n");
+ if (x509 == NULL)
+ goto out;
+ if (type == RTYPE_TAL || type == RTYPE_CRL)
+ goto out;
+
+ if (verbose)
+ if (!X509_print_fp(stdout, x509))
+ errx(1, "X509_print_fp");
+
+ if (printpem)
+ if (!PEM_write_X509(stdout, x509))
+ errx(1, "PEM_write_X509");
+ }
+
+ out:
X509_free(x509);
cert_free(cert);
crl_free(crl);
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.209
diff -u -p -r1.209 main.c
--- main.c 4 Aug 2022 13:44:07 - 1.209
+++ main.c 25 Aug 2022 17:05:09 -
@@ -64,6 +64,7 @@ const char*bird_tablename = "ROAS";
intverbose;
intnoop;
intfilemode;
+intprintpem;
intrrdpon = 1;
intrepo_timeout;
@@ -819,7 +820,7 @@ main(int argc, char *argv[])
"proc exec unveil", NULL) == -1)
err(1, "pledge");
- while ((c = getopt(argc, argv, "b:Bcd:e:fjnorRs:S:t:T:vV")) != -1)
+ while ((c = getopt(argc, argv, "b:Bcd:e:fjnoprRs:S:t:T:vV")) != -1)
switch (c) {
case 'b':
bind_addr = optarg;
@@ -849,6 +850,9 @@ main(int argc, char *argv[])
case 'o':
outformats |= FORMAT_OPENBGPD;
break;
+ case 'p':
+ printpem = 1;
+ break;
case 'R':
rrdpon = 0;
break;
@@ -1278,6 +1282,7 @@ usage:
" [-e rsync_prog]\n"
" [-S skiplist] [-s timeout] [-T table] [-t tal]"
" [outputdir]\n"
- " rpki-client [-Vv] [-d cachedir] [-t tal] -f file ...\n");
+ " rpki-client [-Vv] [-d cachedir] [-j | -p] [-t tal] -f file"
+ " ...\n");
return 1;
}
Index: rpki-client.8
===
RCS file: /cvs/src/usr.sbin/rpki-client/rpki-client.8,v
retrieving revision 1.68
diff -u -p -r1.68 rpki-client.8
--- rpki-client.8 30 Jun 2022 10:27:52 - 1.68
+++ rpki-client.8 25 Aug 2022 17:05:09 -
@@ -34,6 +34,7 @@
.Nm
.Op Fl Vv
.Op Fl d Ar cachedir
+.Op Fl j | p
.Op Fl t Ar tal
.Fl f
.Ar
@@ -144,6 +145,8 @@ If the
and
.Fl j
options are not specified this is the default.
+.It Fl p
+Print the encapsulated X.509 certificate in PEM format.
.It Fl R
Synchronize via RSYNC only.
.It Fl r
Re: rpki-client: print info about encapsulated certs & PEM format in filemode
On Thu, Aug 25, 2022 at 04:04:27PM +, Job Snijders wrote:
> On Thu, Aug 25, 2022 at 03:38:45PM +0200, Claudio Jeker wrote:
> > I wonder why is PEM printing not part of -f? It seems to be something
> > that should be part of filemode.
>
> OK, how about this?
That's a lot better. However X509_print() generates much visual noise
and hides the things I'm primarily interested in. I like the current
concise output of -f mode.
Can we put the X509_print() behind the -v flag and keep the
PEM_write_bio_X509() behind -p?
More comments inline.
>
> Kind regards,
>
> Job
>
> Index: filemode.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 filemode.c
> --- filemode.c25 Aug 2022 11:07:28 - 1.9
> +++ filemode.c25 Aug 2022 16:01:16 -
> @@ -32,13 +32,17 @@
> #include
>
> #include
> +#include
> #include
> #include
> +#include
> #include
> #include
>
> #include "extern.h"
>
> +extern intprintpem;
> +
> static X509_STORE_CTX*ctx;
> static struct auth_tree auths = RB_INITIALIZER();
> static struct crl_treecrlt = RB_INITIALIZER();
> @@ -258,6 +262,7 @@ proc_parser_file(char *file, unsigned ch
> {
> static int num;
> X509 *x509 = NULL;
> + BIO *bio_out = NULL;
> struct cert *cert = NULL;
> struct crl *crl = NULL;
> struct mft *mft = NULL;
> @@ -421,9 +426,24 @@ proc_parser_file(char *file, unsigned ch
>
> if (outformats & FORMAT_JSON)
> printf("\"\n}");
Unrelated to your diff, but I think this lacks an \n after }.
If you agree, ok tb for that change alone.
> - else
> + else {
> printf("\n");
>
> + if (type == RTYPE_TAL || type == RTYPE_CRL)
> + goto out;
x509 can be NULL here (e.g., when mft_parse() fails), so this needs an
if (x509 == NULL)
goto out;
to avoid a crash.
> +
> + if ((bio_out = BIO_new_fp(stdout, BIO_NOCLOSE)) == NULL)
> + errx(1, "BIO_new_fp");
> +
> + if (!X509_print(bio_out, x509))
> + errx(1, "X509_print");
There is actually no need for a BIO:
if (!X509_print_fp(stdout, x509))
errx(1, "X509_print_fp");
> +
> + if (printpem)
> + if (!PEM_write_bio_X509(bio_out, x509))
Similarly here:
if (!PEM_write_X509(stdout, x509))
> + errx(1, "PEM_write_bio_X509");
> + }
> +
> + out:
> X509_free(x509);
> cert_free(cert);
> crl_free(crl);
> @@ -432,6 +452,7 @@ proc_parser_file(char *file, unsigned ch
> gbr_free(gbr);
> tal_free(tal);
> rsc_free(rsc);
> + BIO_free(bio_out);
> }
>
> /*
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.209
> diff -u -p -r1.209 main.c
> --- main.c4 Aug 2022 13:44:07 - 1.209
> +++ main.c25 Aug 2022 16:01:16 -
> @@ -64,6 +64,7 @@ const char *bird_tablename = "ROAS";
> int verbose;
> int noop;
> int filemode;
> +int printpem;
> int rrdpon = 1;
> int repo_timeout;
>
> @@ -819,7 +820,7 @@ main(int argc, char *argv[])
> "proc exec unveil", NULL) == -1)
> err(1, "pledge");
>
> - while ((c = getopt(argc, argv, "b:Bcd:e:fjnorRs:S:t:T:vV")) != -1)
> + while ((c = getopt(argc, argv, "b:Bcd:e:fjnoprRs:S:t:T:vV")) != -1)
> switch (c) {
> case 'b':
> bind_addr = optarg;
> @@ -849,6 +850,9 @@ main(int argc, char *argv[])
> case 'o':
> outformats |= FORMAT_OPENBGPD;
> break;
> + case 'p':
> + printpem = 1;
> + break;
> case 'R':
> rrdpon = 0;
> break;
> @@ -888,6 +892,9 @@ main(int argc, char *argv[])
> argv += optind;
> argc -= optind;
>
> + if ((!filemode && printpem) || (printpem && outformats))
> + goto usage;
Let's take care of this in a separate diff. I think we need to do more
than just that.
> +
> if (!filemode) {
> if (argc == 1)
> outputdir = argv[0];
> @@ -1278,6 +1285,7 @@ usage:
> " [-e rsync_prog]\n"
> " [-S skiplist] [-s timeout] [-T table] [-t tal]"
> " [outputdir]\n"
> - " rpki-client [-Vv] [-d cachedir] [-t tal] -f file ...\n");
> + " rpki-client [-Vv] [-d cachedir] [-j | -p] [-t tal] -f file"
> + " ...\n");
> return 1;
> }
> Index: rpki-client.8
> ===
> RCS file:
Re: rpki-client: print info about encapsulated certs & PEM format in filemode
On Thu, Aug 25, 2022 at 03:38:45PM +0200, Claudio Jeker wrote:
> I wonder why is PEM printing not part of -f? It seems to be something
> that should be part of filemode.
OK, how about this?
Kind regards,
Job
Index: filemode.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
retrieving revision 1.9
diff -u -p -r1.9 filemode.c
--- filemode.c 25 Aug 2022 11:07:28 - 1.9
+++ filemode.c 25 Aug 2022 16:01:16 -
@@ -32,13 +32,17 @@
#include
#include
+#include
#include
#include
+#include
#include
#include
#include "extern.h"
+extern int printpem;
+
static X509_STORE_CTX *ctx;
static struct auth_tree auths = RB_INITIALIZER();
static struct crl_tree crlt = RB_INITIALIZER();
@@ -258,6 +262,7 @@ proc_parser_file(char *file, unsigned ch
{
static int num;
X509 *x509 = NULL;
+ BIO *bio_out = NULL;
struct cert *cert = NULL;
struct crl *crl = NULL;
struct mft *mft = NULL;
@@ -421,9 +426,24 @@ proc_parser_file(char *file, unsigned ch
if (outformats & FORMAT_JSON)
printf("\"\n}");
- else
+ else {
printf("\n");
+ if (type == RTYPE_TAL || type == RTYPE_CRL)
+ goto out;
+
+ if ((bio_out = BIO_new_fp(stdout, BIO_NOCLOSE)) == NULL)
+ errx(1, "BIO_new_fp");
+
+ if (!X509_print(bio_out, x509))
+ errx(1, "X509_print");
+
+ if (printpem)
+ if (!PEM_write_bio_X509(bio_out, x509))
+ errx(1, "PEM_write_bio_X509");
+ }
+
+ out:
X509_free(x509);
cert_free(cert);
crl_free(crl);
@@ -432,6 +452,7 @@ proc_parser_file(char *file, unsigned ch
gbr_free(gbr);
tal_free(tal);
rsc_free(rsc);
+ BIO_free(bio_out);
}
/*
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.209
diff -u -p -r1.209 main.c
--- main.c 4 Aug 2022 13:44:07 - 1.209
+++ main.c 25 Aug 2022 16:01:16 -
@@ -64,6 +64,7 @@ const char*bird_tablename = "ROAS";
intverbose;
intnoop;
intfilemode;
+intprintpem;
intrrdpon = 1;
intrepo_timeout;
@@ -819,7 +820,7 @@ main(int argc, char *argv[])
"proc exec unveil", NULL) == -1)
err(1, "pledge");
- while ((c = getopt(argc, argv, "b:Bcd:e:fjnorRs:S:t:T:vV")) != -1)
+ while ((c = getopt(argc, argv, "b:Bcd:e:fjnoprRs:S:t:T:vV")) != -1)
switch (c) {
case 'b':
bind_addr = optarg;
@@ -849,6 +850,9 @@ main(int argc, char *argv[])
case 'o':
outformats |= FORMAT_OPENBGPD;
break;
+ case 'p':
+ printpem = 1;
+ break;
case 'R':
rrdpon = 0;
break;
@@ -888,6 +892,9 @@ main(int argc, char *argv[])
argv += optind;
argc -= optind;
+ if ((!filemode && printpem) || (printpem && outformats))
+ goto usage;
+
if (!filemode) {
if (argc == 1)
outputdir = argv[0];
@@ -1278,6 +1285,7 @@ usage:
" [-e rsync_prog]\n"
" [-S skiplist] [-s timeout] [-T table] [-t tal]"
" [outputdir]\n"
- " rpki-client [-Vv] [-d cachedir] [-t tal] -f file ...\n");
+ " rpki-client [-Vv] [-d cachedir] [-j | -p] [-t tal] -f file"
+ " ...\n");
return 1;
}
Index: rpki-client.8
===
RCS file: /cvs/src/usr.sbin/rpki-client/rpki-client.8,v
retrieving revision 1.68
diff -u -p -r1.68 rpki-client.8
--- rpki-client.8 30 Jun 2022 10:27:52 - 1.68
+++ rpki-client.8 25 Aug 2022 16:01:16 -
@@ -34,6 +34,7 @@
.Nm
.Op Fl Vv
.Op Fl d Ar cachedir
+.Op Fl j | p
.Op Fl t Ar tal
.Fl f
.Ar
@@ -144,6 +145,8 @@ If the
and
.Fl j
options are not specified this is the default.
+.It Fl p
+Print the encapsulated X.509 certificate in PEM format.
.It Fl R
Synchronize via RSYNC only.
.It Fl r
Re: bgplgd use memset and memcpy instead of bzero and bcopy
On Thu, Aug 25, 2022 at 05:31:55PM +0200, Claudio Jeker wrote:
> The same change was done in bgpd and bgpctl. So here is bgplgd.
> I replaced one bcopy() with memmove() since this is most probably an
> overlapping memory move.
Agreed, that looks like it could be overlapping.
ok
>
> --
> :wq Claudio
>
> Index: qs.c
> ===
> RCS file: /cvs/src/usr.sbin/bgplgd/qs.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 qs.c
> --- qs.c 28 Jun 2022 16:11:30 - 1.1
> +++ qs.c 17 Aug 2022 15:17:27 -
> @@ -148,7 +148,7 @@ valid_prefix(char *str)
> p[0] = '\0';
> }
>
> - bzero(, sizeof(hints));
> + memset(, 0, sizeof(hints));
> hints.ai_family = AF_UNSPEC;
> hints.ai_socktype = SOCK_DGRAM;
> hints.ai_flags = AI_NUMERICHOST;
> Index: slowcgi.c
> ===
> RCS file: /cvs/src/usr.sbin/bgplgd/slowcgi.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 slowcgi.c
> --- slowcgi.c 12 Aug 2022 13:24:30 - 1.3
> +++ slowcgi.c 17 Aug 2022 15:18:50 -
> @@ -387,7 +387,7 @@ slowcgi_listen(char *path, struct passwd
> 0)) == -1)
> lerr(1, "slowcgi_listen: socket");
>
> - bzero(, sizeof(sun));
> + memset(, 0, sizeof(sun));
> sun.sun_family = AF_UNIX;
> if (strlcpy(sun.sun_path, path, sizeof(sun.sun_path)) >=
> sizeof(sun.sun_path))
> @@ -681,7 +681,7 @@ slowcgi_request(int fd, short events, vo
>
> /* Make space for further reads */
> if (c->buf_len > 0) {
> - bcopy(c->buf + c->buf_pos, c->buf, c->buf_len);
> + memmove(c->buf, c->buf + c->buf_pos, c->buf_len);
> c->buf_pos = 0;
> }
> return;
> @@ -789,11 +789,11 @@ parse_params(uint8_t *buf, uint16_t n, s
> return;
> }
>
> - bcopy(buf, env_entry->key, name_len);
> + memcpy(env_entry->key, buf, name_len);
> buf += name_len;
> n -= name_len;
> env_entry->key[name_len] = '\0';
> - bcopy(buf, env_entry->val, val_len);
> + memcpy(env_entry->val, buf, val_len);
> buf += val_len;
> n -= val_len;
> env_entry->val[val_len] = '\0';
>
bgplgd use memset and memcpy instead of bzero and bcopy
The same change was done in bgpd and bgpctl. So here is bgplgd.
I replaced one bcopy() with memmove() since this is most probably an
overlapping memory move.
--
:wq Claudio
Index: qs.c
===
RCS file: /cvs/src/usr.sbin/bgplgd/qs.c,v
retrieving revision 1.1
diff -u -p -r1.1 qs.c
--- qs.c28 Jun 2022 16:11:30 - 1.1
+++ qs.c17 Aug 2022 15:17:27 -
@@ -148,7 +148,7 @@ valid_prefix(char *str)
p[0] = '\0';
}
- bzero(, sizeof(hints));
+ memset(, 0, sizeof(hints));
hints.ai_family = AF_UNSPEC;
hints.ai_socktype = SOCK_DGRAM;
hints.ai_flags = AI_NUMERICHOST;
Index: slowcgi.c
===
RCS file: /cvs/src/usr.sbin/bgplgd/slowcgi.c,v
retrieving revision 1.3
diff -u -p -r1.3 slowcgi.c
--- slowcgi.c 12 Aug 2022 13:24:30 - 1.3
+++ slowcgi.c 17 Aug 2022 15:18:50 -
@@ -387,7 +387,7 @@ slowcgi_listen(char *path, struct passwd
0)) == -1)
lerr(1, "slowcgi_listen: socket");
- bzero(, sizeof(sun));
+ memset(, 0, sizeof(sun));
sun.sun_family = AF_UNIX;
if (strlcpy(sun.sun_path, path, sizeof(sun.sun_path)) >=
sizeof(sun.sun_path))
@@ -681,7 +681,7 @@ slowcgi_request(int fd, short events, vo
/* Make space for further reads */
if (c->buf_len > 0) {
- bcopy(c->buf + c->buf_pos, c->buf, c->buf_len);
+ memmove(c->buf, c->buf + c->buf_pos, c->buf_len);
c->buf_pos = 0;
}
return;
@@ -789,11 +789,11 @@ parse_params(uint8_t *buf, uint16_t n, s
return;
}
- bcopy(buf, env_entry->key, name_len);
+ memcpy(env_entry->key, buf, name_len);
buf += name_len;
n -= name_len;
env_entry->key[name_len] = '\0';
- bcopy(buf, env_entry->val, val_len);
+ memcpy(env_entry->val, buf, val_len);
buf += val_len;
n -= val_len;
env_entry->val[val_len] = '\0';
Re: rpki-client: add mode to print encapsulated certs/crls in human-readable & PEM format
On Thu, Aug 25, 2022 at 01:25:24PM +, Job Snijders wrote:
> Hi all,
>
> Thanks for taking the time to review & suggest improvements. I amended
> the changeset based on your feedback.
>
> To summarize the changes:
>
> * to address sloppiness in command line option handling, make pemmode
> mutually exclusive with filemode and specifying outformats
> * rename PEM printing function to pem_print()
> * don't limit printing PEM to just one file, allow multiple arguments
> like filemode also does
> * visually align variables inside pem_print()
> * avoid the ASN1_item_d2i gotcha of der_in being advanced past the
> parsed data
> * free buf unconditionally
>
> OK?
I wonder why is PEM printing not part of -f? It seems to be something that
should be part of filemode.
> Kind regards,
>
> Job
>
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.150
> diff -u -p -r1.150 extern.h
> --- extern.h 19 Aug 2022 12:45:53 - 1.150
> +++ extern.h 25 Aug 2022 13:00:26 -
> @@ -664,6 +664,7 @@ void mft_print(const X509 *, const str
> void roa_print(const X509 *, const struct roa *);
> void gbr_print(const X509 *, const struct gbr *);
> void rsc_print(const X509 *, const struct rsc *);
> +int print_pem(const char *);
>
> /* Output! */
>
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.209
> diff -u -p -r1.209 main.c
> --- main.c4 Aug 2022 13:44:07 - 1.209
> +++ main.c25 Aug 2022 13:00:27 -
> @@ -64,6 +64,7 @@ const char *bird_tablename = "ROAS";
> int verbose;
> int noop;
> int filemode;
> +int pemmode;
> int rrdpon = 1;
> int repo_timeout;
>
> @@ -819,7 +820,7 @@ main(int argc, char *argv[])
> "proc exec unveil", NULL) == -1)
> err(1, "pledge");
>
> - while ((c = getopt(argc, argv, "b:Bcd:e:fjnorRs:S:t:T:vV")) != -1)
> + while ((c = getopt(argc, argv, "b:Bcd:e:fjnoprRs:S:t:T:vV")) != -1)
> switch (c) {
> case 'b':
> bind_addr = optarg;
> @@ -849,6 +850,9 @@ main(int argc, char *argv[])
> case 'o':
> outformats |= FORMAT_OPENBGPD;
> break;
> + case 'p':
> + pemmode = 1;
> + break;
> case 'R':
> rrdpon = 0;
> break;
> @@ -888,6 +892,20 @@ main(int argc, char *argv[])
> argv += optind;
> argc -= optind;
>
> + if (pemmode && (filemode || outformats))
> + goto usage;
> +
> + if (pemmode) {
> + if (pledge("stdio rpath", NULL) == -1)
> + err(1, "pledge");
> +
> + for (; *argv != NULL; argv++) {
> + if ((rc = print_pem(*argv)) != 0)
> + return rc;
> + }
> + return rc;
> + }
> +
> if (!filemode) {
> if (argc == 1)
> outputdir = argv[0];
> @@ -1278,6 +1296,7 @@ usage:
> " [-e rsync_prog]\n"
> " [-S skiplist] [-s timeout] [-T table] [-t tal]"
> " [outputdir]\n"
> - " rpki-client [-Vv] [-d cachedir] [-t tal] -f file ...\n");
> + " rpki-client [-Vv] [-d cachedir] [-t tal] -f file ...\n"
> + " rpki-client -p file ...\n");
> return 1;
> }
> Index: print.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/print.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 print.c
> --- print.c 14 Jul 2022 13:24:56 - 1.14
> +++ print.c 25 Aug 2022 13:00:27 -
> @@ -1,5 +1,6 @@
> /* $OpenBSD: print.c,v 1.14 2022/07/14 13:24:56 job Exp $ */
> /*
> + * Copyright (c) 2022 Job Snijders
> * Copyright (c) 2021 Claudio Jeker
> * Copyright (c) 2019 Kristaps Dzonsons
> *
> @@ -26,6 +27,8 @@
> #include
>
> #include
> +#include
> +#include
>
> #include "extern.h"
>
> @@ -567,4 +570,99 @@ rsc_print(const X509 *x, const struct rs
>
> if (outformats & FORMAT_JSON)
> printf("\t],\n");
> +}
> +
> +/*
> + * Read a file, extract the encapsulated X509 cert and print in PEM format.
> + * Return zero on success, non-zero on failure.
> + */
> +int
> +print_pem(const char *fn)
> +{
> + BIO *bio_out = NULL;
> + X509*x = NULL;
> + X509_CRL*c = NULL;
> + struct gbr *gbr = NULL;
> + struct mft *mft = NULL;
> + struct roa *roa = NULL;
> + struct rsc *rsc = NULL;
> + unsigned char *buf = NULL;
> + const unsigned char *der;
> +
Re: rpki-client: add mode to print encapsulated certs/crls in human-readable & PEM format
Hi all,
Thanks for taking the time to review & suggest improvements. I amended
the changeset based on your feedback.
To summarize the changes:
* to address sloppiness in command line option handling, make pemmode
mutually exclusive with filemode and specifying outformats
* rename PEM printing function to pem_print()
* don't limit printing PEM to just one file, allow multiple arguments
like filemode also does
* visually align variables inside pem_print()
* avoid the ASN1_item_d2i gotcha of der_in being advanced past the
parsed data
* free buf unconditionally
OK?
Kind regards,
Job
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.150
diff -u -p -r1.150 extern.h
--- extern.h19 Aug 2022 12:45:53 - 1.150
+++ extern.h25 Aug 2022 13:00:26 -
@@ -664,6 +664,7 @@ void mft_print(const X509 *, const str
voidroa_print(const X509 *, const struct roa *);
voidgbr_print(const X509 *, const struct gbr *);
voidrsc_print(const X509 *, const struct rsc *);
+int print_pem(const char *);
/* Output! */
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.209
diff -u -p -r1.209 main.c
--- main.c 4 Aug 2022 13:44:07 - 1.209
+++ main.c 25 Aug 2022 13:00:27 -
@@ -64,6 +64,7 @@ const char*bird_tablename = "ROAS";
intverbose;
intnoop;
intfilemode;
+intpemmode;
intrrdpon = 1;
intrepo_timeout;
@@ -819,7 +820,7 @@ main(int argc, char *argv[])
"proc exec unveil", NULL) == -1)
err(1, "pledge");
- while ((c = getopt(argc, argv, "b:Bcd:e:fjnorRs:S:t:T:vV")) != -1)
+ while ((c = getopt(argc, argv, "b:Bcd:e:fjnoprRs:S:t:T:vV")) != -1)
switch (c) {
case 'b':
bind_addr = optarg;
@@ -849,6 +850,9 @@ main(int argc, char *argv[])
case 'o':
outformats |= FORMAT_OPENBGPD;
break;
+ case 'p':
+ pemmode = 1;
+ break;
case 'R':
rrdpon = 0;
break;
@@ -888,6 +892,20 @@ main(int argc, char *argv[])
argv += optind;
argc -= optind;
+ if (pemmode && (filemode || outformats))
+ goto usage;
+
+ if (pemmode) {
+ if (pledge("stdio rpath", NULL) == -1)
+ err(1, "pledge");
+
+ for (; *argv != NULL; argv++) {
+ if ((rc = print_pem(*argv)) != 0)
+ return rc;
+ }
+ return rc;
+ }
+
if (!filemode) {
if (argc == 1)
outputdir = argv[0];
@@ -1278,6 +1296,7 @@ usage:
" [-e rsync_prog]\n"
" [-S skiplist] [-s timeout] [-T table] [-t tal]"
" [outputdir]\n"
- " rpki-client [-Vv] [-d cachedir] [-t tal] -f file ...\n");
+ " rpki-client [-Vv] [-d cachedir] [-t tal] -f file ...\n"
+ " rpki-client -p file ...\n");
return 1;
}
Index: print.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/print.c,v
retrieving revision 1.14
diff -u -p -r1.14 print.c
--- print.c 14 Jul 2022 13:24:56 - 1.14
+++ print.c 25 Aug 2022 13:00:27 -
@@ -1,5 +1,6 @@
/* $OpenBSD: print.c,v 1.14 2022/07/14 13:24:56 job Exp $ */
/*
+ * Copyright (c) 2022 Job Snijders
* Copyright (c) 2021 Claudio Jeker
* Copyright (c) 2019 Kristaps Dzonsons
*
@@ -26,6 +27,8 @@
#include
#include
+#include
+#include
#include "extern.h"
@@ -567,4 +570,99 @@ rsc_print(const X509 *x, const struct rs
if (outformats & FORMAT_JSON)
printf("\t],\n");
+}
+
+/*
+ * Read a file, extract the encapsulated X509 cert and print in PEM format.
+ * Return zero on success, non-zero on failure.
+ */
+int
+print_pem(const char *fn)
+{
+ BIO *bio_out = NULL;
+ X509*x = NULL;
+ X509_CRL*c = NULL;
+ struct gbr *gbr = NULL;
+ struct mft *mft = NULL;
+ struct roa *roa = NULL;
+ struct rsc *rsc = NULL;
+ unsigned char *buf = NULL;
+ const unsigned char *der;
+ size_t len;
+ enum rtype type;
+ int rc = 1;
+
+ x509_init_oid();
+
+ type = rtype_from_file_extension(fn);
+ if (type == RTYPE_INVALID) {
+ warnx("%s: unsupported file type", fn);
+ goto out;
+ }
+
+
Re: libutil: opendev: require block/character devices
On Thu, 25 Aug 2022 11:50:31 -, Klemens Nanni wrote: > And this should use the ternary operator, as this would still succeed > if path is a character device even though OPENDEV_BLCK was passed. Sure. OK millert@ - todd
Re: bgpd silence "connection from non-peer" unless verbose
On Thu, Aug 25, 2022 at 01:48:50PM +0100, Stuart Henderson wrote: > On 2022/08/25 14:38, Claudio Jeker wrote: > > On Thu, Aug 25, 2022 at 09:23:01AM +0100, Stuart Henderson wrote: > > > On 2022/08/24 18:47, Denis Fondras wrote: > > > > Le Tue, Aug 23, 2022 at 06:28:12PM +0200, Claudio Jeker a écrit : > > > > > I noticed that the "connection from non-peer" message can fill the > > > > > log and > > > > > be so chatty that it is hard to see the other messages. The system I > > > > > see > > > > > this on is a bit special since it gets hammered by incorrectly > > > > > configured > > > > > systems. Maybe other people find this message helpful. If so please > > > > > speak up now because I think the message does not add much info and > > > > > should > > > > > be skipped unless verbose logging is used. > > > > > > > > > > > > > I agree with this change (I also have a log full of this message). > > > > > > btw I like the log message, it shows me if I messed up and forgot to add a > > > session, or if someone else messed up and added a session without > > > arranging > > > it (or typoed the address, etc). But I only allow port 179 connections > > > from > > > possible candidates for peering (IXP peering lans etc) - I consider that > > > good practice anyway - and means it isn't too noisy. > > > > True but in my case of a route collector misconfigured neighbors try to > > connect more or less every other second. This results in a lot of log > > chatter that is very annoying. > > > > Maybe bgpd needs to keep some state so that the message is not shown over > > and > > over again. > > Looking at the actual log message I see -v isn't much more noisy for bgpd > anyway, so it's not a problem to use that. -v enables a lot of LOG_DEBUG messages which syslog will drop by default. This is one of the few LOG_INFO that is based on -v. Now if you log with -v it will be more noisy (but since I run bgpd often with -v I try to keep the noise down). > I thought about keeping state, but there are a lot of potential non-peers > that might try to connect, which could result in a a lot of addresses > for bgpd to keep track of :) We could use a fixed upper limit and LRU to keep the number of connections small. -- :wq Claudio
Re: libutil: opendev: require block/character devices
On Thu, 25 Aug 2022 05:36:53 -, Klemens Nanni wrote: > Ah yes, the failure check does not return early but falls through, so > all further logic needs to check fd and/or errno (like the isduid() case > already does). OK millert@ - todd
Re: bgpd silence "connection from non-peer" unless verbose
On 2022/08/25 14:38, Claudio Jeker wrote: > On Thu, Aug 25, 2022 at 09:23:01AM +0100, Stuart Henderson wrote: > > On 2022/08/24 18:47, Denis Fondras wrote: > > > Le Tue, Aug 23, 2022 at 06:28:12PM +0200, Claudio Jeker a écrit : > > > > I noticed that the "connection from non-peer" message can fill the log > > > > and > > > > be so chatty that it is hard to see the other messages. The system I see > > > > this on is a bit special since it gets hammered by incorrectly > > > > configured > > > > systems. Maybe other people find this message helpful. If so please > > > > speak up now because I think the message does not add much info and > > > > should > > > > be skipped unless verbose logging is used. > > > > > > > > > > I agree with this change (I also have a log full of this message). > > > > btw I like the log message, it shows me if I messed up and forgot to add a > > session, or if someone else messed up and added a session without arranging > > it (or typoed the address, etc). But I only allow port 179 connections from > > possible candidates for peering (IXP peering lans etc) - I consider that > > good practice anyway - and means it isn't too noisy. > > True but in my case of a route collector misconfigured neighbors try to > connect more or less every other second. This results in a lot of log > chatter that is very annoying. > > Maybe bgpd needs to keep some state so that the message is not shown over and > over again. Looking at the actual log message I see -v isn't much more noisy for bgpd anyway, so it's not a problem to use that. I thought about keeping state, but there are a lot of potential non-peers that might try to connect, which could result in a a lot of addresses for bgpd to keep track of :)
Re: bgpd silence "connection from non-peer" unless verbose
On Thu, Aug 25, 2022 at 09:23:01AM +0100, Stuart Henderson wrote: > On 2022/08/24 18:47, Denis Fondras wrote: > > Le Tue, Aug 23, 2022 at 06:28:12PM +0200, Claudio Jeker a écrit : > > > I noticed that the "connection from non-peer" message can fill the log and > > > be so chatty that it is hard to see the other messages. The system I see > > > this on is a bit special since it gets hammered by incorrectly configured > > > systems. Maybe other people find this message helpful. If so please > > > speak up now because I think the message does not add much info and should > > > be skipped unless verbose logging is used. > > > > > > > I agree with this change (I also have a log full of this message). > > btw I like the log message, it shows me if I messed up and forgot to add a > session, or if someone else messed up and added a session without arranging > it (or typoed the address, etc). But I only allow port 179 connections from > possible candidates for peering (IXP peering lans etc) - I consider that > good practice anyway - and means it isn't too noisy. True but in my case of a route collector misconfigured neighbors try to connect more or less every other second. This results in a lot of log chatter that is very annoying. Maybe bgpd needs to keep some state so that the message is not shown over and over again. -- :wq Claudio
Re: libutil: opendev: require block/character devices
On Thu, Aug 25, 2022 at 05:36:53AM +, Klemens Nanni wrote:
> On Wed, Aug 24, 2022 at 08:02:03PM -0600, Todd C. Miller wrote:
> > On Wed, 24 Aug 2022 20:06:00 -, Klemens Nanni wrote:
> >
> > > Feedback? Am I missing anything?
> >
> > If fstat(2) fails you should not try to access sb. Perhaps:
> >
> > if (((dflags & OPENDEV_BLCK) && ...
> >
> > should be an "else if (..."
>
> Ah yes, the failure check does not return early but falls through, so
> all further logic needs to check fd and/or errno (like the isduid() case
> already does).
> + } else if (((dflags & OPENDEV_BLCK) &&
> + !S_ISBLK(sb.st_mode)) ||
> + !S_ISCHR(sb.st_mode)) {
And this should use the ternary operator, as this would still succeed
if path is a character device even though OPENDEV_BLCK was passed.
OK?
Index: opendev.3
===
RCS file: /cvs/src/lib/libutil/opendev.3,v
retrieving revision 1.22
diff -u -p -r1.22 opendev.3
--- opendev.3 15 Jan 2015 19:06:32 - 1.22
+++ opendev.3 24 Aug 2022 19:34:20 -
@@ -90,10 +90,12 @@ is not
.Dv NULL ,
it is modified to point at the fully expanded device name.
.Sh RETURN VALUES
-The
+If successful,
.Fn opendev
-return value and errors are the same as the return value and errors of
-.Xr open 2 .
+returns a file descriptor.
+Otherwise, a value of -1 is returned and
+.Va errno
+is set to indicate the error.
.Sh SEE ALSO
.Xr open 2 ,
.Xr getrawpartition 3 ,
Index: opendev.c
===
RCS file: /cvs/src/lib/libutil/opendev.c,v
retrieving revision 1.15
diff -u -p -r1.15 opendev.c
--- opendev.c 30 Jun 2011 15:04:58 - 1.15
+++ opendev.c 25 Aug 2022 11:46:26 -
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include "util.h"
@@ -63,8 +64,23 @@ opendev(const char *path, int oflags, in
prefix = "r"; /* character device */
if ((slash = strchr(path, '/'))) {
+ struct stat sb;
+
strlcpy(namebuf, path, sizeof(namebuf));
fd = open(namebuf, oflags);
+
+ if (fd != -1) {
+ if (fstat(fd, ) == -1) {
+ close(fd);
+ fd = -1;
+ } else if ((dflags & OPENDEV_BLCK) ?
+ !S_ISBLK(sb.st_mode) :
+ !S_ISCHR(sb.st_mode)) {
+ close(fd);
+ fd = -1;
+ errno = ENOTBLK;
+ }
+ }
} else if (isduid(path, dflags)) {
strlcpy(namebuf, path, sizeof(namebuf));
if ((fd = open("/dev/diskmap", oflags)) != -1) {
Re: bgpd silence "connection from non-peer" unless verbose
On 2022/08/24 18:47, Denis Fondras wrote: > Le Tue, Aug 23, 2022 at 06:28:12PM +0200, Claudio Jeker a écrit : > > I noticed that the "connection from non-peer" message can fill the log and > > be so chatty that it is hard to see the other messages. The system I see > > this on is a bit special since it gets hammered by incorrectly configured > > systems. Maybe other people find this message helpful. If so please > > speak up now because I think the message does not add much info and should > > be skipped unless verbose logging is used. > > > > I agree with this change (I also have a log full of this message). btw I like the log message, it shows me if I messed up and forgot to add a session, or if someone else messed up and added a session without arranging it (or typoed the address, etc). But I only allow port 179 connections from possible candidates for peering (IXP peering lans etc) - I consider that good practice anyway - and means it isn't too noisy.
