Term::Cap full revamp
I guess i will probably leave it alone after this.
This does quite a few things compared to my former patches.
- totally get rid of eval, it doen't make sense anymore
- declare variables before they get used, which tends to
simplify things.
- change quaint formatting to something more BSD like
- update documentation to new style of doing OO
- use defined logic on entry and such
- always try to run infocmp as a last resort, even if
we have a path.
- run infocmp with the best options we have to get a good termcap
- use \Q\E, which gets rid of termpat entirely
- dedup the path along the way: for us, /etc/termcap
and /usr/share/misc/termcap are the same.
- redo recursion logic by just recording which term values we
already saw, the max=32 value overflow was absurd, proper parsing
yields roughly 10 or so tc redirections for xterm, not >32.
Index: Cap.pm
===
RCS file: /cvs/src/gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm,v
retrieving revision 1.3
diff -u -p -r1.3 Cap.pm
--- Cap.pm 18 Oct 2023 01:49:26 - 1.3
+++ Cap.pm 20 Oct 2023 09:47:05 -
@@ -16,8 +16,8 @@ sub croak
use strict;
+use v5.16;
use vars qw($VERSION $VMS_TERMCAP);
-use vars qw($termpat $state $first $entry);
$VERSION = '1.17';
@@ -33,7 +33,7 @@ Term::Cap - Perl termcap interface
=head1 SYNOPSIS
require Term::Cap;
-$terminal = Tgetent Term::Cap { TERM => undef, OSPEED => $ospeed };
+$terminal = Term::Cap->Tgetent({ TERM => undef, OSPEED => $ospeed });
$terminal->Trequire(qw/ce ku kd/);
$terminal->Tgoto('cm', $col, $row, $FH);
$terminal->Tputs('dl', $count, $FH);
@@ -75,10 +75,10 @@ if ( $^O eq 'VMS' )
sub termcap_path
{## private
-my @termcap_path;
+my @l;
# $TERMCAP, if it's a filespec
-push( @termcap_path, $ENV{TERMCAP} )
+push(@l, $ENV{TERMCAP})
if (
( exists $ENV{TERMCAP} )
&& (
@@ -87,23 +87,27 @@ sub termcap_path
: $ENV{TERMCAP} =~ /^\//s
)
);
-if ( ( exists $ENV{TERMPATH} ) && ( $ENV{TERMPATH} ) )
-{
-
+if (exists $ENV{TERMPATH} && $ENV{TERMPATH}) {
# Add the users $TERMPATH
-push( @termcap_path, split( /(:|\s+)/, $ENV{TERMPATH} ) );
-}
-else
-{
-
+push(@l, split( /(:|\s+)/, $ENV{TERMPATH}));
+} else {
# Defaults
-push( @termcap_path,
-exists $ENV{'HOME'} ? $ENV{'HOME'} . '/.termcap' : undef,
-'/etc/termcap', '/usr/share/misc/termcap', );
+ if (exists $ENV{HOME}) {
+ push(@l, $ENV{HOME}.'/.termcap');
+ }
+push(@l, '/etc/termcap', '/usr/share/misc/termcap', );
+}
+my @termcap_path;
+my $seen = {};
+for my $i (@l) {
+ next unless -f $i;
+ my $k = join(',', (stat _)[0,1]);
+ next if $seen->{$k};
+ push(@termcap_path, $i);
+ $seen->{$k} = 1;
}
-# return the list of those termcaps that exist
-return grep { defined $_ && -f $_ } @termcap_path;
+return @termcap_path;
}
=over 4
@@ -164,195 +168,158 @@ It calls C on failure.
sub Tgetent
{## public -- static method
-my $class = shift;
-my ($self) = @_;
+my ($class, $self) = @_;
$self = {} unless defined $self;
bless $self, $class;
-my ( $term, $cap, $search, $field, $max, $tmp_term, $TERMCAP );
-local ( $termpat, $state, $first, $entry );# used inside eval
+my ($cap, $field);
+
local $_;
# Compute PADDING factor from OSPEED (to be used by Tpad)
-if ( !$self->{OSPEED} )
-{
-if ($^W)
-{
+if (!$self->{OSPEED}) {
+if ($^W) {
carp "OSPEED was not set, defaulting to 9600";
}
$self->{OSPEED} = 9600;
}
-if ( $self->{OSPEED} < 16 )
-{
-
+if ($self->{OSPEED} < 16) {
# delays for old style speeds
my @pad = (
0,200, 133.3, 90.9, 74.3, 66.7, 50, 33.3,
16.7, 8.3, 5.5, 4.1, 2,1,.5, .2
);
$self->{PADDING} = $pad[ $self->{OSPEED} ];
-}
-else
-{
+} else {
$self->{PADDING} = 1 / $self->{OSPEED};
}
-unless ( $self->{TERM} )
-{
- if ( $ENV{TERM} )
- {
- $self->{TERM} = $ENV{TERM} ;
- }
- else
- {
- if ( $^O eq 'MSWin32' )
- {
+unless ($self->{TERM}) {
+ if ($ENV{TERM}) {
+ $self->{TERM} = $ENV{TERM} ;
+ } else {
+ if ( $^O eq 'MSWin32' ) {
$self->{TERM} = 'dumb';
- }
- else
- {
+ } else {
croak "TERM not set";
}
}
}
-$term = $self->{TERM};# $term is the term type we are looking for
+my $term = $self->{TERM};# $term is the term type we are looking for
# $tmp_term is always the next term (possibly :tc=...:) we are looking for
-$tmp_term = $self->{TERM};
+
better fix for Term::Cap
Instead of an archaic limit, just keep track of tc's.
This is actually slightly faster, because the termcap links is a tree,
not a list.
Please test.
Index: Cap.pm
===
RCS file: /cvs/src/gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm,v
retrieving revision 1.3
diff -u -p -r1.3 Cap.pm
--- Cap.pm 18 Oct 2023 01:49:26 - 1.3
+++ Cap.pm 18 Oct 2023 08:24:54 -
@@ -280,7 +280,7 @@ sub Tgetent
$first = 0;# first entry (keeps term name)
-$max = 64; # max :tc=...:'s
+my $seen = {}; # keep track of :tc=...:s
if ($entry)
{
@@ -291,6 +291,7 @@ sub Tgetent
if ( $entry =~ s/:tc=([^:]+):/:/ )
{
$tmp_term = $1;
+ $seen->{$tmp_term} = 1;
# protect any pattern metacharacters in $tmp_term
$termpat = $tmp_term;
@@ -332,10 +333,7 @@ sub Tgetent
}
else
{
-
# do the same file again
-# prevent endless recursion
-$max-- || croak "failed termcap loop at $tmp_term";
$state = 1;# ok, maybe do a new file next time
}
@@ -345,11 +343,20 @@ sub Tgetent
close TERMCAP;
# If :tc=...: found then search this file again
-$entry =~ s/:tc=([^:]+):/:/ && ( $tmp_term = $1, $state = 2 );
+while ($entry =~ s/:tc=([^:]+):/:/) {
+ $tmp_term = $1;
+ if ($seen->{$tmp_term}) {
+ # XXX first version of this croaked, but we can actually
+ # get several intermediate entries with the same tc !
+ next;
+ }
+ $seen->{$tmp_term} = 1;
+ $state = 2;
-# protect any pattern metacharacters in $tmp_term
-$termpat = $tmp_term;
-$termpat =~ s/(\W)/\\$1/g;
+ # protect any pattern metacharacters in $tmp_term
+ $termpat = $tmp_term;
+ $termpat =~ s/(\W)/\\$1/g;
+ }
}
croak "Can't find $term" if $entry eq '';
Termp/Cap
The max approach is bogus. It happens because the terminal uses tc, and then tc again. The proper approach, especially in perl, is to actually detect a loop, which should be fairly easy by saving the tc that have already been done in a hash.
Re: Don't lock package db if fw_update(8) is not installing firmware
On Sat, Oct 14, 2023 at 11:24:08AM -0700, Andrew Hewus Fresh wrote:
> While testing another patch, I noticed that fw_update will lock the
> package database even if it's just downloading and not touching the
> installed packages.
>
> Currently we do _read_ the existing firmware as part of detecting what
> we might need to download or upgrade without locking. I'm unsure if
> that is an issue.
>
> Comments, OK?
>
>
>
> Index: fw_update.sh
> ===
> RCS file: /cvs/src/usr.sbin/fw_update/fw_update.sh,v
> retrieving revision 1.51
> diff -u -p -r1.51 fw_update.sh
> --- fw_update.sh 14 Oct 2023 18:10:47 - 1.51
> +++ fw_update.sh 14 Oct 2023 18:12:08 -
> @@ -593,7 +593,7 @@ kept=''
> unregister=''
>
> if [ "${devices[*]:-}" ]; then
> - lock_db
> + "$INSTALL" && lock_db
> for f in "${devices[@]}"; do
> d="$( firmware_devicename "$f" )"
>
>
>
the package tools have two lock modes: read-only and read-write.
If you want to match them, you want lock read-only for this.
Re: Buffer overflow in /usr/bin/deroff
On Tue, Sep 19, 2023 at 09:48:25AM -0300, Crystal Kolipe wrote: > deroff chokes when given lines > 2048 bytes, and produces non-deterministic > output on little endian archs. > > Reproducer: > > $ jot -s '' -b blah 513 > /tmp/blah > $ for i in 1 2 3 4 ; do deroff /tmp/blah | md5 ; done > 2d8f4eebd61ed2a07101419169fc9997 > ae19be78a09e6b371787003bf76f5937 > 82b4bcea8510605bea2723ffc70c99b4 > 0ea7b0ddc76d2a280dd30cff6a69574e Since you went to the trouble of reproducing the issue, it would be great if you could turn that into a non-regression test (deroff has none so far, but creating a new Makefile would be comparatively easy, you just need to massage this into a Makefile target that doesn't error our when the bug is fixed) That might even entice people into writing more non-regression tests :) Thank you. -- Marc
Re: path: speed-up pkg-config
On Mon, Sep 11, 2023 at 09:55:53AM +0200, Marc Espie wrote:
> Not to pkgconf levels, but still way faster than what we had
>
> Updated patch from what I've shown to people, turns out the second grep
> wasn't quite working.
>
> This does cache the set_variables_from_env shennanigans, speeding up large
> processing of recursive files by a large factor (since we keep a cache
> of relevant env variables, and don't bother setting the same value twice)
>
>
> The optimisation in PkgConfig.pm is sligtly less powerful: we got a marker
> for variable expansions straight up when we parse a pkgconfig file, before
> even splitting into lists, so instead of "raw" lists,
> tag them as NoExpand/ToExpand classes, so that we can forego variable
> expansion altogether.
>
> Please test, this appears to pass regress, and I've just put this into
> a partial bulk.
Oops, as gkoehler noticed, I sent out the old patch.
Here's the actual fixed one:
Index: pkg-config
===
RCS file: /cvs/src/usr.bin/pkg-config/pkg-config,v
retrieving revision 1.96
diff -u -p -r1.96 pkg-config
--- pkg-config 8 Jun 2023 08:55:27 - 1.96
+++ pkg-config 11 Sep 2023 07:50:57 -
@@ -279,6 +279,26 @@ if ($mode{cflags} || $mode{libs} || $mod
exit $rc;
###
+sub set_variables_from_env($file)
+{
+ state (%done, @l);
+
+ if (!defined $done{$file}) {
+ my $pkg = $file;
+
+ $pkg =~ s/(^.*\/)?(.*?)\.pc$/$2/g;
+ $pkg = uc($pkg);
+ if (!@l) {
+ @l = grep {/PKG_CONFIG_/} keys %ENV;
+ }
+ for my $k (@l) {
+ next unless $k =~ m/PKG_CONFIG_${pkg}_(\w+)/;
+ $variables->{lc($1)} = $ENV{$k};
+ }
+ $done{$file} = 1;
+ }
+
+}
sub handle_config($p, $op, $v, $list)
{
@@ -300,22 +320,7 @@ sub handle_config($p, $op, $v, $list)
}
my $get_props = sub($property) {
- my $pkg;
-
- # See if there's anything in the environment that we need to
- # take into account.
- ($pkg = $p) =~ s/(^.*\/)?(.*?)\.pc$/$2/g;
- $pkg = uc($pkg);
-
- if (grep {/PKG_CONFIG_${pkg}.*/} keys %ENV) {
- # Now that we know we have something to look for, do
- # the inefficient iteration.
- while (my ($k, $v) = each %ENV) {
- if ($k =~ /^PKG_CONFIG_${pkg}_(\w+)/) {
- $variables->{lc($1)} = $v;
- }
- }
- }
+ set_variables_from_env($p);
my $deps = $cfg->get_property($property, $variables);
return unless defined $deps;
Index: OpenBSD/PkgConfig.pm
===
RCS file: /cvs/src/usr.bin/pkg-config/OpenBSD/PkgConfig.pm,v
retrieving revision 1.10
diff -u -p -r1.10 PkgConfig.pm
--- OpenBSD/PkgConfig.pm8 Jun 2023 08:55:27 - 1.10
+++ OpenBSD/PkgConfig.pm11 Sep 2023 07:50:57 -
@@ -16,6 +16,7 @@
use v5.36;
+
# interface to the *.pc file format of pkg-config.
package OpenBSD::PkgConfig;
@@ -72,10 +73,14 @@ sub add_variable($self, $name, $value)
sub parse_value($self, $name, $value)
{
+ my $class = "OpenBSD::PkgConfig::NoExpand";
+ if ($value =~ m/\$\{.*\}/) {
+ $class = "OpenBSD::PkgConfig::ToExpand";
+ }
if (defined $parse->{$name}) {
- return $parse->{$name}($value);
+ return bless $parse->{$name}($value), $class;
} else {
- return [split /(?parse_value($name, $value);
} else {
- $v = [];
+ $v = bless [], "OpenBSD::PkgConfig::NoExpand";
}
$self->{properties}{$name} = $v;
}
@@ -121,8 +126,9 @@ sub read_fh($class, $fh, $name = '')
}
}
if (defined $cfg->{properties}{Libs}) {
- $cfg->{properties}{Libs} =
- $cfg->compress_list($cfg->{properties}{Libs});
+ $cfg->{properties}{Libs} = bless
+ $cfg->compress_list($cfg->{properties}{Libs}),
+ ref($cfg->{properties}{Libs});
}
return $cfg;
}
@@ -220,6 +226,9 @@ sub get_property($self, $k, $extra = {})
if (!defined $l) {
return undef;
}
+ if ($l->noexpand) {
+ return [@$l];
+ }
my $r = [];
for my $v (@$l) {
my $w = $self->expanded($v, $extra);
@@ -263,4 +272,17
Re: ps.1/kvm documentation
On Mon, Sep 11, 2023 at 12:10:17PM +0200, Claudio Jeker wrote: > On Mon, Sep 11, 2023 at 11:02:00AM +0200, Marc Espie wrote: > > I was reading through ps.1, which has two slightly different options > > -H Also display information about kernel visible threads. > > -k Also display information about kernel threads. > > > > It's not at all obvious what the difference between these options might be. > > > > kernel threads == kthread(9) created threads > > Those should have K in STAT and the name is in () like: > 3141 ?? RK/1 4057:57.90 (idle1) > > kernel visible threads == __tfork_thread(3) created threads for userland > applications. For example: > > 43838 556612 ?? IpU 0:01.58 firefox (firefox/Cache2 I/O) > 43838 415551 ?? IpU 0:00.01 firefox (firefox/Cookie) > 43838 377915 ?? IpU 0:00.01 firefox (firefox/Worker Launcher) > > These threads all share the same PID but have different TID. > > I think the "kernel visible" is there to tell that pure userland threads > can not be reported by ps. I think go routines are such an example. > > > From the log: > > > > revision 1.77 > > date: 2011/09/25 00:29:59; author: guenther; state: Exp; lines: +5 -3; > > Add -H option to show rthreads, hiding them by default > > > > Diff from uwe@ > > > > so slightly more info. > > > > Looking at the code, now this is KERN_PROC_KTHREAD vs KERN_PROC_SHOW_THREADS > > in kvm_getprocs(3). > > > > Now KERN_PROC_KTHREAD is documented, but there is nothing about > > KERN_PROC_SHOW_THREADS. > > > > The code around (dothreads) in kvm* doesn't make things really obvious. So far I have two explanations, but it's far away from my comfort zone that I don't know how to make the documentation less crappy. Any of you guys willing to provide a patch to ps(1) and kvm_getprocs(3) ?
Re: ps.1/kvm documentation
On Mon, Sep 11, 2023 at 11:00:41AM +0100, Stuart Henderson wrote: > -H is userland threads, -k is kernel threads. I guess "kernel visible" was > to distinguish between the old uthread where threads were handled in > userland and not visible to the kernel, and rthread ... > > -- > Sent from a phone, apologies for poor formatting. > > On 11 September 2023 10:02:32 Marc Espie wrote: > > > I was reading through ps.1, which has two slightly different options > > -H Also display information about kernel visible threads. > > -k Also display information about kernel threads. > > > > > > It's not at all obvious what the difference between these options might be. > > > > From the log: > > > > revision 1.77 > > date: 2011/09/25 00:29:59; author: guenther; state: Exp; lines: +5 -3; > > Add -H option to show rthreads, hiding them by default > > > > Diff from uwe@ > > > > so slightly more info. > > > > Looking at the code, now this is KERN_PROC_KTHREAD vs KERN_PROC_SHOW_THREADS > > in kvm_getprocs(3). > > > > Now KERN_PROC_KTHREAD is documented, but there is nothing about > > KERN_PROC_SHOW_THREADS. > > > > The code around (dothreads) in kvm* doesn't make things really obvious. > We can probably just say "userland threads"/"kernel threads" these days ? I expect documenting KERN_PROC_SHOW_THREADS would be a good idea /
ps.1/kvm documentation
I was reading through ps.1, which has two slightly different options -H Also display information about kernel visible threads. -k Also display information about kernel threads. It's not at all obvious what the difference between these options might be. >From the log: revision 1.77 date: 2011/09/25 00:29:59; author: guenther; state: Exp; lines: +5 -3; Add -H option to show rthreads, hiding them by default Diff from uwe@ so slightly more info. Looking at the code, now this is KERN_PROC_KTHREAD vs KERN_PROC_SHOW_THREADS in kvm_getprocs(3). Now KERN_PROC_KTHREAD is documented, but there is nothing about KERN_PROC_SHOW_THREADS. The code around (dothreads) in kvm* doesn't make things really obvious.
path: speed-up pkg-config
Not to pkgconf levels, but still way faster than what we had
Updated patch from what I've shown to people, turns out the second grep
wasn't quite working.
This does cache the set_variables_from_env shennanigans, speeding up large
processing of recursive files by a large factor (since we keep a cache
of relevant env variables, and don't bother setting the same value twice)
The optimisation in PkgConfig.pm is sligtly less powerful: we got a marker
for variable expansions straight up when we parse a pkgconfig file, before
even splitting into lists, so instead of "raw" lists,
tag them as NoExpand/ToExpand classes, so that we can forego variable
expansion altogether.
Please test, this appears to pass regress, and I've just put this into
a partial bulk.
Index: pkg-config
===
RCS file: /cvs/src/usr.bin/pkg-config/pkg-config,v
retrieving revision 1.96
diff -u -p -r1.96 pkg-config
--- pkg-config 8 Jun 2023 08:55:27 - 1.96
+++ pkg-config 11 Sep 2023 07:11:54 -
@@ -279,6 +279,25 @@ if ($mode{cflags} || $mode{libs} || $mod
exit $rc;
###
+sub set_variables_from_env($file)
+{
+ state (%done, @l);
+
+ if (!defined $done{$file}) {
+ my $pkg = $file;
+
+ $pkg =~ s/(^.*\/)?(.*?)\.pc$/$2/g;
+ $pkg = uc($pkg);
+ if (!@l) {
+ @l = grep {/PKG_CONFIG_/} keys %ENV;
+ }
+ for my $k (grep {/PKG_CONFIG_${pkg}_(\w+)/} @l) {
+ $variables->{lc($1)} = $ENV{$k};
+ }
+ $done{$file} = 1;
+ }
+
+}
sub handle_config($p, $op, $v, $list)
{
@@ -300,22 +319,7 @@ sub handle_config($p, $op, $v, $list)
}
my $get_props = sub($property) {
- my $pkg;
-
- # See if there's anything in the environment that we need to
- # take into account.
- ($pkg = $p) =~ s/(^.*\/)?(.*?)\.pc$/$2/g;
- $pkg = uc($pkg);
-
- if (grep {/PKG_CONFIG_${pkg}.*/} keys %ENV) {
- # Now that we know we have something to look for, do
- # the inefficient iteration.
- while (my ($k, $v) = each %ENV) {
- if ($k =~ /^PKG_CONFIG_${pkg}_(\w+)/) {
- $variables->{lc($1)} = $v;
- }
- }
- }
+ set_variables_from_env($p);
my $deps = $cfg->get_property($property, $variables);
return unless defined $deps;
Index: OpenBSD/PkgConfig.pm
===
RCS file: /cvs/src/usr.bin/pkg-config/OpenBSD/PkgConfig.pm,v
retrieving revision 1.10
diff -u -p -r1.10 PkgConfig.pm
--- OpenBSD/PkgConfig.pm8 Jun 2023 08:55:27 - 1.10
+++ OpenBSD/PkgConfig.pm11 Sep 2023 07:11:54 -
@@ -16,6 +16,7 @@
use v5.36;
+
# interface to the *.pc file format of pkg-config.
package OpenBSD::PkgConfig;
@@ -72,10 +73,14 @@ sub add_variable($self, $name, $value)
sub parse_value($self, $name, $value)
{
+ my $class = "OpenBSD::PkgConfig::NoExpand";
+ if ($value =~ m/\$\{.*\}/) {
+ $class = "OpenBSD::PkgConfig::ToExpand";
+ }
if (defined $parse->{$name}) {
- return $parse->{$name}($value);
+ return bless $parse->{$name}($value), $class;
} else {
- return [split /(?parse_value($name, $value);
} else {
- $v = [];
+ $v = bless [], "OpenBSD::PkgConfig::NoExpand";
}
$self->{properties}{$name} = $v;
}
@@ -121,8 +126,9 @@ sub read_fh($class, $fh, $name = '')
}
}
if (defined $cfg->{properties}{Libs}) {
- $cfg->{properties}{Libs} =
- $cfg->compress_list($cfg->{properties}{Libs});
+ $cfg->{properties}{Libs} = bless
+ $cfg->compress_list($cfg->{properties}{Libs}),
+ ref($cfg->{properties}{Libs});
}
return $cfg;
}
@@ -220,6 +226,9 @@ sub get_property($self, $k, $extra = {})
if (!defined $l) {
return undef;
}
+ if ($l->noexpand) {
+ return [@$l];
+ }
my $r = [];
for my $v (@$l) {
my $w = $self->expanded($v, $extra);
@@ -263,4 +272,17 @@ sub add_bases($self, $extra)
}
}
+package OpenBSD::PkgConfig::NoExpand;
+our @ISA = qw(OpenBSD::PkgConfig);
+sub noexpand($)
+{
+ 1
+}
+
+package OpenBSD::PkgConfig::ToExpand;
+our @ISA = qw(OpenBSD::PkgConfig);
+sub noexpand($)
+{
+ 0
+}
1;
Re: unifdef HAS_INLINES in make(1)
On Tue, Sep 05, 2023 at 11:51:34AM +1000, Jonathan Gray wrote:
> On Mon, Sep 04, 2023 at 11:51:33PM +0200, Marc Espie wrote:
> > On Tue, Sep 05, 2023 at 12:04:24AM +1000, Jonathan Gray wrote:
> > > inline is part of gnu89 and c99
> > >
> > > Index: defines.h
> > > ===
> > > RCS file: /cvs/src/usr.bin/make/defines.h,v
> > > retrieving revision 1.15
> > > diff -u -p -r1.15 defines.h
> > > --- defines.h 14 Oct 2015 13:50:22 - 1.15
> > > +++ defines.h 4 Sep 2023 13:49:16 -
> > > @@ -61,16 +61,8 @@ typedef struct Suff_ Suff;
> > >
> > > #ifdef __GNUC__
> > > # define UNUSED __attribute__((__unused__))
> > > -# define HAS_INLINES
> > > -# define INLINE __inline__
> > > #else
> > > # define UNUSED
> > > -#endif
> > > -
> > > -#ifdef HAS_INLINES
> > > -# ifndef INLINE
> > > -# define INLINE inline
> > > -# endif
> > > #endif
> > >
> > > /*
> > > Index: lst.h
> > > ===
> > > RCS file: /cvs/src/usr.bin/make/lst.h,v
> > > retrieving revision 1.33
> > > diff -u -p -r1.33 lst.h
> > > --- lst.h 4 Mar 2021 09:45:31 - 1.33
> > > +++ lst.h 4 Sep 2023 13:49:52 -
> > > @@ -159,25 +159,16 @@ extern void * Lst_DeQueue(Lst);
> > > #define Lst_Adv(ln) ((ln)->nextPtr)
> > > #define Lst_Rev(ln) ((ln)->prevPtr)
> > >
> > > -
> > > -/* Inlines are preferable to macros here because of the type checking. */
> > > -#ifdef HAS_INLINES
> > > -static INLINE LstNode
> > > +static inline LstNode
> > > Lst_FindConst(Lst l, FindProcConst cProc, const void *d)
> > > {
> > > return Lst_FindFrom(Lst_First(l), (FindProc)cProc, (void *)d);
> > > }
> > >
> > > -static INLINE LstNode
> > > +static inline LstNode
> > > Lst_FindFromConst(LstNode ln, FindProcConst cProc, const void *d)
> > > {
> > > return Lst_FindFrom(ln, (FindProc)cProc, (void *)d);
> > > }
> > > -#else
> > > -#define Lst_FindConst(l, cProc, d) \
> > > - Lst_FindFrom(Lst_First(l), (FindProc)cProc, (void *)d)
> > > -#define Lst_FindFromConst(ln, cProc, d) \
> > > - Lst_FindFrom(ln, (FindProc)cProc, (void *)d)
> > > -#endif
> > >
> > > #endif /* _LST_H_ */
> > > Index: lst.lib/lst.h
> > > ===
> > > RCS file: /cvs/src/usr.bin/make/lst.lib/lst.h,v
> > > retrieving revision 1.2
> > > diff -u -p -r1.2 lst.h
> > > --- lst.lib/lst.h 14 Oct 2015 13:52:11 - 1.2
> > > +++ lst.lib/lst.h 4 Sep 2023 13:50:30 -
> > > @@ -154,25 +154,16 @@ extern void * Lst_DeQueue(Lst);
> > > #define Lst_Adv(ln) ((ln)->nextPtr)
> > > #define Lst_Rev(ln) ((ln)->prevPtr)
> > >
> > > -
> > > -/* Inlines are preferable to macros here because of the type checking. */
> > > -#ifdef HAS_INLINES
> > > -static INLINE LstNode
> > > +static inline LstNode
> > > Lst_FindConst(Lst l, FindProcConst cProc, const void *d)
> > > {
> > > return Lst_FindFrom(Lst_First(l), (FindProc)cProc, (void *)d);
> > > }
> > >
> > > -static INLINE LstNode
> > > +static inline LstNode
> > > Lst_FindFromConst(LstNode ln, FindProcConst cProc, const void *d)
> > > {
> > > return Lst_FindFrom(ln, (FindProc)cProc, (void *)d);
> > > }
> > > -#else
> > > -#define Lst_FindConst(l, cProc, d) \
> > > - Lst_FindFrom(Lst_First(l), (FindProc)cProc, (void *)d)
> > > -#define Lst_FindFromConst(ln, cProc, d) \
> > > - Lst_FindFrom(ln, (FindProc)cProc, (void *)d)
> > > -#endif
> > >
> > > #endif /* _LST_H_ */
> > >
> > >
> > Please Cc: me on make stuff, I don't always see all mails.
> >
> > Have you checked it works on gcc3 or gotten someone to check ?
> > If so, it's okay.
>
> No, but it must work. Used in many places, such as:
> sys/kern/subr_pool.c:static inline int
> usr.bin/sed/process.c:static inline int
>
>
Okay then
Re: unifdef HAS_INLINES in make(1)
On Tue, Sep 05, 2023 at 12:04:24AM +1000, Jonathan Gray wrote:
> inline is part of gnu89 and c99
>
> Index: defines.h
> ===
> RCS file: /cvs/src/usr.bin/make/defines.h,v
> retrieving revision 1.15
> diff -u -p -r1.15 defines.h
> --- defines.h 14 Oct 2015 13:50:22 - 1.15
> +++ defines.h 4 Sep 2023 13:49:16 -
> @@ -61,16 +61,8 @@ typedef struct Suff_ Suff;
>
> #ifdef __GNUC__
> # define UNUSED __attribute__((__unused__))
> -# define HAS_INLINES
> -# define INLINE __inline__
> #else
> # define UNUSED
> -#endif
> -
> -#ifdef HAS_INLINES
> -# ifndef INLINE
> -# define INLINE inline
> -# endif
> #endif
>
> /*
> Index: lst.h
> ===
> RCS file: /cvs/src/usr.bin/make/lst.h,v
> retrieving revision 1.33
> diff -u -p -r1.33 lst.h
> --- lst.h 4 Mar 2021 09:45:31 - 1.33
> +++ lst.h 4 Sep 2023 13:49:52 -
> @@ -159,25 +159,16 @@ extern void * Lst_DeQueue(Lst);
> #define Lst_Adv(ln) ((ln)->nextPtr)
> #define Lst_Rev(ln) ((ln)->prevPtr)
>
> -
> -/* Inlines are preferable to macros here because of the type checking. */
> -#ifdef HAS_INLINES
> -static INLINE LstNode
> +static inline LstNode
> Lst_FindConst(Lst l, FindProcConst cProc, const void *d)
> {
> return Lst_FindFrom(Lst_First(l), (FindProc)cProc, (void *)d);
> }
>
> -static INLINE LstNode
> +static inline LstNode
> Lst_FindFromConst(LstNode ln, FindProcConst cProc, const void *d)
> {
> return Lst_FindFrom(ln, (FindProc)cProc, (void *)d);
> }
> -#else
> -#define Lst_FindConst(l, cProc, d) \
> - Lst_FindFrom(Lst_First(l), (FindProc)cProc, (void *)d)
> -#define Lst_FindFromConst(ln, cProc, d) \
> - Lst_FindFrom(ln, (FindProc)cProc, (void *)d)
> -#endif
>
> #endif /* _LST_H_ */
> Index: lst.lib/lst.h
> ===
> RCS file: /cvs/src/usr.bin/make/lst.lib/lst.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 lst.h
> --- lst.lib/lst.h 14 Oct 2015 13:52:11 - 1.2
> +++ lst.lib/lst.h 4 Sep 2023 13:50:30 -
> @@ -154,25 +154,16 @@ extern void * Lst_DeQueue(Lst);
> #define Lst_Adv(ln) ((ln)->nextPtr)
> #define Lst_Rev(ln) ((ln)->prevPtr)
>
> -
> -/* Inlines are preferable to macros here because of the type checking. */
> -#ifdef HAS_INLINES
> -static INLINE LstNode
> +static inline LstNode
> Lst_FindConst(Lst l, FindProcConst cProc, const void *d)
> {
> return Lst_FindFrom(Lst_First(l), (FindProc)cProc, (void *)d);
> }
>
> -static INLINE LstNode
> +static inline LstNode
> Lst_FindFromConst(LstNode ln, FindProcConst cProc, const void *d)
> {
> return Lst_FindFrom(ln, (FindProc)cProc, (void *)d);
> }
> -#else
> -#define Lst_FindConst(l, cProc, d) \
> - Lst_FindFrom(Lst_First(l), (FindProc)cProc, (void *)d)
> -#define Lst_FindFromConst(ln, cProc, d) \
> - Lst_FindFrom(ln, (FindProc)cProc, (void *)d)
> -#endif
>
> #endif /* _LST_H_ */
>
>
Please Cc: me on make stuff, I don't always see all mails.
Have you checked it works on gcc3 or gotten someone to check ?
If so, it's okay.
Re: PATCH: remove "dead code" in make
On Thu, Aug 31, 2023 at 08:59:53AM +0200, Marc Espie wrote:
> A long time ago, I tried to host our fork of make, in the hope it would get
> picked up by other systems.
>
> Accordingly, some features were added to mimic netbsd's extensions, hidden
> behind FEATURES macros.
>
> Turns out that, for better or for worse, FreeBSD decided to go with NetBSD's
> fork of bmake, so this is just dead weight that makes stuff more complex.
>
> Accordingly, config.h is no longer really needed, as it shrinks to two
> little defines.
>
> (the only part worth looking at is varmodifiers... this survived a full build
> of base and xenocara without any issue, along with quite a few ports)
Slightly updated version. I hadn't bothered to actually remove config.h
and thus missed an include.
Also: all remaining modifiers are now strictly either word_apply or apply,
none does both, so I killed the extra code and added an assert just in case.
I could use some okays. Fairly safe.
Index: Makefile
===
RCS file: /cvs/src/usr.bin/make/Makefile,v
retrieving revision 1.64
diff -u -p -r1.64 Makefile
--- Makefile13 Jan 2020 15:41:53 - 1.64
+++ Makefile3 Sep 2023 16:44:43 -
@@ -6,8 +6,7 @@ HOSTCFLAGS+= -I${.OBJDIR} -I${.CURDIR}
CDIAGFLAGS=-Wall -W -Wno-char-subscripts -Wstrict-prototypes -pedantic \
-Wmissing-prototypes -Wdeclaration-after-statement -std=c99
-CDEFS+=-DHAS_PATHS_H
-CDEFS+=-DHAS_EXTENDED_GETCWD
+CDEFS+=-DMAKE_BSIZE=256 -DDEFMAXJOBS=4
#CDEFS+=-DHAS_STATS
DPADD += ${LIBUTIL}
Index: arch.c
===
RCS file: /cvs/src/usr.bin/make/arch.c,v
retrieving revision 1.93
diff -u -p -r1.93 arch.c
--- arch.c 17 Feb 2023 17:59:36 - 1.93
+++ arch.c 3 Sep 2023 16:44:43 -
@@ -82,7 +82,6 @@
#include
#include
#include
-#include "config.h"
#include "defines.h"
#include "buf.h"
#include "dir.h"
Index: buf.c
===
RCS file: /cvs/src/usr.bin/make/buf.c,v
retrieving revision 1.29
diff -u -p -r1.29 buf.c
--- buf.c 13 Jan 2020 13:54:44 - 1.29
+++ buf.c 3 Sep 2023 16:44:43 -
@@ -75,7 +75,6 @@
#include
#include
#include
-#include "config.h"
#include "defines.h"
#include "buf.h"
#include "stats.h"
Index: cmd_exec.c
===
RCS file: /cvs/src/usr.bin/make/cmd_exec.c,v
retrieving revision 1.12
diff -u -p -r1.12 cmd_exec.c
--- cmd_exec.c 31 Aug 2023 06:53:28 - 1.12
+++ cmd_exec.c 3 Sep 2023 16:44:43 -
@@ -30,7 +30,6 @@
#include
#include
#include
-#include "config.h"
#include "defines.h"
#include "cmd_exec.h"
#include "buf.h"
Index: compat.c
===
RCS file: /cvs/src/usr.bin/make/compat.c,v
retrieving revision 1.93
diff -u -p -r1.93 compat.c
--- compat.c26 Jan 2020 12:41:21 - 1.93
+++ compat.c3 Sep 2023 16:44:43 -
@@ -40,7 +40,6 @@
#include
#include
#include
-#include "config.h"
#include "defines.h"
#include "dir.h"
#include "engine.h"
Index: cond.c
===
RCS file: /cvs/src/usr.bin/make/cond.c,v
retrieving revision 1.54
diff -u -p -r1.54 cond.c
--- cond.c 21 Dec 2019 15:29:25 - 1.54
+++ cond.c 3 Sep 2023 16:44:44 -
@@ -42,7 +42,6 @@
#include
#include
#include
-#include "config.h"
#include "defines.h"
#include "dir.h"
#include "buf.h"
Index: config.h
===
RCS file: config.h
diff -N config.h
--- config.h5 Jan 2022 02:00:55 - 1.21
+++ /dev/null 1 Jan 1970 00:00:00 -
@@ -1,120 +0,0 @@
-#ifndef CONFIG_H
-#define CONFIG_H
-
-/* $OpenBSD: config.h,v 1.21 2022/01/05 02:00:55 jsg Exp $ */
-/* $NetBSD: config.h,v 1.7 1996/11/06 17:59:03 christos Exp $ */
-
-/*
- * Copyright (c) 1988, 1989, 1990 The Regents of the University of California.
- * Copyright (c) 1988, 1989 by Adam de Boor
- * Copyright (c) 1989 by Berkeley Softworks
- * All rights reserved.
- *
- * This code is derived from software contributed to Berkeley by
- * Adam de Boor.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * 1. Redistributions of source code must retain the above copyright
- *notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- *notice, this list of conditions and the following disc
PATCH: remove "dead code" in make
A long time ago, I tried to host our fork of make, in the hope it would get
picked up by other systems.
Accordingly, some features were added to mimic netbsd's extensions, hidden
behind FEATURES macros.
Turns out that, for better or for worse, FreeBSD decided to go with NetBSD's
fork of bmake, so this is just dead weight that makes stuff more complex.
Accordingly, config.h is no longer really needed, as it shrinks to two
little defines.
(the only part worth looking at is varmodifiers... this survived a full build
of base and xenocara without any issue, along with quite a few ports)
Index: Makefile
===
RCS file: /cvs/src/usr.bin/make/Makefile,v
retrieving revision 1.64
diff -u -p -r1.64 Makefile
--- Makefile13 Jan 2020 15:41:53 - 1.64
+++ Makefile31 Aug 2023 06:56:02 -
@@ -6,8 +6,7 @@ HOSTCFLAGS+= -I${.OBJDIR} -I${.CURDIR}
CDIAGFLAGS=-Wall -W -Wno-char-subscripts -Wstrict-prototypes -pedantic \
-Wmissing-prototypes -Wdeclaration-after-statement -std=c99
-CDEFS+=-DHAS_PATHS_H
-CDEFS+=-DHAS_EXTENDED_GETCWD
+CDEFS+=-DMAKE_BSIZE=256 -DDEFMAXJOBS=4
#CDEFS+=-DHAS_STATS
DPADD += ${LIBUTIL}
Index: arch.c
===
RCS file: /cvs/src/usr.bin/make/arch.c,v
retrieving revision 1.93
diff -u -p -r1.93 arch.c
--- arch.c 17 Feb 2023 17:59:36 - 1.93
+++ arch.c 31 Aug 2023 06:56:02 -
@@ -82,7 +82,6 @@
#include
#include
#include
-#include "config.h"
#include "defines.h"
#include "buf.h"
#include "dir.h"
Index: buf.c
===
RCS file: /cvs/src/usr.bin/make/buf.c,v
retrieving revision 1.29
diff -u -p -r1.29 buf.c
--- buf.c 13 Jan 2020 13:54:44 - 1.29
+++ buf.c 31 Aug 2023 06:56:02 -
@@ -75,7 +75,6 @@
#include
#include
#include
-#include "config.h"
#include "defines.h"
#include "buf.h"
#include "stats.h"
Index: cmd_exec.c
===
RCS file: /cvs/src/usr.bin/make/cmd_exec.c,v
retrieving revision 1.12
diff -u -p -r1.12 cmd_exec.c
--- cmd_exec.c 31 Aug 2023 06:53:28 - 1.12
+++ cmd_exec.c 31 Aug 2023 06:56:02 -
@@ -30,7 +30,6 @@
#include
#include
#include
-#include "config.h"
#include "defines.h"
#include "cmd_exec.h"
#include "buf.h"
Index: compat.c
===
RCS file: /cvs/src/usr.bin/make/compat.c,v
retrieving revision 1.93
diff -u -p -r1.93 compat.c
--- compat.c26 Jan 2020 12:41:21 - 1.93
+++ compat.c31 Aug 2023 06:56:02 -
@@ -40,7 +40,6 @@
#include
#include
#include
-#include "config.h"
#include "defines.h"
#include "dir.h"
#include "engine.h"
Index: cond.c
===
RCS file: /cvs/src/usr.bin/make/cond.c,v
retrieving revision 1.54
diff -u -p -r1.54 cond.c
--- cond.c 21 Dec 2019 15:29:25 - 1.54
+++ cond.c 31 Aug 2023 06:56:03 -
@@ -42,7 +42,6 @@
#include
#include
#include
-#include "config.h"
#include "defines.h"
#include "dir.h"
#include "buf.h"
Index: dir.c
===
RCS file: /cvs/src/usr.bin/make/dir.c,v
retrieving revision 1.68
diff -u -p -r1.68 dir.c
--- dir.c 21 Oct 2016 16:12:38 - 1.68
+++ dir.c 31 Aug 2023 06:56:03 -
@@ -70,7 +70,6 @@
#include
#include
#include
-#include "config.h"
#include "defines.h"
#include "dir.h"
#include "lst.h"
Index: direxpand.c
===
RCS file: /cvs/src/usr.bin/make/direxpand.c,v
retrieving revision 1.8
diff -u -p -r1.8 direxpand.c
--- direxpand.c 21 Oct 2016 16:12:38 - 1.8
+++ direxpand.c 31 Aug 2023 06:56:03 -
@@ -62,7 +62,6 @@
#include
#include
#include
-#include "config.h"
#include "defines.h"
#include "lst.h"
#include "dir.h"
Index: engine.c
===
RCS file: /cvs/src/usr.bin/make/engine.c,v
retrieving revision 1.72
diff -u -p -r1.72 engine.c
--- engine.c31 Aug 2023 06:53:28 - 1.72
+++ engine.c31 Aug 2023 06:56:03 -
@@ -73,7 +73,6 @@
#include
#include
#include
-#include "config.h"
#include "defines.h"
#include "cmd_exec.h"
#include "dir.h"
Index: enginechoice.c
===
RCS file: /cvs/src/usr.bin/make/enginechoice.c,v
retrieving revision 1.3
diff -u -p -r1.3 enginechoice.c
--- enginechoice.c 4 Mar 2021 09:34:30 - 1.3
+++ enginechoice.c 31 Aug 2023 06:56:03 -
@@ -23,7 +23,6 @@
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
-#include "config.h"
#include "defines.h"
#include "compat.h"
#include "make.h"
Re: PATCH: unify VAR!=cmd and other command execution
On Mon, Aug 28, 2023 at 10:03:46PM +0200, Theo Buehler wrote: > On Mon, Aug 28, 2023 at 09:18:40PM +0200, Marc Espie wrote: > > On Mon, Aug 28, 2023 at 08:42:50PM +0200, Marc Espie wrote: > > > Turns out the exec in cmd_exec.c has absolutely zero reason to be > > > different from what engine does. > > > > > > This small patch moves a bit of code around, so that all execv() consumers > > > benefit from the same optimisation (namely, no extra shell when not > > > needed). > > > > > > The only visible change should be that now, VAR!=cmd *will* display > > > some relevant information if exec fails. > > > > > > This is a fairly trivial change, I don't expect any fallout. > > > > > > (still need to run it through tests) > > > > Better with the patch (thx miod@) > > > > Index: cmd_exec.c > > [...] > > Please drop this: > > > +static void retry_with_temp_file(bool, char **); > > Otherwise ok once you're happy with your testing. > > Oh, I thought I had removed the code along with the call. Yeah, I don't intend to commit that part with the patch anyhow. Gonna run a build or two just to be sure.
Re: PATCH: unify VAR!=cmd and other command execution
On Mon, Aug 28, 2023 at 08:42:50PM +0200, Marc Espie wrote:
> Turns out the exec in cmd_exec.c has absolutely zero reason to be
> different from what engine does.
>
> This small patch moves a bit of code around, so that all execv() consumers
> benefit from the same optimisation (namely, no extra shell when not needed).
>
> The only visible change should be that now, VAR!=cmd *will* display
> some relevant information if exec fails.
>
> This is a fairly trivial change, I don't expect any fallout.
>
> (still need to run it through tests)
Better with the patch (thx miod@)
Index: cmd_exec.c
===
RCS file: /cvs/src/usr.bin/make/cmd_exec.c,v
retrieving revision 1.11
diff -u -p -r1.11 cmd_exec.c
--- cmd_exec.c 16 Jan 2020 16:07:18 - 1.11
+++ cmd_exec.c 28 Aug 2023 18:39:37 -
@@ -28,6 +28,8 @@
#include
#include
#include
+#include
+#include
#include
#include "config.h"
#include "defines.h"
@@ -36,11 +38,94 @@
#include "memory.h"
#include "pathnames.h"
#include "job.h"
+#include "str.h"
+
+/* The following array is used to make a fast determination of which
+ * characters are interpreted specially by the shell. If a command
+ * contains any of these characters, it is executed by the shell, not
+ * directly by us. */
+static charmeta[256];
+static void retry_with_temp_file(bool, char **);
+
+void
+CmdExec_Init(void)
+{
+ char *p;
+
+ for (p = "#=|^(){};&<>*?[]:$`\\\n~"; *p != '\0'; p++)
+ meta[(unsigned char) *p] = 1;
+ /* The null character serves as a sentinel in the string. */
+ meta[0] = 1;
+}
+
+static char **
+recheck_command_for_shell(char **av)
+{
+ char *runsh[] = {
+ "!", "alias", "cd", "eval", "exit", "read", "set", "ulimit",
+ "unalias", "unset", "wait", "umask", NULL
+ };
+
+ char **p;
+
+ /* optimization: if exec cmd, we avoid the intermediate shell */
+ if (strcmp(av[0], "exec") == 0)
+ av++;
+
+ if (!av[0])
+ return NULL;
+
+ for (p = runsh; *p; p++)
+ if (strcmp(av[0], *p) == 0)
+ return NULL;
+
+ return av;
+}
+
+void
+run_command(const char *cmd, bool errCheck)
+{
+ const char *p;
+ char *shargv[4];
+ char **todo;
+
+ shargv[0] = _PATH_BSHELL;
+
+ shargv[1] = errCheck ? "-ec" : "-c";
+ shargv[2] = (char *)cmd;
+ shargv[3] = NULL;
+
+ todo = shargv;
+
+
+ /* Search for meta characters in the command. If there are no meta
+* characters, there's no need to execute a shell to execute the
+* command. */
+ for (p = cmd; !meta[(unsigned char)*p]; p++)
+ continue;
+ if (*p == '\0') {
+ char *bp;
+ char **av;
+ int argc;
+ /* No meta-characters, so probably no need to exec a shell.
+* Break the command into words to form an argument vector
+* we can execute. */
+ av = brk_string(cmd, &argc, &bp);
+ av = recheck_command_for_shell(av);
+ if (av != NULL)
+ todo = av;
+ }
+ execvp(todo[0], todo);
+ if (errno == ENOENT)
+ fprintf(stderr, "%s: not found\n", todo[0]);
+ else
+ perror(todo[0]);
+ _exit(1);
+}
char *
Cmd_Exec(const char *cmd, char **err)
{
- char*args[4]; /* Args for invoking the shell */
int fds[2]; /* Pipe streams */
pid_t cpid; /* Child PID */
char*result;/* Result */
@@ -53,12 +138,6 @@ Cmd_Exec(const char *cmd, char **err)
*err = NULL;
- /* Set up arguments for the shell. */
- args[0] = "sh";
- args[1] = "-c";
- args[2] = (char *)cmd;
- args[3] = NULL;
-
/* Open a pipe for retrieving shell's output. */
if (pipe(fds) == -1) {
*err = "Couldn't create pipe for \"%s\"";
@@ -82,8 +161,7 @@ Cmd_Exec(const char *cmd, char **err)
(void)close(fds[1]);
}
- (void)execv(_PATH_BSHELL, args);
- _exit(1);
+ run_command(cmd, false);
/*NOTREACHED*/
case -1:
Index: cmd_exec.h
===
RCS file: /cvs/src/usr.bin/make/cmd_exec.h,v
retrieving revision 1.4
diff -u -p -r1.4 cmd_exec.h
--- cmd_exec.h 19 Jul 2010 19:46:43 - 1.4
+++ cmd_exec.h 28
PATCH: unify VAR!=cmd and other command execution
Turns out the exec in cmd_exec.c has absolutely zero reason to be different from what engine does. This small patch moves a bit of code around, so that all execv() consumers benefit from the same optimisation (namely, no extra shell when not needed). The only visible change should be that now, VAR!=cmd *will* display some relevant information if exec fails. This is a fairly trivial change, I don't expect any fallout. (still need to run it through tests)
Re: coping with large shell commands in make(1)
On Sat, Aug 26, 2023 at 02:25:19PM -0600, Theo de Raadt wrote: > I did not point you in this direction, not at all. Oh you did. It was not intentional. But I was focused on make variables instead of trying to catch the error, and you redirected me towards execv proper. > I said make execve failure report an accurate error. Rather than > requiring a special hook which will report the problem. Meaning > if the problem areas were known, they could be coped with. What > I'm seeing here appears to be a thing like system(), which will > punt the problem further along, to a shell, which will also probably > report the error poorly -- I suspect the more layers you pile on > top of ARG_MAX and/or E2BIG, the worse this will become. execve's failure DOES report an accurate error. We show the error message "Argument list too long" after the exec fails. and the engine itself is rather specific about the exact job that failed. (showing the target name and the location in the Makefile) For now, the locations where the problem occurs in Makefiles is because there are constructed shell commands which become too long. Invariably, the commands themselves loop over the long argument list, or use a builtin like echo. Now, the actual problem is languages like go and friends which have gigantic lists of distfiles. But making that go away cannot happen at our end. The current work we've done on reducing stuff a bit means we can probably last a few years before it overflows ARG_MAX, by which point we might have bumped ARG_MAX again.
coping with large shell commands in make(1)
Theo pointed me in another direction
Proof of concept that appears to work just fine
(very minimal error checking so far)
Of course, if we end up running a command on somethin too large,
thi will fail as usual, but much to my surprise,
make show=_FULL_FETCH_LIST
in sysutils/telegraf worked with this.
(and then I remembered that echo is a built-in, so there's no long exec
involved)
Index: cmd_exec.c
===
RCS file: /cvs/src/usr.bin/make/cmd_exec.c,v
retrieving revision 1.11
diff -u -p -r1.11 cmd_exec.c
--- cmd_exec.c 16 Jan 2020 16:07:18 - 1.11
+++ cmd_exec.c 26 Aug 2023 20:06:45 -
@@ -36,6 +36,7 @@
#include "memory.h"
#include "pathnames.h"
#include "job.h"
+#include "engine.h"
char *
Cmd_Exec(const char *cmd, char **err)
@@ -54,7 +55,7 @@ Cmd_Exec(const char *cmd, char **err)
*err = NULL;
/* Set up arguments for the shell. */
- args[0] = "sh";
+ args[0] = _PATH_BSHELL;
args[1] = "-c";
args[2] = (char *)cmd;
args[3] = NULL;
@@ -82,7 +83,9 @@ Cmd_Exec(const char *cmd, char **err)
(void)close(fds[1]);
}
- (void)execv(_PATH_BSHELL, args);
+ (void)execv(args[0], args);
+ if (errno == E2BIG)
+ retry_with_temp_file(false, args);
_exit(1);
/*NOTREACHED*/
Index: engine.c
===
RCS file: /cvs/src/usr.bin/make/engine.c,v
retrieving revision 1.71
diff -u -p -r1.71 engine.c
--- engine.c30 May 2023 04:42:21 - 1.71
+++ engine.c26 Aug 2023 20:06:45 -
@@ -549,6 +549,30 @@ recheck_command_for_shell(char **av)
return av;
}
+void
+retry_with_temp_file(bool errCheck, char **args)
+{
+ char template[] = "/tmp/shell.XX";
+ int fd;
+ char buf[50];
+ int i = 1;
+
+ fd = mkstemp(template);
+ snprintf(buf, sizeof(buf), "/dev/fd/%d", fd);
+
+ if (fd == -1)
+ return;
+ unlink(template);
+ write(fd, args[2], strlen(args[2]));
+ lseek(fd, 0, SEEK_SET);
+ if (errCheck)
+ args[i++] = "-e";
+ args[i++] = buf;
+ args[i++] = NULL;
+ execv(args[0], args);
+}
+
+
static void
run_command(const char *cmd, bool errCheck)
{
@@ -583,6 +607,8 @@ run_command(const char *cmd, bool errChe
todo = av;
}
execvp(todo[0], todo);
+ if (errno == E2BIG && todo == shargv)
+ retry_with_temp_file(errCheck, todo);
if (errno == ENOENT)
fprintf(stderr, "%s: not found\n", todo[0]);
Index: engine.h
===
RCS file: /cvs/src/usr.bin/make/engine.h,v
retrieving revision 1.17
diff -u -p -r1.17 engine.h
--- engine.h13 Jan 2020 15:12:58 - 1.17
+++ engine.h26 Aug 2023 20:06:45 -
@@ -138,4 +138,5 @@ extern bool job_run_next(Job *);
*/
extern void handle_job_status(Job *, int);
+extern void retry_with_temp_file(bool, char **);
#endif
POC: variable length in make
We've run into a few errors due to very long variable expansion in bsd.port.mk
that overflows execve(2)
I would consider adding a modifier to give the length of a variable, so
that significant tests can be done, along the lines of
.if ${VARIABLE:len} > 20
ERRORS += "Fatal: variable may overflow execve"
.endif
Now, this is a pure extension. We already have some highly similar modifier
in the presence of the very old SystemV :sh to exec variables,
and there's no ambiguity in there.
Would that feature be amenable for inclusion ?
(I'm aware I need to test asprintf's result, it's just a POC at the moment)
Index: varmodifiers.c
===
RCS file: /cvs/src/usr.bin/make/varmodifiers.c,v
retrieving revision 1.48
diff -u -p -r1.48 varmodifiers.c
--- varmodifiers.c 30 Aug 2020 12:16:04 - 1.48
+++ varmodifiers.c 22 Aug 2023 16:19:31 -
@@ -147,6 +147,8 @@ static void *check_empty(const char **,
static void *check_quote(const char **, SymTable *, bool, int);
static char *do_upper(const char *, const struct Name *, void *);
static char *do_lower(const char *, const struct Name *, void *);
+static void *check_length(const char **, SymTable *, bool, int);
+static char *do_length(const char *, const struct Name *, void *);
static void *check_shcmd(const char **, SymTable *, bool, int);
static char *do_shcmd(const char *, const struct Name *, void *);
static char *do_sort(const char *, const struct Name *, void *);
@@ -204,7 +206,8 @@ static struct modifier {
label_mod = {true, check_empty, do_label, NULL, NULL},
path_mod = {true, check_empty, do_path, NULL, NULL},
assign_mod = {true, assign_get_value, do_assign, NULL, free_patternarg},
- exec_mod = {true, get_cmd, do_exec, NULL, free_patternarg}
+ exec_mod = {true, get_cmd, do_exec, NULL, free_patternarg},
+ length_mod = {false, check_length, do_length, NULL, NULL}
;
void
@@ -219,6 +222,7 @@ VarModifiers_Init()
choose_mod['H'] = &head_mod;
choose_mod['E'] = &suffix_mod;
choose_mod['R'] = &root_mod;
+ choose_mod['l'] = &length_mod;
if (FEATURES(FEATURE_UPPERLOWER)) {
choose_mod['U'] = &upper_mod;
choose_mod['L'] = &lower_mod;
@@ -1202,6 +1206,26 @@ do_lower(const char *s, const struct Nam
for (i = 0; i < len; i++)
t[i] = TOLOWER(s[i]);
t[len] = '\0';
+ return t;
+}
+
+static void *
+check_length(const char **p, SymTable *ctxt UNUSED, bool b UNUSED, int endc)
+{
+ if ((*p)[1] == 'e' && (*p)[2] == 'n' &&
+ ((*p)[3] == endc || (*p)[3] == ':')) {
+ (*p)+=3;
+ return dummy_arg;
+ } else
+ return NULL;
+}
+
+static char *
+do_length(const char *s, const struct Name *n UNUSED, void *arg UNUSED)
+{
+ char*t;
+
+ asprintf(&t, "%zu", strlen(s));
return t;
}
Re: [diff] selectable curves in smtpd ?
On Sat, Aug 12, 2023 at 03:21:00PM +, gil...@poolp.org wrote: > August 12, 2023 4:34 PM, "Theo Buehler" wrote: > > > On Sat, Aug 12, 2023 at 02:29:45PM +, gil...@poolp.org wrote: > > > >> Hello, > >> > >> Someone asked about selectable curves in the OpenSMTPD portable tracker, > >> and it turns out I had a diff for that among a few others. > > > > Why do they need this? > > I suspect for the same reason people have needed ciphers selection in the > past, > being able to comply with the requirements of some certification (iirc, > medical > mail systems, for example, have strict requirements regarding their setup). > > Anyways, I've written this a long time ago and I'm providing it in case it's > of > any interest, feel free to discard. > This is moving *backwards* from best practices. Notice that TLS 1.3 did remove EC parameters choice, because this could lead to downgrade MIT attacks.
Re: Diff for evaluation (WACOM tablet driver)
On Sat, Aug 12, 2023 at 05:22:38PM +0200, Peter J. Philipp wrote: > On Sat, Aug 12, 2023 at 02:27:13PM +, Miod Vallat wrote: > > Third time's (hopefully) the charm. How about that diff? Too much things > > have been removed in uwacom. > > partial success! The wacom driver is recognized, no panics this time. But > the input is all over the place when I try to draw anything in gimp. It opens > windows and stuff that I didn't open. I see stuff like that, but that's actually an improvement here: ask xev, and you'll see some extra position information that's mapped to the 4th axis, which needs to be remmapped manually in gimp, as far as I know (for now)
Re: Diff for evaluation (WACOM tablet driver)
On Sat, Aug 12, 2023 at 08:00:48AM +, Miod Vallat wrote: > I have had a look at your diff and I think it's decent enough to go in > after some polishing. > > Can Wacom tablet users try this cleaned up diff? Works on the target tablet here (Wacom Intuos S)
plans for DISTFILES/MASTER_SITES
Now that we got .VARIABLES in make, this is going to be put to great use
in bsd.port.mk.
Namely, instead of the currently (very clunky)
DISTFILES = sometarball:0
MASTER_SITES0 = where to get this
I propose we do something like:
DISTFILES.site = sometarball
MASTER_SITES.site = where to get this
Pros:
- infinite number of alternate sites if need be
- less cumbersome syntax
Before .VARIABLES, there would need to be a way to list those suffixes,
but now, it's about as complicated as
${.VARIABLES:MDISTFILES*}
the bsd.port.mk are reasonably trivial. Repercussions to sqlports and dpb
need a bit more work (it's not complicated, it just needs to be dealt with
This ought to lift a lot of limitations eventually, along with thfr work
(still morphing thru lots of feedback)
Some of the basic support ought to hit the ports tree soon, the rest
requires make's .VARIABLES support to be generally available in snapshots.
Re: distexpand for autogenerated upstream distfile resources (was: standardize and simplify GitHub submodule handling in ports?)
On Wed, Aug 09, 2023 at 12:54:12AM -0400, Thomas Frohwein wrote: > - It includes logic that finds the first MASTER_SITESn that isn't > otherwise used, and throws an ERROR if it overruns past > MASTER_SITES9. That logic will hopefully be soon 100% obsolete. I need some okays on the .VARIABLES make patch. I have code to be able to use more or less arbitrary suffixes in MASTER_SITES in bsd.port.mk, and the corresponding stuff for sqlports and dpb ought to be fairly trivial.
Re: PATCH: a bit of introspection in make
Actually, as far as bsd.port.mk, it doesn't need to
move too much stuff around thanks to make's lazyness.
Note that having a list of defined MASTER_SITES variables simplifies
the check.
I've also added a check for the right MASTER_SITES to be defined,
since currently we do not error out until actually using it, which
means that fiddling around with MASTER_SITES before committing may
often go unnoticed.
(That final part is meant to go in sooner rather than later)
Index: bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1592
diff -u -p -r1.1592 bsd.port.mk
--- bsd.port.mk 13 Jun 2023 10:28:40 - 1.1592
+++ bsd.port.mk 8 Aug 2023 10:59:38 -
@@ -118,9 +118,8 @@ _ALL_VARIABLES_PER_ARCH =
# consumers of (dump-vars) include sqlports generation and dpb
# dpb doesn't need everything, those are speed optimizations
.if ${DPB:L:Mfetch} || ${DPB:L:Mall}
-_ALL_VARIABLES += DISTFILES PATCHFILES SUPDISTFILES DIST_SUBDIR MASTER_SITES \
- MASTER_SITES0 MASTER_SITES1 MASTER_SITES2 MASTER_SITES3 MASTER_SITES4 \
- MASTER_SITES5 MASTER_SITES6 MASTER_SITES7 MASTER_SITES8 MASTER_SITES9 \
+_ALL_VARIABLES += DISTFILES PATCHFILES SUPDISTFILES DIST_SUBDIR \
+ ${_ALL_MASTER_SITES} \
CHECKSUM_FILE FETCH_MANUALLY MISSING_FILES PERMIT_DISTFILES
.endif
.if ${DPB:L:Mtest} || ${DPB:L:Mall}
@@ -1280,19 +1280,15 @@ MASTER_SITES ?=
# sites for distfiles, add them to MASTER_SITE_BACKUP
_warn_checksum = :
-.if !empty(MASTER_SITES:M*[^/])
-_warn_checksum += ;echo ">>> MASTER_SITES not ending in /:
${MASTER_SITES:M*[^/]}"
-.endif
-.for _I in 0 1 2 3 4 5 6 7 8 9
-. if defined(MASTER_SITES${_I})
-.if !empty(MASTER_SITES${_I}:M*[^/])
-_warn_checksum += ;echo ">>> MASTER_SITES${_I} not ending in /:
${MASTER_SITES${_I}:M*[^/]}"
-.endif
+_ALL_MASTER_SITES = ${.VARIABLES:MMASTER_SITES*:NMASTER_SITES_*}
+
+.for _S in ${_ALL_MASTER_SITES}
+. if !empty(${_S}:M*[^/])
+_warn_checksum += ;echo ">>> ${_S} not ending in /: ${${_S}:M*[^/]}"
. endif
.endfor
-
EXTRACT_SUFX ?= .tar.gz
.if !empty(GH_COMMIT)
@@ -1322,6 +1318,9 @@ _FILES=
. if !empty($v)
.for e in ${$v}
. for f m u in ${e:C/:[0-9]$//:C/^(.*)\{.*\}(.*)$/\1\2/}
MASTER_SITES${e:M*\:[0-9]:C/^.*:([0-9])$/\1/}
${e:C/:[0-9]$//:C/^.*\{(.*)\}(.*)$/\1\2/}
+.if !defined($m)
+ERRORS += "Fatal: $m is not defined but referenced by $e in $v"
+.endif
.if empty(_FILES:M$f)
_FILES += $f
. if empty(DIST_SUBDIR)
Re: PATCH: a bit of introspection in make
Here's a revised diff (reordered plus actual use of the boolean)
plus concrete example use for bsd.port.mk (disregarding the fact
_ALL_VARIABLES would have to move *after* all MASTER_SITES have been defined.
Index: var.c
===
RCS file: /cvs/src/usr.bin/make/var.c,v
retrieving revision 1.104
diff -u -p -r1.104 var.c
--- var.c 9 Jun 2022 13:13:14 - 1.104
+++ var.c 8 Aug 2023 10:48:05 -
@@ -104,6 +104,8 @@ static char varNoError[] = "";
bool errorIsOkay;
static boolcheckEnvFirst; /* true if environment should be searched for
* variables before the global context */
+ /* do we need to recompute varname_list */
+static boolvarname_list_changed = true;
void
Var_setCheckEnvFirst(bool yes)
@@ -222,6 +224,7 @@ typedef struct Var_ {
#define VAR_FROM_ENV 8 /* Special source: environment */
#define VAR_SEEN_ENV 16 /* No need to go look up environment again */
#define VAR_IS_SHELL 32 /* Magic behavior */
+#define VAR_IS_NAMES 1024/* Very expensive, only defined when needed */
/* XXX there are also some flag values which are part of the visible API
* and thus defined inside var.h, don't forget to look there if you want
* to define some new flags !
@@ -231,6 +234,8 @@ typedef struct Var_ {
char name[1]; /* the variable's name */
} Var;
+/* for GNU make compatibility */
+#define VARNAME_LIST ".VARIABLES"
static struct ohash_info var_info = {
offsetof(Var, name),
@@ -245,10 +250,11 @@ static void fill_from_env(Var *);
static Var *create_var(const char *, const char *);
static void var_set_initial_value(Var *, const char *);
static void var_set_value(Var *, const char *);
-#define var_get_value(v) ((v)->flags & VAR_EXEC_LATER ? \
- var_exec_cmd(v) : \
- Buf_Retrieve(&((v)->val)))
-static char *var_exec_cmd(Var *);
+static char *var_get_value(Var *);
+static void var_exec_cmd(Var *);
+static void varname_list_retrieve(Var *);
+
+
static void var_append_value(Var *, const char *);
static void poison_check(Var *);
static void var_set_append(const char *, const char *, const char *, int,
bool);
@@ -423,6 +429,7 @@ var_set_initial_value(Var *v, const char
len = strlen(val);
Buf_Init(&(v->val), len+1);
Buf_AddChars(&(v->val), len, val);
+ varname_list_changed = true;
}
/* Normal version of var_set_value(), to be called after variable is fully
@@ -440,6 +447,16 @@ var_set_value(Var *v, const char *val)
}
}
+static char *
+var_get_value(Var *v)
+{
+ if (v->flags & VAR_IS_NAMES)
+ varname_list_retrieve(v);
+ else if (v->flags & VAR_EXEC_LATER)
+ var_exec_cmd(v);
+ return Buf_Retrieve(&(v->val));
+}
+
/* Add to a variable, insert a separating space if the variable was already
* defined.
*/
@@ -628,6 +645,7 @@ Var_Deletei(const char *name, const char
ohash_remove(&global_variables, slot);
delete_var(v);
+ varname_list_changed = true;
}
/* Set or add a global variable, either to VAR_CMD or VAR_GLOBAL.
@@ -687,7 +705,7 @@ Var_Appendi_with_ctxt(const char *name,
var_set_append(name, ename, val, ctxt, true);
}
-static char *
+static void
var_exec_cmd(Var *v)
{
char *arg = Buf_Retrieve(&(v->val));
@@ -699,7 +717,30 @@ var_exec_cmd(Var *v)
var_set_value(v, res1);
free(res1);
v->flags &= ~VAR_EXEC_LATER;
- return Buf_Retrieve(&(v->val));
+}
+
+static void
+varname_list_retrieve(Var *v)
+{
+ unsigned int i;
+ void *e;
+ bool first = true;
+
+ if (!varname_list_changed)
+ return;
+ for (e = ohash_first(&global_variables, &i); e != NULL;
+ e = ohash_next(&global_variables, &i)) {
+ Var *v2 = e;
+ if (v2->flags & VAR_DUMMY)
+ continue;
+
+ if (first)
+ var_set_value(v, v2->name);
+ else
+ var_append_value(v, v2->name);
+ first = false;
+ }
+ varname_list_changed = false;
}
/* XXX different semantics for Var_Valuei() and Var_Definedi():
@@ -1339,6 +1380,19 @@ set_magic_shell_variable()
v->flags = VAR_IS_SHELL | VAR_SEEN_ENV;
}
+static void
+set_magic_name_list_variable()
+{
+ const char *name = VARNAME_LIST;
+ const char *ename = NULL;
+ uint32_t k;
+ Var *v;
+
+ k = ohash_interval(name, &ename);
+ v = find_global_var_without_env(name, ename, k);
+ var_set_initial_value(v, "");
+ v->flags = VAR_IS_NAMES;
+}
/*
* Var_Init
* Initialize the module
@@ -1348,11 +1402,10 @@ Var_Init(void)
{
ohash_init(&global_variables, 10, &var_info);
set_magic_shell_variable();
-
+ set_magic_name_list_variable();
errorIsOkay
Re: PATCH: a bit of introspection in make
On Mon, Aug 07, 2023 at 10:05:37PM -0400, Thomas Frohwein wrote:
> I looked through the file and it seems that varname_list_changed is
> never set to anything but true. Is there a bit missing, like down lower
> in varname_list_retrieve()?
Yep, I removed it at some point because it looked like some extra
unneeded work, and then I did change my mind because actual patterns
require it several times. So, yeah, it should be set to false there.
As for patterns, well, it will allow to write loops like
.for v in ${.VARIABLES:MMASTER_SITES*}
instead of having to look at MASTER_SITES0...9 manually, just as a for
instance.
PATCH: a bit of introspection in make
I think it could be occasionally useful to know which variables have
been defined in make.
Incidentally, this DOES exist in GNU make, so I've reused the same name
for basically the same functionality.
I haven't checked whether NetBSD/FreeBSD introduced something similar.
This is a fairly straightforward patch, introduces .VARIABLES corresponding
to the full list of global variables (from the command line and the Makefile)
that have been defined.
(^ says the guy who had to remember a few details from his own(!) var.c
implementation from a few years ago)
I just took var_get_value offline from the old macro, for readability,
even though it's likely the compiler may still decide to inline it.
For efficiency, that list is only computed as needed, since it is
somewhat long.
For debugging purposes, this can come in fairly handy, and I see at
least another application in ports.
Comments and nits welcome.
Note that the list is completely unsorted. This could be sorted through
since we already have the code for dumping purposes, but it would be even
more expensive (the order will be "random", as per the hash)
Index: var.c
===
RCS file: /cvs/src/usr.bin/make/var.c,v
retrieving revision 1.104
diff -u -p -r1.104 var.c
--- var.c 9 Jun 2022 13:13:14 - 1.104
+++ var.c 7 Aug 2023 14:33:42 -
@@ -104,6 +104,8 @@ static char varNoError[] = "";
bool errorIsOkay;
static boolcheckEnvFirst; /* true if environment should be searched for
* variables before the global context */
+ /* do we need to recompute varname_list */
+static boolvarname_list_changed = true;
void
Var_setCheckEnvFirst(bool yes)
@@ -228,9 +230,12 @@ typedef struct Var_ {
*/
#define POISONS (POISON_NORMAL | POISON_EMPTY | POISON_NOT_DEFINED)
/* Defined in var.h */
+#define VAR_IS_NAMES 1024/* Very expensive, only defined when needed */
char name[1]; /* the variable's name */
} Var;
+/* for GNU make compatibility */
+#define VARNAME_LIST ".VARIABLES"
static struct ohash_info var_info = {
offsetof(Var, name),
@@ -245,10 +250,11 @@ static void fill_from_env(Var *);
static Var *create_var(const char *, const char *);
static void var_set_initial_value(Var *, const char *);
static void var_set_value(Var *, const char *);
-#define var_get_value(v) ((v)->flags & VAR_EXEC_LATER ? \
- var_exec_cmd(v) : \
- Buf_Retrieve(&((v)->val)))
-static char *var_exec_cmd(Var *);
+static char *var_get_value(Var *);
+static void var_exec_cmd(Var *);
+static void varname_list_retrieve(Var *);
+
+
static void var_append_value(Var *, const char *);
static void poison_check(Var *);
static void var_set_append(const char *, const char *, const char *, int,
bool);
@@ -423,6 +429,7 @@ var_set_initial_value(Var *v, const char
len = strlen(val);
Buf_Init(&(v->val), len+1);
Buf_AddChars(&(v->val), len, val);
+ varname_list_changed = true;
}
/* Normal version of var_set_value(), to be called after variable is fully
@@ -440,6 +447,16 @@ var_set_value(Var *v, const char *val)
}
}
+static char *
+var_get_value(Var *v)
+{
+ if (v->flags & VAR_IS_NAMES)
+ varname_list_retrieve(v);
+ else if (v->flags & VAR_EXEC_LATER)
+ var_exec_cmd(v);
+ return Buf_Retrieve(&(v->val));
+}
+
/* Add to a variable, insert a separating space if the variable was already
* defined.
*/
@@ -628,6 +645,7 @@ Var_Deletei(const char *name, const char
ohash_remove(&global_variables, slot);
delete_var(v);
+ varname_list_changed = true;
}
/* Set or add a global variable, either to VAR_CMD or VAR_GLOBAL.
@@ -687,7 +705,7 @@ Var_Appendi_with_ctxt(const char *name,
var_set_append(name, ename, val, ctxt, true);
}
-static char *
+static void
var_exec_cmd(Var *v)
{
char *arg = Buf_Retrieve(&(v->val));
@@ -699,7 +717,27 @@ var_exec_cmd(Var *v)
var_set_value(v, res1);
free(res1);
v->flags &= ~VAR_EXEC_LATER;
- return Buf_Retrieve(&(v->val));
+}
+
+static void
+varname_list_retrieve(Var *v)
+{
+ unsigned int i;
+ void *e;
+ bool first = true;
+
+ for (e = ohash_first(&global_variables, &i); e != NULL;
+ e = ohash_next(&global_variables, &i)) {
+ Var *v2 = e;
+ if (v2->flags & VAR_DUMMY)
+ continue;
+
+ if (first)
+ var_set_value(v, v2->name);
+ else
+ var_append_value(v, v2->name);
+ first = false;
+ }
}
/* XXX different semantics for Var_Valuei() and Var_Definedi():
@@ -1339,6 +1377,19 @@ set_magic_shell_variable()
v->flags = VAR_IS_SHELL | VAR_SEEN_ENV;
}
+static void
+set_magic_name_list_varia
Re: standardize and simplify GitHub submodule handling in ports?
On Sat, Aug 05, 2023 at 09:50:57PM -0400, Thomas Frohwein wrote:
> On Sat, Aug 05, 2023 at 11:27:24PM +0200, Marc Espie wrote:
> > Some comments already. I haven't looked very closely.
>
> > On Sat, Aug 05, 2023 at 03:12:18PM -0400, Thomas Frohwein wrote:
> > > The current draft hijacks post-extract target, but it would be easy to
> > > add this to _post-extract-finalize in bsd.port.mk similar to how the
> > > post-extract commands from modules are handled, if this is of interest.
> >
> > Please do that.
>
> Thanks, I updated the draft. Realized that including it with
> MODULES=github is easiest and then this can just use
> MODGITHUB_post-extract and doesn't need any custom code in bsd.port.mk.
> I had a thinko in post-extract (needs to be '||', not '&&') which is
> also corrected.
>
> > > # where submodule distfiles will be stored
> > > GHSM_DIST_SUBDIR ?= gh-submodules
> >
> > Please keep to the GH_* subspace.
> >
> > Already, modules usually use MOD* variable names, but in the GH case, that
> > would be a bit long.
>
> I renamed GHSM_* to GH_*. I wouldn't mind using MODGH_* if that's an
> option, but MODGITHUB_* would be pretty unwieldy, especially if we were
> to make the existing GH_{ACCOUNT,PROJECT,TAGNAME} etc. part of this.
>
> [...]
>
> > Please do a single loop. That's slightly more readable for me.
>
> yes, done.
>
> [...]
>
> > Also please draft a diff for port-modules(5)
>
> I'm attaching a diff for port-modules.5, along with the updated
> github.port.mk.
>
> > I'm also wondering if keeping the main GH_* stuff in bsd.port.mk makes a lot
> > of sense, instead of grouping everything in github.port.mk
>
> I'm for it, maybe as a second step after the module for just the
> submodule handling is done because there would be a lot of ports churn
> with moving the main GH_* stuff out of bsd.port.mk.
Probably not. We have a few "big" modules that don't depend on explicitly
adding to modules, but on some variable triggering it (historic imake stuff
or configure), having github.port.mk brought in from one of the GH_* variables
(probably don't need to test them all) would be acceptable.
Re: standardize and simplify GitHub submodule handling in ports?
Some comments already. I haven't looked very closely.
On Sat, Aug 05, 2023 at 03:12:18PM -0400, Thomas Frohwein wrote:
> The current draft hijacks post-extract target, but it would be easy to
> add this to _post-extract-finalize in bsd.port.mk similar to how the
> post-extract commands from modules are handled, if this is of interest.
Please do that.
> # where submodule distfiles will be stored
> GHSM_DIST_SUBDIR ?= gh-submodules
Please keep to the GH_* subspace.
Already, modules usually use MOD* variable names, but in the GH case, that
would be a bit long.
> .for _ghaccount _ghproject _ghtagcommit _targetdir in ${GH_SUBMODULES}
> DISTFILES +=
> ${GHSM_DIST_SUBDIR}/{}${_ghaccount}/${_ghproject}/archive/${_ghtagcommit}${GH_SUFX}:${GHSM_MASTER_SITESN}
> .endfor
>
> # post-extract target for moving the submodules to the target directories
> GHSM_post-extract =
> .for _ghaccount _ghproject _ghtagcommit _targetdir in ${GH_SUBMODULES}
> GHSM_post-extract += \
> test -d ${GHSM_WRKSR}/${_targetdir} || rm -rf
> ${GHSM_WRKSRC}/${_targetdir} ; \
That line is weird.
> mv ${WRKDIR}/${_ghproject}-${_ghtagcommit} ${GHSM_WRKSRC}/${_targetdir}
> ;
> .endfor
Please do a single loop. That's slightly more readable for me.
> # XXX: would best belong in _post-extract-finalize in bsd.port.mk rather than
> # hijacking post-extract here
> post-extract:
> @${ECHO_MSG} "moving GitHub submodules to ${GHSM_WRKSRC}" ;
> mkdir -p ${GHSM_WRKSRC} ;
> ${GHSM_post-extract}
Also please draft a diff for port-modules(5)
I'm also wondering if keeping the main GH_* stuff in bsd.port.mk makes a lot
of sense, instead of grouping everything in github.port.mk
Re: Stop using direct syscall(2) from perl(1)
On Sun, Jul 09, 2023 at 01:29:58PM -0700, Andrew Hewus Fresh wrote:
> Here is a patch to replace perl(1)'s use of syscall(2) with a dispatcher
> that will call the libc function instead.
>
> I have to do some work on style before this is ready to commit, but it
> should be ready for some testing.
>
> I don't currently plan on committing syscall_emulator.c because we need
> to regenerate it each time as I'm not sure how to tie it into sys/kern's
> `make syscalls` to only do it when things change.
>
> Looking for tests from folks who use syscall from perl as well as style
> suggestions (like how do I correctly sort headers?) and maybe even
> ways to enable additional syscalls.
Nits in the perl part.
> Index: gnu/usr.bin/perl/gen_syscall_emulator.pl
> ===
> RCS file: gnu/usr.bin/perl/gen_syscall_emulator.pl
> diff -N gnu/usr.bin/perl/gen_syscall_emulator.pl
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ gnu/usr.bin/perl/gen_syscall_emulator.pl 9 Jul 2023 19:42:50 -
> @@ -0,0 +1,354 @@
> +#!/usr/bin/perl
> +#$OpenBSD$ #
> +use v5.36;
> +
> +# Copyright (c) 2023 Andrew Hewus Fresh
> +#
> +# Permission to use, copy, modify, and distribute this software for any
> +# purpose with or without fee is hereby granted, provided that the above
> +# copyright notice and this permission notice appear in all copies.
> +#
> +# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> +# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> +# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> +# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> +# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> +# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> +# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> +
> +my $includes = '/usr/include';
> +
> +# See also /usr/src/sys/kern/syscalls.master
> +my %syscalls = parse_syscalls(
> +"$includes/sys/syscall.h",
> +"$includes/sys/syscallargs.h",
> +);
> +delete $syscalls{MAXSYSCALL}; # not an actual function
> +
> +# The ordered list of all the headers we need
> +my @headers = qw<
> + sys/syscall.h
> + stdarg.h
> + errno.h
> +
> + sys/socket.h
> + sys/event.h
> + sys/futex.h
> + sys/ioctl.h
> + sys/ktrace.h
> + sys/mman.h
> + sys/mount.h
> + sys/msg.h
> + sys/poll.h
> + sys/ptrace.h
> + sys/resource.h
> + sys/select.h
> + sys/sem.h
> + sys/shm.h
> + sys/stat.h
> + sys/sysctl.h
> + sys/time.h
> + sys/uio.h
> + sys/wait.h
> +
> + dirent.h
> + fcntl.h
> + sched.h
> + signal.h
> + stdlib.h
> + stdio.h
> + syslog.h
> + tib.h
> + time.h
> + unistd.h
> +>;
> +
> +foreach my $header (@headers) {
> + my $file = "$includes/$header";
> + open my $fh, '<', $file or die "Unable to open $file: $!";
you could just autodie as you don't ever do anything fancy when open fails.
> + my $content = do { local $/; readline $fh };
> + close $fh;
> +
> + # Look for matching syscalls in this header
not sure that comment brings anything, or maybe you want a function, as
you tend to get fairly long winded.
> + foreach my $name (sort keys %syscalls) {
> + my $s = $syscalls{$name};
> + my $func_sig = find_func_sig($content, $name, $s);
> +
> + if (ref $func_sig) {
> + die "Multiple defs for $name <$header> <$s->{header}>"
> + if $s->{header};
> + $s->{func} = $func_sig;
> + $s->{header} = $header;
> + } elsif ($func_sig) {
> + $s->{mismatched_sig} = "$func_sig <$header>";
> + }
> + }
> +}
> +
> +say "/*\n * Generated from gen_syscall_emulator.pl\n */";
> +say "#include <$_>" for @headers;
> +print <<"EOL";
> +
> +long
> +syscall_emulator(int syscall, ...)
> +{
> + long ret = 0;
> + va_list args;
> + va_start(args, syscall);
> +
> + switch(syscall) {
> +EOL
> +
> +foreach my $name (
> + sort { $syscalls{$a}{id} <=> $syscalls{$b}{id} } keys %syscalls
> +) {
> + my %s = %{ $syscalls{$name} };
I never put spaces in this kind of construct, as I find it fairly readable
by itself
> +
> + # Some syscalls we can't emulate, so we comment those out.
> + $s{skip} //= "Indirect syscalls not supported"
> + if !$s{argtypes} && ($s{args}[-1] || '') eq '...';
> + $s{skip} //= "Mismatched func: $s{mismatched_sig}"
> + if $s{mismatched_sig} and not $s{func};
> + $s{skip} //= "No signature found in headers"
> + unless $s{header};
> +
> + my $ret = $s{ret} eq 'void' ? '' : 'ret = ';
> + $ret .= '(long)' if $s{ret} eq 'void *';
> +
> + my (@args, @defines);
> + my $argname = '';
> + if ($s{argtypes}) {
> +
Re: OpenBSD::MkTemp vs Devel::Cover
Tl;Dr: I'm not trying too hard to use Devel::Cover but so far it fails abysmally with BOTH pkg_add and dpb. I'll admit that both are playing "fun" games with identity but seriously, Devel::NYTProf didn't care at all ! I've spent a few hours trying to figure out options (loose_perms won't help) Knowing how much of my perl is actually used would be a HUGE HELP in crafting more non regression tests, but so far, Devel::Cover lives up to its "alpha for 20 years" reputation. Triggering the problems is not hard: pkg_add breaks, dpb breaks even earlier...
pkg_*: the road forward
I spent some time during the last hackathon, talking to various people over where we're going. I'm a bit afraid that people are going to see the ports/package framework as "not invited" (because some of the subsystems of OpenBSD are not exactly oustide friendly) Right now, I got one student directly working with me on the tools, and I must say it is a good thing: I have wy too many idiosyncrasies. And I'm trying to get better at documenting ?` So where are we going ? there are lots of pieces missing, some of them trivial, some really annoying. - the level of testing is abysmal. I would really like for someone to have fun with regress and make it manageable. Right now, it's a pain ! More tooling to make pkg_add "fully" regression testable would be very helpful. - likewise register-plist needs some tests. I have made some progress in options to make that happen. - I would love to get some Devel::Cover stats... zero progress about this during this week (well, the main progress was that it does not work) - I converted ALL my code to perl 5.36. Having signatures is a HUGE change, not wrt actual code, but with readability. It makes perl code look like "other" programming languages, in a good way, thus making it sooo much easier for new people to get in. As for the actual code changes: - we need more code to deal with "global state", mainly /etc/rc changes and messages. Those files are going to move around, which means it's going to be GLOBAL state, because it can move from package to package. I would really really love to have regress test cases first. - we still need better Vstat code that deals with symlinks. There's one test that actually fails... and writing more tests should be simpler. - the code dealing with "old libs" is still incredibly ad-hock and needing some love. - there's something fishy in pkg_create wrt dependency handling... figure it out, AND make it work with tags as well. - fragments building is oversimplified, that's the main thing holding back ocaml "smart fragments". - texlive wants some love. That one is pretty much mine: a bit of targets in bsd.port.mk, a bit of "hints" code in update-plist, and also the ability to say "oh, don't package this, it's already in". Nothing really difficult, it's just that texlive is large... - displays and codes from pkg_add/pkg_info. The main culprit right now is FETCH_PACKAGES, it could be better. - "libsets". Updating wantlibs is really a pain for big changes. I have a keyword called libset to deal with that. It requires changes to bsd.port.mk and to pkg_add. Simply put: a libset would have a version and say "we need those libs to work". If a libset got bumped, then we wouldn't say the affected libs are a problem if they change ! This is mostly a question of writing the code. - lib-depends-check: the script AND its dependent libraries need a lot of clean-up, so that an average person can understand them. Then we can deal (probably) with subdirectories (which we don't) - dpb changes: the "files needed" change. I have all the pieces to figure out which files and directories a given build requires: in the ports tree, MAKEFILES_LIST + PKGDIR + PATCHDIR + FILESDIR + package + distfiles would be enough. I need to actually provide this. This would enable TWO huge things: -> disconnected builds, with a cluster that doesn't use nfs, but runs openrsync instead -> separate builds into separate chroots (roughly 6000+ links per build) - regress tests: should be much easier now that diskspace is (mostly) not an issue - roaching while fetching: the main part would be to figure out which part of roach we actually need to plug in after fetch. Having a sqlite database is actually a huge hindrance. dpb could output "decent" logs, and it should be easy to coerce them. I probably missed some entries, but this is what occupies my ports/pkg* thoughts. Help welcome. Not perl solutions NOT welcome.
Re: Icky test patches for Devel::Cover and pkg_add
Somehow, it's worse than I thought. Turns out something in
Devel::Cover is interfering with normal behavior.
I get very low coverage numbers, because basically pkg_add can't even
get to the end of installing a package.
I'm wondering if anyone actually manages to use Devel::Cover for non
trivial real worlds problems :(
On Sat, Jul 8, 2023 at 11:27 AM Marc Espie wrote:
>
> Contrary to the NYT profiler, which works like a charm
> Devel::Cover is a tricky beast.
>
> Right now, I've isolated two issues:
> - OpenBSD::MkTemp (subject to a separate mail)
> - identity changes on the fly. Even with looseperms, Devel::Cover
> doesn't cope too well with multiple processes and multiple identities
>
> Leading to hilarious errors like:
>
> http://cdn.openbsd.org/pub/OpenBSD/snapshots/packages/amd64/: Devel::Cover:
> Oops, it looks like something went wrong writing the coverage.
> It's possible that more bad things may happen but we'll try to
> carry on anyway as if nothing happened. At a minimum you'll
> probably find that you are missing coverage. If you're
> interested, the problem was:
>
> Can't open /tmp/cover_db/structure/177bda0715eb6b804bab8afc36107d32.lock:
> Permission denied
>
>
>
> So I have the following fairly icky patch in my tree to be able to try
> and get some coverage data and keep going
>
> (note that the dynamic call to OpenBSD::MkTemp in pkg_create is probably
> not affected since it gets run only with DEPENDS_CACHE set)
>
>
> I consider dpb to be somewhat hopeless for now in its standard privsep'd
> mode. I've tried it, and not only does Devel::Cover break, but dpb itself
> gets confused with getpwuid somehow not even finding its own parameters.
>
> (update-plist is going to be a bit annoying as well, but probably not too
> much as identity change can be managed as long as we don't try to write
> the lists)
>
>
> Anyways, as I was saying to kn@, I want to make some kind of progress towards
> figuring out pkg_* code coverage, and probably adding some regression tests.
>
> So, in case someone else wants to play:
>
> Index: OpenBSD/PackageRepository.pm
> ===
> RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/PackageRepository.pm,v
> retrieving revision 1.176
> diff -u -p -r1.176 PackageRepository.pm
> --- OpenBSD/PackageRepository.pm13 Jun 2023 09:07:17 - 1.176
> +++ OpenBSD/PackageRepository.pm8 Jul 2023 09:21:42 -
> @@ -753,13 +753,13 @@ sub ftp_cmd($self)
> sub drop_privileges_and_setup_env($self)
> {
> my ($uid, $gid, $user) = $self->fetch_id;
> - if (defined $uid) {
> - # we happen right before exec, so change id permanently
> - $( = $gid;
> - $) = "$gid $gid";
> - $< = $uid;
> - $> = $uid;
> - }
> +# if (defined $uid) {
> +# # we happen right before exec, so change id permanently
> +# $( = $gid;
> +# $) = "$gid $gid";
> +# $< = $uid;
> +# $> = $uid;
> +# }
> # create sanitized env for ftp
> my %newenv = (
> HOME => '/var/empty',
> Index: OpenBSD/Temp.pm
> ===
> RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/Temp.pm,v
> retrieving revision 1.39
> diff -u -p -r1.39 Temp.pm
> --- OpenBSD/Temp.pm 13 Jun 2023 09:07:17 - 1.39
> +++ OpenBSD/Temp.pm 8 Jul 2023 09:21:42 -
> @@ -19,7 +19,7 @@ use v5.36;
>
> package OpenBSD::Temp;
>
> -use OpenBSD::MkTemp;
> +use File::Temp;
> use OpenBSD::Paths;
> use OpenBSD::Error;
>
> @@ -112,7 +112,7 @@ sub permanent_file($dir, $stem)
> if (defined $dir) {
> $template = "$dir/$template";
> }
> - if (my @l = OpenBSD::MkTemp::mkstemp($template)) {
> + if (my @l = File::Temp::tempfile($template)) {
> return @l;
> }
> ($lastname, $lasttype, $lasterror) = ($template, 'file', $!);
> @@ -125,7 +125,7 @@ sub permanent_dir($dir, $stem)
> if (defined $dir) {
> $template = "$dir/$template";
> }
> - if (my $d = OpenBSD::MkTemp::mkdtemp($template)) {
> + if (my $d = File::Temp::tempdir($template)) {
> return $d;
> }
> ($lastname, $lasttype, $lasterror) = ($template, 'dir', $!);
>
Icky test patches for Devel::Cover and pkg_add
Contrary to the NYT profiler, which works like a charm
Devel::Cover is a tricky beast.
Right now, I've isolated two issues:
- OpenBSD::MkTemp (subject to a separate mail)
- identity changes on the fly. Even with looseperms, Devel::Cover
doesn't cope too well with multiple processes and multiple identities
Leading to hilarious errors like:
http://cdn.openbsd.org/pub/OpenBSD/snapshots/packages/amd64/: Devel::Cover:
Oops, it looks like something went wrong writing the coverage.
It's possible that more bad things may happen but we'll try to
carry on anyway as if nothing happened. At a minimum you'll
probably find that you are missing coverage. If you're
interested, the problem was:
Can't open /tmp/cover_db/structure/177bda0715eb6b804bab8afc36107d32.lock:
Permission denied
So I have the following fairly icky patch in my tree to be able to try
and get some coverage data and keep going
(note that the dynamic call to OpenBSD::MkTemp in pkg_create is probably
not affected since it gets run only with DEPENDS_CACHE set)
I consider dpb to be somewhat hopeless for now in its standard privsep'd
mode. I've tried it, and not only does Devel::Cover break, but dpb itself
gets confused with getpwuid somehow not even finding its own parameters.
(update-plist is going to be a bit annoying as well, but probably not too
much as identity change can be managed as long as we don't try to write
the lists)
Anyways, as I was saying to kn@, I want to make some kind of progress towards
figuring out pkg_* code coverage, and probably adding some regression tests.
So, in case someone else wants to play:
Index: OpenBSD/PackageRepository.pm
===
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/PackageRepository.pm,v
retrieving revision 1.176
diff -u -p -r1.176 PackageRepository.pm
--- OpenBSD/PackageRepository.pm13 Jun 2023 09:07:17 - 1.176
+++ OpenBSD/PackageRepository.pm8 Jul 2023 09:21:42 -
@@ -753,13 +753,13 @@ sub ftp_cmd($self)
sub drop_privileges_and_setup_env($self)
{
my ($uid, $gid, $user) = $self->fetch_id;
- if (defined $uid) {
- # we happen right before exec, so change id permanently
- $( = $gid;
- $) = "$gid $gid";
- $< = $uid;
- $> = $uid;
- }
+# if (defined $uid) {
+# # we happen right before exec, so change id permanently
+# $( = $gid;
+# $) = "$gid $gid";
+# $< = $uid;
+# $> = $uid;
+# }
# create sanitized env for ftp
my %newenv = (
HOME => '/var/empty',
Index: OpenBSD/Temp.pm
===
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/Temp.pm,v
retrieving revision 1.39
diff -u -p -r1.39 Temp.pm
--- OpenBSD/Temp.pm 13 Jun 2023 09:07:17 - 1.39
+++ OpenBSD/Temp.pm 8 Jul 2023 09:21:42 -
@@ -19,7 +19,7 @@ use v5.36;
package OpenBSD::Temp;
-use OpenBSD::MkTemp;
+use File::Temp;
use OpenBSD::Paths;
use OpenBSD::Error;
@@ -112,7 +112,7 @@ sub permanent_file($dir, $stem)
if (defined $dir) {
$template = "$dir/$template";
}
- if (my @l = OpenBSD::MkTemp::mkstemp($template)) {
+ if (my @l = File::Temp::tempfile($template)) {
return @l;
}
($lastname, $lasttype, $lasterror) = ($template, 'file', $!);
@@ -125,7 +125,7 @@ sub permanent_dir($dir, $stem)
if (defined $dir) {
$template = "$dir/$template";
}
- if (my $d = OpenBSD::MkTemp::mkdtemp($template)) {
+ if (my $d = File::Temp::tempdir($template)) {
return $d;
}
($lastname, $lasttype, $lasterror) = ($template, 'dir', $!);
OpenBSD::MkTemp vs Devel::Cover
Hey, Philip, you wrote this a long time ago. Now, I'm trying to get some coverage out of Devel::Cover on pkg_add, and somehow, it gets in the way. # perl -MDevel::Cover=+select,OpenBSD/.* /usr/sbin/pkg_add random_run Selecting packages matching: OpenBSD/.* Ignoring packages matching: /Devel/Cover[./] Ignoring packages in: /usr/local/libdata/perl5/site_perl/amd64-openbsd /usr/local/libdata/perl5/site_perl /usr/libdata/perl5/amd64-openbsd /usr/libdata/perl5 Couldn't install random_run-2.15 Devel::Cover: getting BEGIN block coverage Devel::Cover: 100% - 1s taken Devel::Cover: getting CHECK block coverage Devel::Cover: 100% - 0s taken Devel::Cover: getting END/INIT block coverage Devel::Cover: 100% - 0s taken Devel::Cover: getting CV coverage Devel::Cover: 9% Argument "*OpenBSD::MkTemp::_GEN_1" isn't numeric in addition (+) at /usr/local/libdata/perl5/site_perl/amd64-openbsd/Devel/Cover.pm line 852. Devel::Cover: 49% Argument "*OpenBSD::MkTemp::_GEN_2" isn't numeric in scalar assignment at /usr/local/libdata/perl5/site_perl/amd64-openbsd/Devel/Cover.pm line 900. Devel::Cover: 100% - 3s taken Devel::Cover: Writing coverage database to /home/espie/bin/cover_db/runs/1688807352.41650.28355 Devel::Cover: Oops, it looks like something went wrong writing the coverage. It's possible that more bad things may happen but we'll try to carry on anyway as if nothing happened. At a minimum you'll probably find that you are missing coverage. If you're interested, the problem was: Found type 9 GLOB(0x64a7292fc10), but it is not representable by the Sereal encoding format at /usr/local/libdata/perl5/site_perl/amd64-openbsd/Devel/Cover/DB/IO/Sereal.pm line 46. I have zero idea if Devel::Cover is to blame or if OpenBSD::MkTemp is missing some magic annotation to cover for _GEN_1, _GEN_2, but the end result is that NO coverage data gets written, none at all (I suspect some missing annotations, see below) Which is of course slightly annoying. I can tweak pkg_add to use File::Temp instead for coverage purposes, but I'd rather not. (note that using File::Temp, including +select,File/Temp.pm Devel::Cover works, which would tend to lean towards the XS code being subtly incomplete)
Re: Diff for evaluation (WACOM tablet driver)
On Tue, Jul 04, 2023 at 07:20:51PM -0400, Thomas Frohwein wrote: > Thanks, I built a kernel with this and no issues observed. I have a > Wacom Bamboo (CTH-470, product id 0x00de) that doesn't attach yet, but > this can be left for future work. I don't see anything glaring. hid.c > could probably use some refactoring at some point in the future because > at least 3 functions now share 99% identical code (hid_is_collection, > hid_get_collection_data, hid_get_id_of_collection). Yeah, I think it's better to get this in first, try to recognize more tablets, then see how similar the code still is and how to refactor it. It's likely there are still monsters hiding in the wacom hw.
Re: Diff for evaluation (WACOM tablet driver)
On Mon, Jul 03, 2023 at 03:20:33PM +0300, Matthieu Herrb wrote: > > Index: dev/usb/usbdevs.h > > === > > RCS file: /cvs/src/sys/dev/usb/usbdevs.h,v > > retrieving revision 1.769 > > diff -u -p -r1.769 usbdevs.h > > --- dev/usb/usbdevs.h 12 Jun 2023 11:26:54 - 1.769 > > +++ dev/usb/usbdevs.h 3 Jul 2023 09:04:50 - > > @@ -1,4 +1,4 @@ > > -/* $OpenBSD: usbdevs.h,v 1.769 2023/06/12 11:26:54 jsg Exp $ */ > > +/* $OpenBSD$ */ > > > > /* > > * THIS FILE IS AUTOMATICALLY GENERATED. DO NOT EDIT. > > @@ -2117,6 +2117,8 @@ > > #defineUSB_PRODUCT_GARMIN_DAKOTA20 0x23c0 /* Dakota 20 */ > > #defineUSB_PRODUCT_GARMIN_GPSMAP62S0x2459 /* GPSmap 62s */ > > > > +/* Gaomon */ > > + > > Strange... looks like you edited the file instead of re-generating it. Nah, I just forgot to re-regenerate after removing the Gaomon comment ;) Since the commit has to be staged in twos, that's not a bother
Re: Diff for evaluation (WACOM tablet driver)
I hope Vladimir will find the time to complete this answer.
As far as Vlad's work goes, he did a presentation last week-end:
https://www.lre.epita.fr/news_content/SS_summer_week_pres/Vladimir_Driver_OpenBSD.pptx
(sorry for the medium, fortunately we have libreoffice)
In the mean time, here is an updated diff.
I removed the Gaomon stuff, which if anything should be a different patch.
And I cleaned up the 20+ minor style violations I could find...
(tabs instead of +4 spaces for continued lines, a few non-style compliant
function declarations and/or code blocks, oh well)
plus an extra malloc.h that snuck in and is not at all needed.
And some typos in comments.
And a C++ style comment. Oh well
I would really for some version of this to get in soonish.
I can vouch that my tablet "works" with it (well, as good as it can work
within the limitations of wscons not allowing it to be easily differentiated
from the normal mouse, which is really a pain for programs like gimp)
dmesg for the tablet with the diff
| uhidev1 at uhub1 port 4 configuration 1 interface 0 "Wacom Co.,Ltd. Intuos S"
rev 2.00/1.07 addr 6
| uhidev1: iclass 3/0, 228 report ids
| uwacom0 at uhidev1: 9 buttons, Z and W dir, tip, barrel
| wsmouse5 at uwacom0 mux 0
| uwacom1 at uhidev1: 9 buttons, Z and W dir, tip, barrel
| wsmouse6 at uwacom1 mux 0
| uwacom2 at uhidev1: 9 buttons, Z and W dir, tip, barrel
| wsmouse7 at uwacom2 mux 0
as far as I understand, it appears as several mice because the stylus
acts as totally different devices depending on the mode/end used
(stuff that wscons completely hides from us).
Without the patch, that tablet appears as 42 different uhid devices (!)
The idea is that the parser for collections was really primitive. The
debug stuff can show the details of various collection. There is the actual
tablet mechanisms (which becomes one device) including scale, stylus, etc,
and some other wacky collections (!): a debug collection that the wacom guys
told us "oh some of our hw team needs that, but don't ever touch" and
some other stuff we can't support yet (like battery support for some
advanced models of stylus)
Index: dev/hid/hid.c
===
RCS file: /cvs/src/sys/dev/hid/hid.c,v
retrieving revision 1.5
diff -u -p -r1.5 hid.c
--- dev/hid/hid.c 20 May 2022 05:03:45 - 1.5
+++ dev/hid/hid.c 3 Jul 2023 09:04:50 -
@@ -657,3 +657,51 @@ hid_is_collection(const void *desc, int
hid_end_parse(hd);
return (0);
}
+
+struct hid_data *
+hid_get_collection_data(const void *desc, int size, int32_t usage,
+uint32_t collection)
+{
+ struct hid_data *hd;
+ struct hid_item hi;
+
+ hd = hid_start_parse(desc, size, hid_all);
+
+ DPRINTF("%s: usage=0x%x\n", __func__, usage);
+ while (hid_get_item(hd, &hi)) {
+ DPRINTF("%s: kind=%d id=%d usage=0x%x(0x%x)\n", __func__,
+ hi.kind, hi.report_ID, hi.usage, usage);
+ if (hi.kind == hid_collection &&
+ hi.collection == collection && hi.usage == usage){
+ DPRINTF("%s: found\n", __func__);
+ return hd;
+ }
+ }
+ DPRINTF("%s: not found\n", __func__);
+ hid_end_parse(hd);
+ return NULL;
+}
+
+int
+hid_get_id_of_collection(const void *desc, int size, int32_t usage,
+uint32_t collection)
+{
+ struct hid_data *hd;
+ struct hid_item hi;
+
+ hd = hid_start_parse(desc, size, hid_all);
+
+ DPRINTF("%s: id=%d usage=0x%x\n", __func__, id, usage);
+ while (hid_get_item(hd, &hi)) {
+ DPRINTF("%s: kind=%d id=%d usage=0x%x(0x%x)\n", __func__,
+ hi.kind, hi.report_ID, hi.usage, usage);
+ if (hi.kind == hid_collection &&
+ hi.collection == collection && hi.usage == usage){
+ DPRINTF("%s: found\n", __func__);
+ return hi.report_ID;
+ }
+ }
+ DPRINTF("%s: not found\n", __func__);
+ hid_end_parse(hd);
+ return 0;
+}
Index: dev/hid/hid.h
===
RCS file: /cvs/src/sys/dev/hid/hid.h,v
retrieving revision 1.10
diff -u -p -r1.10 hid.h
--- dev/hid/hid.h 20 May 2022 05:03:45 - 1.10
+++ dev/hid/hid.h 3 Jul 2023 09:04:50 -
@@ -93,6 +93,10 @@ int hid_locate(const void *, int, int32_
int32_thid_get_data(const uint8_t *buf, int, struct hid_location *);
uint32_t hid_get_udata(const uint8_t *buf, int, struct hid_location *);
inthid_is_collection(const void *, int, uint8_t, int32_t);
+struct hid_data * hid_get_collection_data(const void *, int, int32_t,
+ uint32_t);
+inthid_get_id_of_collection(const void *desc, int size, int32_t usage,
+ uint32_t collection);
#endif /* _KERNEL */
@@ -353,6 +357,7 @@ int hid_is
Re: pkg_add optional behavior "like syspatch"
On Sun, Jul 02, 2023 at 06:06:28PM +0100, Stuart Henderson wrote: > On 2023/07/02 16:49, Solène Rapenne wrote: > > On Sun, 2023-07-02 at 15:51 +0200, Marc Espie wrote: > > > Use-case: some people want to branch automated installs based on > > > whether > > > pkg_add -u (or some other variation) actually did something. > > > > > > As usual we ignore quirks. This adds a flag (-DSYSPATCH_LIKE) > > > which governs the behavior. Code is fairly self-explanatory. > > > > > > I had no better idea for the flag name so far, suggestions welcome. > > > > if I read well, the exit code is 2 when something pkg_add changed > > something? > > > > syspatch exits with 0 when installing an update, 1 if it fails, 2 if > > didn't do anything > > > > Could it be possible to keep it consistent? pkg_add upgrading/installing > > a package should exit with 0, so it doesn't break current scripts, and > > this is what you would expect. > > I wonder whether there's actually a need to make this optional. > 0 for "updated successfully", non-zero values for failed or "no updates > available" makes a lot of sense to me (and makes it easier to check > the common "did this successfully update some package" case). Notice that it works on *every* invocation of pkg_add, so no, it has to be limited. I frequently use "pkg_add something" to make sure some code has been installed (or even pkg_add -u something to make sure it's uptodate) In a script with set -e, having it not return 0 would play havoc.
Re: pkg_add optional behavior "like syspatch"
On Sun, Jul 02, 2023 at 04:49:41PM +0200, Solène Rapenne wrote:
> On Sun, 2023-07-02 at 15:51 +0200, Marc Espie wrote:
> > Use-case: some people want to branch automated installs based on
> > whether
> > pkg_add -u (or some other variation) actually did something.
> >
> > As usual we ignore quirks. This adds a flag (-DSYSPATCH_LIKE)
> > which governs the behavior. Code is fairly self-explanatory.
> >
> > I had no better idea for the flag name so far, suggestions welcome.
> >
> >
> > +sub exit_code($self, $state)
> > +{
> > + my $rc = $self->SUPER::exit_code($state);
> > + if ($rc == 0 && $state->defines("SYSPATCH_LIKE")) {
> > + if (!$state->{did_something}) {
> > + $rc = 2;
> > + }
> > + }
> > + return $rc;
> > +}
> >
> > sub new_state($self, $cmd)
> > {
> >
>
> if I read well, the exit code is 2 when something pkg_add changed
> something?
>
> syspatch exits with 0 when installing an update, 1 if it fails, 2 if
> didn't do anything
>
> Could it be possible to keep it consistent? pkg_add upgrading/installing
> a package should exit with 0, so it doesn't break current scripts, and
> this is what you would expect.
>
> Although, it could exit with 2 if you asked to install a package that
> already exist. And 1 if pkg_add failed.
>
>
I think you missed a negation in the patch
pkg_add optional behavior "like syspatch"
Use-case: some people want to branch automated installs based on whether
pkg_add -u (or some other variation) actually did something.
As usual we ignore quirks. This adds a flag (-DSYSPATCH_LIKE)
which governs the behavior. Code is fairly self-explanatory.
I had no better idea for the flag name so far, suggestions welcome.
Index: OpenBSD/PkgAdd.pm
===
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/PkgAdd.pm,v
retrieving revision 1.142
diff -u -p -r1.142 PkgAdd.pm
--- OpenBSD/PkgAdd.pm 27 Jun 2023 11:11:46 - 1.142
+++ OpenBSD/PkgAdd.pm 2 Jul 2023 13:49:03 -
@@ -861,6 +861,9 @@ sub really_add($set, $state)
if ($state->{received}) {
die "interrupted";
}
+ if (!$set->{quirks}) {
+ $state->{did_something} = 1;
+ }
}
sub newer_has_errors($set, $state)
@@ -1163,6 +1166,8 @@ sub process_parameters($self, $state)
{
my $add_hints = $state->{fuzzy} ? "add_hints" : "add_hints2";
+ $state->{did_something} = 0;
+
# match against a list
if ($state->{pkglist}) {
open my $f, '<', $state->{pkglist} or
@@ -1178,7 +1183,6 @@ sub process_parameters($self, $state)
# update existing stuff
if ($state->{update}) {
-
if (@ARGV == 0) {
@ARGV = sort(installed_packages());
}
@@ -1239,6 +1243,16 @@ sub main($self, $state)
$self->process_setlist($state);
}
+sub exit_code($self, $state)
+{
+ my $rc = $self->SUPER::exit_code($state);
+ if ($rc == 0 && $state->defines("SYSPATCH_LIKE")) {
+ if (!$state->{did_something}) {
+ $rc = 2;
+ }
+ }
+ return $rc;
+}
sub new_state($self, $cmd)
{
Re: posix_spawn(3): explain that handling NULL envp is an extension
On Sun, Jun 25, 2023 at 07:07:33PM -0300, Lucas de Sena wrote: > The manual already describes how posix_spawn(3) behaves when passing it > a NULL envp, but does not make it clear that it is an OpenBSD extension: > > > If envp is NULL, the environment is passed unchanged from the parent > > process. > > That differs from GNU/Linux, for example, where a NULL envp gives the > child an empty environment rather than a copy. Note that a NULL environment is undefined behavior according to POSIX. If you read the OpenGroup description, it very clearly states that envp is a pointer to a NULL terminated array. Does GNU/Linux at least document that passing a NULL pointer means no environment for them ?
Re: pkg-config tweaks
A bit more pkg-config tweaks.
- I missed two anonymous subs.
- somehow I did mangle the flags with respect to --libs-only-other
- so I added regress tests for those.
- I found out that stuff like --libs-only-l will tend to prepend a space like
-lalpha2
turns out the glib2 version of pkg-config doesn't, so I fixed that (not
that hard to do, it's just a question of having do_libs return a proper list
with only existing elements)
- also found a warning message wrt -stable in the regress test
now, support for an extra suffix shouldn't be too hard. But seeing duplicate
code for suffixes handling made me cringe so much I had to fix it.
Please test :)
Hopefully the last iteration.
Index: regress/usr.bin/pkg-config/Makefile
===
RCS file: /cvs/src/regress/usr.bin/pkg-config/Makefile,v
retrieving revision 1.62
diff -u -p -r1.62 Makefile
--- regress/usr.bin/pkg-config/Makefile 15 Sep 2020 07:19:31 - 1.62
+++ regress/usr.bin/pkg-config/Makefile 5 Jun 2023 08:05:19 -
@@ -101,7 +101,12 @@ REGRESS_TARGETS=cmp-vers1-1 \
filter-system-dirs-5 \
filter-system-dirs-6 \
cflags-system-path-1 \
- cflags-system-path-2
+ cflags-system-path-2 \
+ lib-flags-1 \
+ lib-flags-2 \
+ lib-flags-3 \
+ lib-flags-4 \
+
PKG_CONFIG?= /usr/bin/pkg-config
PCONFIG = PKG_CONFIG_PATH=${.CURDIR}/pcdir/ ${PKG_CONFIG}
@@ -329,7 +334,7 @@ cmp-vers5-10:
cmp-vers6-1:
# Test suffixed versions in Requires
- @echo " -lalpha2" > ${WANT}
+ @echo "-lalpha2" > ${WANT}
@${VPCONFIG} --libs requires-test2
@diff -u ${WANT} ${GOT}
@@ -474,7 +479,7 @@ whitespace-libs:
whitespace-linebreak:
# Test linebreak in Description field
- @echo " -lc" > ${WANT}
+ @echo "-lc" > ${WANT}
@${VPCONFIG} --libs linebreak
@diff -u ${WANT} ${GOT}
@@ -631,19 +636,19 @@ variables-4:
variables-5:
# Test --variable
- @echo ' -lfoo-1' > ${WANT}
+ @echo '-lfoo-1' > ${WANT}
@${VPCONFIG} --libs variables
@diff -u ${WANT} ${GOT}
variables-6:
# Test variable overriding from environment
- @echo ' -lfoo-2' > ${WANT}
+ @echo '-lfoo-2' > ${WANT}
@PKG_CONFIG_VARIABLES_FOO_API_VERSION=2 ${VPCONFIG} --libs variables
@diff -u ${WANT} ${GOT}
variables-7:
# Ensure variable overriding only uses uppercase keys
- @echo ' -lfoo-1' > ${WANT}
+ @echo '-lfoo-1' > ${WANT}
@PKG_CONFIG_variables_foo_api_version=2 ${VPCONFIG} --libs variables
@diff -u ${WANT} ${GOT}
@@ -655,13 +660,13 @@ filter-system-dirs-1:
filter-system-dirs-2:
# Test removing -L/usr/lib as a system directory
- @echo ' -lfilter' > ${WANT}
+ @echo '-lfilter' > ${WANT}
@${VPCONFIG} --libs filter
@diff -u ${WANT} ${GOT}
filter-system-dirs-3:
# Test removing -L/usr/lib as a system directory (static)
- @echo ' -lfilter -lprivate-filter' > ${WANT}
+ @echo '-lfilter -lprivate-filter' > ${WANT}
@${VPCONFIG} --static --libs filter
@diff -u ${WANT} ${GOT}
@@ -697,6 +702,30 @@ cflags-system-path-2:
clean:
rm -f *.want *.got
+
+lib-flags-1:
+ # Test --libs-only-other
+ @echo "-pthread" > ${WANT}
+ @${VPCONFIG} --libs-only-other lib-flags
+ @diff -u ${WANT} ${GOT}
+
+lib-flags-2:
+ # Test --libs
+ @echo "-L/usr/local/lib -pthread -lalpha2" > ${WANT}
+ @${VPCONFIG} --libs lib-flags
+ @diff -u ${WANT} ${GOT}
+
+lib-flags-3:
+ # Test --libs-only-L
+ @echo "-L/usr/local/lib" > ${WANT}
+ @${VPCONFIG} --libs-only-L lib-flags
+ @diff -u ${WANT} ${GOT}
+
+lib-flags-4:
+ # Test --libs-only-l
+ @echo "-lalpha2" > ${WANT}
+ @${VPCONFIG} --libs-only-l lib-flags
+ @diff -u ${WANT} ${GOT}
.PHONY: ${REGRESS_TARGETS}
Index: regress/usr.bin/pkg-config/pcdir/lib-flags.pc
===
RCS file: regress/usr.bin/pkg-config/pcdir/lib-flags.pc
diff -N regress/usr.bin/pkg-config/pcdir/lib-flags.pc
--- /dev/null 1 Jan 1970 00:00:00 -
+++ regress/usr.bin/pkg-config/pcdir/lib-flags.pc 5 Jun 2023 08:05:19
-
@@ -0,0 +1,4 @@
+Name: lib separation test
+Description: pkg-config(1) regress file
+Version: 0.0
+Libs: -lalpha2 -L/usr/local/lib -pthread
Index: usr.bin/pkg-config/pkg-config
===
RCS file: /cvs/src/usr.bin/pkg-config/pkg-config,v
retrieving revision 1.95
diff -u -p -r1.95 pkg-config
--- usr.bin/pkg-config/pkg-config 15 Sep 2020 07:18:45 - 1.95
+++ usr.bin/pkg-config/pkg-config 5 Jun 2023 08:05:19 -
@@ -16,14 +16,20 @@
# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
# OR IN CONNECTION WITH T
Re: make: slightly better diagnostic
Here's a slightly more specific diff avoiding useless stat(2)
Index: engine.c
===
RCS file: /cvs/src/usr.bin/make/engine.c,v
retrieving revision 1.70
diff -u -p -r1.70 engine.c
--- engine.c25 Oct 2021 19:54:29 - 1.70
+++ engine.c28 May 2023 16:07:44 -
@@ -84,6 +84,7 @@
#include "extern.h"
#include "lst.h"
#include "timestamp.h"
+#include "main.h"
#include "make.h"
#include "pathnames.h"
#include "error.h"
@@ -210,6 +211,7 @@ node_failure(GNode *gn)
fflush(stdout);
else {
print_errors();
+ dump_unreadable();
Punt(NULL);
}
}
Index: main.c
===
RCS file: /cvs/src/usr.bin/make/main.c,v
retrieving revision 1.129
diff -u -p -r1.129 main.c
--- main.c 17 Jan 2023 13:03:22 - 1.129
+++ main.c 28 May 2023 16:07:45 -
@@ -87,6 +87,8 @@ bool ignoreErrors; /* -i flag */
bool beSilent; /* -s flag */
bool dumpData; /* -p flag */
+static LISTunreadable;
+
struct dirs {
char *current;
char *object;
@@ -826,6 +828,48 @@ main(int argc, char **argv)
return 0;
}
+struct unreadable {
+ char *fname;
+ int errcode;
+};
+
+void
+dump_unreadable(void)
+{
+ struct unreadable *u;
+
+ if (Lst_IsEmpty(&unreadable))
+ return;
+
+ fprintf(stderr, "Makefile(s) that couldn't be read:\n");
+
+ while ((u = Lst_Pop(&unreadable))) {
+ fprintf(stderr, "\t%s: %s\n", u->fname, strerror(u->errcode));
+ free(u->fname);
+ free(u);
+ }
+}
+
+static FILE *
+open_makefile(const char *fname)
+{
+ FILE *stream;
+ struct unreadable *u;
+
+ stream = fopen(fname, "r");
+ if (stream != NULL)
+ return stream;
+
+ if (errno != ENOENT) {
+ u = emalloc(sizeof *u);
+ u->fname = strdup(fname);
+ u->errcode = errno;
+ Lst_AtEnd(&unreadable, u);
+ }
+
+ return NULL;
+}
+
/*-
* ReadMakefile --
* Open and parse the given makefile.
@@ -848,14 +892,14 @@ ReadMakefile(void *p, void *q)
Var_Set("MAKEFILE", "");
Parse_File(estrdup("(stdin)"), stdin);
} else {
- if ((stream = fopen(fname, "r")) != NULL)
+ if ((stream = open_makefile(fname)) != NULL)
goto found;
/* if we've chdir'd, rebuild the path name */
if (d->current != d->object && *fname != '/') {
char *path;
path = Str_concat(d->current, fname, '/');
- if ((stream = fopen(path, "r")) == NULL)
+ if ((stream = open_makefile(path)) == NULL)
free(path);
else {
fname = path;
@@ -866,7 +910,14 @@ ReadMakefile(void *p, void *q)
name = Dir_FindFile(fname, userIncludePath);
if (!name)
name = Dir_FindFile(fname, systemIncludePath);
- if (!name || !(stream = fopen(name, "r")))
+ if (!name)
+ return false;
+ /* do not try to open a file we already have, so that
+* dump_unreadable() yields non-confusing results.
+*/
+ if (strcmp(name, fname) == 0)
+ return false;
+ if ((stream = open_makefile(name)) == NULL)
return false;
fname = name;
/*
Index: main.h
===
RCS file: /cvs/src/usr.bin/make/main.h,v
retrieving revision 1.6
diff -u -p -r1.6 main.h
--- main.h 13 Jan 2020 14:51:50 - 1.6
+++ main.h 28 May 2023 16:07:45 -
@@ -41,4 +41,8 @@ extern Lstcreate;
/* set_notparallel(): used to influence running mode from parse.c */
extern void set_notparallel(void);
+/* dump_unreadable: in case of some errors, dump makefile names
+ * we found but are unable to read.
+ */
+extern void dump_unreadable(void);
#endif
Index: parse.c
===
RCS file: /cvs/src/usr.bin/make/parse.c,v
retrieving revision 1.134
diff -u -p -r1.134 parse.c
--- parse.c 6 Mar 2021 08:31:42 - 1.134
+++ parse.c 28 May 2023 16:07:45 -
@@ -1696,6 +1696,7 @@ void
Parse_MainName(Lst listmain) /* result list */
{
if (mainNode == NULL) {
+ dump_unreadable();
Punt("no target to make.");
/*NOTREACHED*/
} else if (mainNode->type & OP_DOUBLEDEP) {
Re: make: slightly better diagnostic
On Sun, May 28, 2023 at 04:44:37PM +0200, Omar Polo wrote:
> On 2023/05/28 13:16:34 +0200, Marc Espie wrote:
> > Turns out that, due to the search rules we use, make
> > is mostly silent in case it couldn't read a Makefile
> >
> > The following patch lets it track the makefilenames that
> > do exist, but that it wasn't able to open, so that
> > a post-mortem can include these.
> >
> >
> > Not sure if other use-cases could also use the post-mortem
> >
> > The tweak to ReadMakefiles is to avoid pushing the same
> > path name twice.
> >
> > Using Lst_Pop ensures this diagnostic only ever occurs once,
> > in case we find other places we might want to stick this.
> >
> > (and realizing that, I should probably add comments in both
> > places instead of explaining this through email)
>
> It would indeed be nice to have a "post-mortem" message with the
> Makefiles that couldn't be opened. it would have saved me some
> headaches in the past with ports that have too tight permissions.
>
> Diff below reads fine to me, just one question inline, but it doesn't
> seem to catch all the situations, consider:
>
> % mkdir /tmp/foo && cd /tmp/foo
> /tmp/foo
> % touch Makefile
> % chmod 000 Makefile
> % make
> make: no target to make.
> % make all
> make: don't know how to make all
> Stop in /tmp/foo
> Makefile(s) that couldn't be read: Makefile
>
> a bare `make' don't print the nice error message, trying with a target
> instead does.
Like I said, yeah, there are more error cases that warrant errors.
We got to be careful, because make has got some default behavior built-in,
so we should only print things if stuff gets erroring out.
> > Index: main.c
> > ===
> > RCS file: /cvs/src/usr.bin/make/main.c,v
> > retrieving revision 1.129
> > diff -u -p -r1.129 main.c
> > --- main.c 17 Jan 2023 13:03:22 - 1.129
> > +++ main.c 28 May 2023 11:12:48 -
> > [...]
> > +static FILE *
> > +open_makefile(const char *fname)
> > +{
> > + FILE *stream;
> > + struct stat buffer;
> > +
> > + stream = fopen(fname, "r");
> > + if (stream != NULL)
> > + return stream;
> > +
> > + if (stat(fname, &buffer) == 0) {
> > + Lst_AtEnd(&unreadable, strdup(fname));
> > + }
>
> Why the stat? Can't we just check errno for EACCES instead? EACCES
> is also returned when search permission is denied for a component of
> the path prefix, which would be a difference but maybe a nice one?
You're right, checking errno makes more sense and would catch more cases.
Actually, the only errno that's "normal" in our case is ENOENT. Anything else
is suspicious.
make: slightly better diagnostic
Turns out that, due to the search rules we use, make
is mostly silent in case it couldn't read a Makefile
The following patch lets it track the makefilenames that
do exist, but that it wasn't able to open, so that
a post-mortem can include these.
Not sure if other use-cases could also use the post-mortem
The tweak to ReadMakefiles is to avoid pushing the same
path name twice.
Using Lst_Pop ensures this diagnostic only ever occurs once,
in case we find other places we might want to stick this.
(and realizing that, I should probably add comments in both
places instead of explaining this through email)
Index: engine.c
===
RCS file: /cvs/src/usr.bin/make/engine.c,v
retrieving revision 1.70
diff -u -p -r1.70 engine.c
--- engine.c25 Oct 2021 19:54:29 - 1.70
+++ engine.c28 May 2023 11:12:48 -
@@ -84,6 +84,7 @@
#include "extern.h"
#include "lst.h"
#include "timestamp.h"
+#include "main.h"
#include "make.h"
#include "pathnames.h"
#include "error.h"
@@ -210,6 +211,7 @@ node_failure(GNode *gn)
fflush(stdout);
else {
print_errors();
+ dump_unreadable();
Punt(NULL);
}
}
Index: main.c
===
RCS file: /cvs/src/usr.bin/make/main.c,v
retrieving revision 1.129
diff -u -p -r1.129 main.c
--- main.c 17 Jan 2023 13:03:22 - 1.129
+++ main.c 28 May 2023 11:12:48 -
@@ -87,6 +87,8 @@ bool ignoreErrors; /* -i flag */
bool beSilent; /* -s flag */
bool dumpData; /* -p flag */
+static LISTunreadable;
+
struct dirs {
char *current;
char *object;
@@ -826,6 +828,40 @@ main(int argc, char **argv)
return 0;
}
+void
+dump_unreadable(void)
+{
+ char *fname;
+
+ if (Lst_IsEmpty(&unreadable))
+ return;
+
+ fprintf(stderr, "Makefile(s) that couldn't be read: ");
+
+ while ((fname = Lst_Pop(&unreadable))) {
+ fprintf(stderr, "%s ", fname);
+ free(fname);
+ }
+ fprintf(stderr, "\n");
+}
+
+static FILE *
+open_makefile(const char *fname)
+{
+ FILE *stream;
+ struct stat buffer;
+
+ stream = fopen(fname, "r");
+ if (stream != NULL)
+ return stream;
+
+ if (stat(fname, &buffer) == 0) {
+ Lst_AtEnd(&unreadable, strdup(fname));
+ }
+
+ return NULL;
+}
+
/*-
* ReadMakefile --
* Open and parse the given makefile.
@@ -848,14 +884,14 @@ ReadMakefile(void *p, void *q)
Var_Set("MAKEFILE", "");
Parse_File(estrdup("(stdin)"), stdin);
} else {
- if ((stream = fopen(fname, "r")) != NULL)
+ if ((stream = open_makefile(fname)) != NULL)
goto found;
/* if we've chdir'd, rebuild the path name */
if (d->current != d->object && *fname != '/') {
char *path;
path = Str_concat(d->current, fname, '/');
- if ((stream = fopen(path, "r")) == NULL)
+ if ((stream = open_makefile(path)) == NULL)
free(path);
else {
fname = path;
@@ -866,7 +902,11 @@ ReadMakefile(void *p, void *q)
name = Dir_FindFile(fname, userIncludePath);
if (!name)
name = Dir_FindFile(fname, systemIncludePath);
- if (!name || !(stream = fopen(name, "r")))
+ if (!name)
+ return false;
+ if (strcmp(name, fname) == 0)
+ return false;
+ if ((stream = open_makefile(name)) == NULL)
return false;
fname = name;
/*
Index: main.h
===
RCS file: /cvs/src/usr.bin/make/main.h,v
retrieving revision 1.6
diff -u -p -r1.6 main.h
--- main.h 13 Jan 2020 14:51:50 - 1.6
+++ main.h 28 May 2023 11:12:48 -
@@ -41,4 +41,8 @@ extern Lstcreate;
/* set_notparallel(): used to influence running mode from parse.c */
extern void set_notparallel(void);
+/* dump_unreadable: in case of some errors, dump makefile names
+ * we found but are unable to read.
+ */
+extern void dump_unreadable(void);
#endif
Re: pkg-config tweaks
There was a small typo which broke xenocara, as noticed by tb@
(sidenote: I hate this shitty configure stuff that can't even give you
the warning messages from tools that ran. *OF COURSE* it's because so
much of that shit talks incessantly even when things are fine)
Index: pkg-config
===
RCS file: /cvs/src/usr.bin/pkg-config/pkg-config,v
retrieving revision 1.95
diff -u -p -r1.95 pkg-config
--- pkg-config 15 Sep 2020 07:18:45 - 1.95
+++ pkg-config 26 May 2023 09:40:46 -
@@ -16,14 +16,20 @@
# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
-use strict;
-use warnings;
+use v5.36;
use Config;
use Getopt::Long;
use File::Basename;
use File::stat;
use OpenBSD::PkgConfig;
+use constant {
+ ONLY_I => 1,
+ ONLY_l => 2,
+ ONLY_L => 4,
+ ONLY_OTHER => 8
+};
+
my @PKGPATH = qw(/usr/lib/pkgconfig
/usr/local/lib/pkgconfig
/usr/local/share/pkgconfig
@@ -70,7 +76,7 @@ defined $ENV{PKG_CONFIG_DEBUG_SPEW} ? $m
if ($logfile) {
open my $L, ">>" , $logfile or die;
- print $L beautify_list($0, @ARGV), "\n";
+ say $L beautify_list($0, @ARGV);
close $L;
}
@@ -87,7 +93,7 @@ GetOptions( 'debug' => \$mode{debug},
'help' => \&help, #does not return
'usage' => \&help, #does not return
'list-all' => \$mode{list},
- 'version' => sub { print "$version\n" ; exit(0);}
,
+ 'version' => sub { say $version ; exit(0);} ,
'errors-to-stdout' => sub { $mode{estdout} = 1},
'print-errors' => sub { $mode{printerr} = 1},
'silence-errors'=> sub { $mode{printerr} = 0},
@@ -97,13 +103,13 @@ GetOptions('debug' =>
\$mode{debug},
'print-requires'=> \$mode{printrequires},
'print-requires-private' => \$mode{printrequiresprivate},
- 'cflags'=> sub { $mode{cflags} = 3},
- 'cflags-only-I' => sub { $mode{cflags} |= 1},
- 'cflags-only-other' => sub { $mode{cflags} |= 2},
- 'libs' => sub { $mode{libs} = 7},
- 'libs-only-l' => sub { $mode{libs} |= 1},
- 'libs-only-L' => sub { $mode{libs} |= 2},
- 'libs-only-other' => sub { $mode{libs} |= 4},
+ 'cflags'=> sub { $mode{cflags} =
ONLY_I|ONLY_OTHER},
+ 'cflags-only-I' => sub { $mode{cflags} |= ONLY_I},
+ 'cflags-only-other' => sub { $mode{cflags} |= ONLY_OTHER},
+ 'libs' => sub { $mode{libs} =
ONLY_L|ONLY_l|ONLY_OTHER},
+ 'libs-only-l' => sub { $mode{libs} |= ONLY_l},
+ 'libs-only-L' => sub { $mode{libs} |= ONLY_L},
+ 'libs-only-other' => sub { $mode{libs} |= ONLY_OTHER},
'exists'=> sub { $mode{exists} = 1} ,
'validate' => sub { $mode{validate} = 1},
'static'=> sub { $mode{static} = 1},
@@ -178,9 +184,9 @@ sub get_next_module
if ($module =~ m/,/) {
my @ms = split(/,/, $module);
$m = shift @ms;
- unshift(@ARGV, @ms) if (scalar(@ms) > 0);
+ unshift(@ARGV, @ms) if @ms != 0;
} else {
- return $module;
+ return $module;
}
return $m;
@@ -267,16 +273,15 @@ if ($mode{static}){
if ($mode{cflags} || $mode{libs} || $mode{variable}) {
push @vlist, do_cflags($dep_cfg_list) if $mode{cflags};
push @vlist, do_libs($dep_cfg_list) if $mode{libs};
- print join(' ', @vlist), "\n" if $rc == 0;
+ say join(' ', @vlist) if $rc == 0;
}
exit $rc;
###
-sub handle_config
+sub handle_config($p, $op, $v, $list)
{
- my ($p, $op, $v, $list) = @_;
my $cfg = cache_find_config($p);
unshift @$list, $p if defined $cfg;
@@ -316,7 +321,7 @@ sub handle_config
my $deps = $cfg->get_property($property, $variables);
return unless defined $deps;
for my $dep (@$deps) {
- if ($dep =~
m/^(.*?)\s*([<=>]+)\s*([\d\.]+|[\d\.]+[\w]*[\d]+)$/) {
+ if ($dep =~
m/^(.*?)\s*([<=>]+)\s*([\d\.]+|[\d\.]+\w*\d+)$/) {
handle_config($1, $2, $3, $list);
} else {
handle_config($dep, undef, undef, $list);
@@ -339,10 +344,8 @@ sub handle_config
# look for the .pc file in each of the PKGPATH el
pkg-config tweaks
- move to 5.36: signatures + prototypes mostly everywhere
Not used for Getopt::Long, because the calling conventions are somewhat
too verbose.
- use constant for the mode{libs} and mode{cflags} values
- remove two completely unneeded [] in regexps
- fix indentation and parentheses in a few locations
There should be no behavior change, please test.
Deeper question: as it stands \w and such do handle unicode, more or less.
Is this something we actually want/need in pkg-config ? should we look at
restricting the regexps through one of the locale modifiers ?
Index: pkg-config
===
RCS file: /cvs/src/usr.bin/pkg-config/pkg-config,v
retrieving revision 1.95
diff -u -p -r1.95 pkg-config
--- pkg-config 15 Sep 2020 07:18:45 - 1.95
+++ pkg-config 22 May 2023 07:22:10 -
@@ -16,14 +16,20 @@
# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
-use strict;
-use warnings;
+use v5.36;
use Config;
use Getopt::Long;
use File::Basename;
use File::stat;
use OpenBSD::PkgConfig;
+use constant {
+ ONLY_I => 1,
+ ONLY_l => 2,
+ ONLY_L => 4,
+ ONLY_OTHER => 8
+};
+
my @PKGPATH = qw(/usr/lib/pkgconfig
/usr/local/lib/pkgconfig
/usr/local/share/pkgconfig
@@ -70,7 +76,7 @@ defined $ENV{PKG_CONFIG_DEBUG_SPEW} ? $m
if ($logfile) {
open my $L, ">>" , $logfile or die;
- print $L beautify_list($0, @ARGV), "\n";
+ say $L beautify_list($0, @ARGV);
close $L;
}
@@ -87,7 +93,7 @@ GetOptions( 'debug' => \$mode{debug},
'help' => \&help, #does not return
'usage' => \&help, #does not return
'list-all' => \$mode{list},
- 'version' => sub { print "$version\n" ; exit(0);}
,
+ 'version' => sub { say $version ; exit(0);} ,
'errors-to-stdout' => sub { $mode{estdout} = 1},
'print-errors' => sub { $mode{printerr} = 1},
'silence-errors'=> sub { $mode{printerr} = 0},
@@ -97,13 +103,13 @@ GetOptions('debug' =>
\$mode{debug},
'print-requires'=> \$mode{printrequires},
'print-requires-private' => \$mode{printrequiresprivate},
- 'cflags'=> sub { $mode{cflags} = 3},
- 'cflags-only-I' => sub { $mode{cflags} |= 1},
- 'cflags-only-other' => sub { $mode{cflags} |= 2},
- 'libs' => sub { $mode{libs} = 7},
- 'libs-only-l' => sub { $mode{libs} |= 1},
- 'libs-only-L' => sub { $mode{libs} |= 2},
- 'libs-only-other' => sub { $mode{libs} |= 4},
+ 'cflags'=> sub { $mode{cflags} =
ONLY_I|ONLY_OTHER},
+ 'cflags-only-I' => sub { $mode{cflags} |= ONLY_I},
+ 'cflags-only-other' => sub { $mode{cflags} |= ONLY_OTHER},
+ 'libs' => sub { $mode{libs} =
ONLY_L|ONLY_l|ONLY_OTHER},
+ 'libs-only-l' => sub { $mode{libs} |= ONLY_l},
+ 'libs-only-L' => sub { $mode{libs} |= ONLY_L},
+ 'libs-only-other' => sub { $mode{libs} |= ONLY_OTHER},
'exists'=> sub { $mode{exists} = 1} ,
'validate' => sub { $mode{validate} = 1},
'static'=> sub { $mode{static} = 1},
@@ -178,9 +184,9 @@ sub get_next_module
if ($module =~ m/,/) {
my @ms = split(/,/, $module);
$m = shift @ms;
- unshift(@ARGV, @ms) if (scalar(@ms) > 0);
+ unshift(@ARGV, @ms) if @ms != 0;
} else {
- return $module;
+ return $module;
}
return $m;
@@ -267,16 +273,15 @@ if ($mode{static}){
if ($mode{cflags} || $mode{libs} || $mode{variable}) {
push @vlist, do_cflags($dep_cfg_list) if $mode{cflags};
push @vlist, do_libs($dep_cfg_list) if $mode{libs};
- print join(' ', @vlist), "\n" if $rc == 0;
+ say join(' ', @vlist) if $rc == 0;
}
exit $rc;
###
-sub handle_config
+sub handle_config($p, $op, $v, $list)
{
- my ($p, $op, $v, $list) = @_;
my $cfg = cache_find_config($p);
unshift @$list, $p if defined $cfg;
@@ -316,7 +321,7 @@ sub handle_config
my $deps = $cfg->get_property($property, $variables);
return unless defined $deps;
for my $dep (@$deps) {
- if ($dep =~
m/^(.*?)\s*([<=>]+)\s*([\d\.]+|[\d\.]+[\w]*[\d]+)$/) {
+ if ($dep =~
m/^(.*
Re: cwm: add fvwm and tvm as default wm entries
As another rant: we old farts know which window manager we want to use. But for newer users, there might be a chance to find something cool before they get totally fossilized. And secondary rant: X is a failure, in that there is a *choice* of window managers, but so many of them haven't been really upgraded and thus are "useless" because they don't support modern hints. It looks like a very interesting line to die on.
Re: cwm: add fvwm and tvm as default wm entries
On Tue, May 16, 2023 at 02:33:34AM +, Klemens Nanni wrote: > On Mon, May 15, 2023 at 09:42:47AM -0400, Bryan Steele wrote: > > On Mon, May 15, 2023 at 09:17:00AM -0400, Okan Demirmen wrote: > > > On Mon 2023.05.15 at 10:41 +0200, Matthieu Herrb wrote: > > > > On Mon, May 15, 2023 at 06:26:41AM +, Klemens Nanni wrote: > > > > > Both fvwm(1) and twm(1) have a restart menu that contains other window > > > > > managers by default, which is useful if you want to switch around > > > > > without restarting X and/or custom window manager config. > > > > > > > > > > cwm(1) only offers to restart into itself by deafult. > > > > > Add the other two we ship by default so users can round trip between > > > > > them. > > > > > > > > > > Feedback? OK? > > > > > > > > Last year I mentionned that I think we should retire twm. It's really > > > > too old and missing support for the modern window managers hints. > > > > > > > > People still using it should switch to cwm or maybe ctwm from ports > > > > (to keep the same configurarion system), or someone should step up to > > > > maintain it and enhance it with exwmh support. (but this is somehow > > > > just wasting time imho). > > I don't know anything about twm, so I'll leave that to others. > > > > > > > > > Otherwise ok to add this and fix the other WM menus for other window > > > > managers (those parts of the configs are already local changes in > > > > Xenocara) > > > > > > I might argue the opposite, to remove cwm from fvwm and twm restart > > > menus, if > > > this inconsistency is a real concern. The entries in fvwm/twm are in the > > > (shipped) example config files, where-as below it is, well, there for > > > good with > > > no user choice. Heck, how often to do people even use this restart wm to > > > another WM outside of playing around? Most window managers handle restarts > > > differently, regardless of what ICCCM/EWMH says) and even then, crossing > > > window > > > managers like this introduces inconsistencies. It's fine for playing > > > around I > > > suppose, but is it really a demanded "workflow"? > > It is convenient for testing because you keep all your windows and don't > have to login out and in again; that's about it for me. I have to side with kn on this. Actually, it would be cool if ywe could register more wm and switch on runtimes (a bit like @shell ?)
Re: converting perl stuff to v5.36
On Mon, May 08, 2023 at 01:23:25AM -0400, George Koehler wrote:
> On Sun, 7 May 2023 19:21:11 -0700
> Philip Guenther wrote:
>
> > On Sun, May 7, 2023 at 6:13 AM Marc Espie
> > wrote:
> >
> > > I'm actually wondering whether keeping the prototype is worth it.
> > ...
> > For plain subs, I would only keep them if they _really_ help the calls look
> > for more perl-ish, by whatever scale you currently measure that. Maybe a
> > (&@) prototype so you can do "mysub { s/implicit/sub here/ } qw(args here)"
> > ala map and grep, but...eh.
>
> I wrote some (&@) prototypes before v5.36,
>
> | use v5.28;
> | use warnings;
> | use experimental 'signatures';
> |
> | sub bsearch :prototype(&@) ($block, @list) { ... }
> | sub bsearch_range :prototype(&@) ($block, $low, $high) { ... }
>
> The signature checks that bsearch_range has exactly 3 arguments.
>
> I sometimes call subs with the wrong number of arguments. My other
> frequent mistakes in Perl are syntax errors, strict(3p) errors, and
> warnings(3p) of uninitialized values.
>
>
I was always using strict and warnings unless I forgot.
I've got a few (very few) tricky uses on prototypes.
I might fix them as a switch to v5.36 before doing anything else!
(so far, the main abuser of perl funky syntax is the system stuff in State...
Not convinced about any other "regular" way of doing things, though I might
decide to try for a hash with names as first part, since the part that's run
in child/parent isn't obvious)
(the other sticky part is native sig handlers not having clear signatures
thanks in parts to WARN/DIE)
Re: converting perl stuff to v5.36
On Sun, May 07, 2023 at 07:21:11PM -0700, Philip Guenther wrote: > Yeah, the downside of signatures is that by default it makes adding > parameters a breaking change and can thus calcify the interface. Something > for authors of shared modules that have callbacks to carefully consider. :/ So far, my stance on my code is that only stuff that's already in-tree matters. If people have external code, I'll be glad to fix things for them. "Bugs" in current code have been few and resulted in improvements in either documentation or interface in 99% of the cases. (and opening a constructor to extra params takes about 5 minutes)
converting perl stuff to v5.36
It is generally a good thing, I'm mostly talking about
the "signatures" stuff that allows functions with stuff that looks like
usual languages.
Other benefits include somewhat "cheap" check for correct number of parameters
and generally making code shorter.
Basically this converts very perlish code like
package OpenBSD::PackingElement::DirlikeObject;
sub process_dependency
{
my ($self, $mtree) = @_;
$mtree->{dir}{$self->fullname} = 1;
}
into
package OpenBSD::PackingElement::DirlikeObject;
sub process_dependency($self, $mtree)
{
$mtree->{dir}{$self->fullname} = 1;
}
which is
- shorter
- more readable for people not used to perl
- actually errors out on runtime if you use more parameters than expected,
thus tightening the actual usage.
(I also hope that the binding is/will become more efficient than writing
stuff manually)
Stuff to watch out for:
- it supersedes the syntax for prototypes. So if you had prototypes
(I did!), you need to explicitly write :prototypes in front of them.
For instance, constant methods like
package X;
sub is_okay()
{
1;
}
would become:
package X;
sub is_okay :prototype() ($)
{
1;
}
under the new rules.
I'm actually wondering whether keeping the prototype is worth it.
- it's still possible to not add any signature when you don't know.
- use v5.36; implies "use strict;" "use warnings;" and "use feature qw(say);"
which is cool
- beware that calling code without parenthesis, e.g., stuff like
&$code;
*will* not create a new context (and thus reuse @_ implicitly, which doesn't
mesh well with signatures)
- parameters which aren't used can use a single $ or something, which leaves
the question of documentation.
After fiddling a bit with it, I've settled on documenting the method
fully in comments, e.g., for instance:
# $class->recognize($filename, $fsobj, $data):
# called for each class in order until the "default" file
# some retrieved data such as file's contents are cached for
# efficiency
sub recognize($, $, $, $)
{
return 1;
}
(which is very useful for any base class that actually does nothing)
- lambdas (anonymous subs) can use signatures. Note that stuff like
File::Find explicitly takes 0 parameters, whereas stuff like signal handler
is woefully underspecified !
Re: spurious error message from signify
On Sat, Apr 22, 2023 at 11:02:23AM +0200, Marc Espie wrote: > Well, sdk stumbled upon it > (see docbooks-dsssl-1.79.tgz in snapshots right now) > > Turns out that, if the archive is *exactly* a multiple of 64KB, > we will error out at EOF. > > I believe keeping the check for short reads and exiting as > well for files that do not match 64KB lengths is the right thing > to do. > > (note that this does not affect the security of actual packages > in any way, since the important verification, namely only passing > signed checksummed data through the pipe, is preserved) > > 0kay ? And of course, thinking some more, I got a better patch, since the sha part of the signature should match the file exactly. So, not only should we exit the loop when we run into a completely empty buffer, but we should *also* report if we still have checksums from the gzip header that haven't been matched by any data. (somewhat unlikely unless done deliberately, because it requires the file read to be truncated at the checksum divide precisely, and that won't fall on any natural boundary thanks to the gzip header length being somewhat arbitrary) Index: zsig.c === RCS file: /cvs/src/usr.bin/signify/zsig.c,v retrieving revision 1.18 diff -u -p -r1.18 zsig.c --- zsig.c 22 Dec 2019 06:37:25 - 1.18 +++ zsig.c 22 Apr 2023 09:05:29 - @@ -160,6 +160,8 @@ copy_blocks(int fdout, int fdin, const c if (more == 0) break; } + if (n == 0) + break; SHA512_256Data(buffer, n, output); if (endsha - sha < SHA512_256_DIGEST_STRING_LENGTH-1) errx(4, "signature truncated"); @@ -172,6 +174,8 @@ copy_blocks(int fdout, int fdin, const c if (n != bufsize) break; } + if (endsha != sha) + errx(4, "file truncated"); free(buffer); }
Re: pax: GNU tar base-256 size field support
On Tue, Apr 18, 2023 at 08:10:06PM -0600, Todd C. Miller wrote: > I recently ran into a problem with busybox tar generating archives > where the size field is base-256 encoded for files larger than 8GB. > Apparently this is a GNU tar extension. > > Do we want to support this in pax? Below is an initial diff that > at least produces the correct results when listing the archive. I > have not yet verified that it gets extracted correctly. If there > is interest I will do some more testing. It's likely we may encounter this, sooner or later, in ports. Either in source archives, or in data files for a game or something. It's also likely it's obscure enough that it won't be diagnosed correctly by the people running into that, so we may end up with another port requiring GNU tar as a dependency. I'd say that yes, at least supporting proper diagnostics for that would be a good idea.
spurious error message from signify
Well, sdk stumbled upon it (see docbooks-dsssl-1.79.tgz in snapshots right now) Turns out that, if the archive is *exactly* a multiple of 64KB, we will error out at EOF. I believe keeping the check for short reads and exiting as well for files that do not match 64KB lengths is the right thing to do. (note that this does not affect the security of actual packages in any way, since the important verification, namely only passing signed checksummed data through the pipe, is preserved) 0kay ? Index: zsig.c === RCS file: /cvs/src/usr.bin/signify/zsig.c,v retrieving revision 1.18 diff -u -p -r1.18 zsig.c --- zsig.c 22 Dec 2019 06:37:25 - 1.18 +++ zsig.c 22 Apr 2023 09:00:11 - @@ -160,6 +160,8 @@ copy_blocks(int fdout, int fdin, const c if (more == 0) break; } + if (n == 0) + break; SHA512_256Data(buffer, n, output); if (endsha - sha < SHA512_256_DIGEST_STRING_LENGTH-1) errx(4, "signature truncated");
PKU ?
I'm curious about the new enforcement strategies. Unfortunately I'm a bit lost in the 1000+ pages of the intel manual. Could someone point me to the document that describes PKU, specifically ?
Re: Preferred TERM for pkg_add
Actual patches to pkg_add (if need be) welcome The switch to not using full term length was actually done at Theo's request. I don't remember the details, but Theo's rationale was quite reasonable and made lots of sense. (specifically, using the full line length would be cool, but there were lots of fringe cases where it would fuck the display up for no reason).
Re: OpenBSD perl 5.36.0 - Call for Testing
One thing that's going to be very useful IMO is newer function bindings. Us perlers don't really care, matching $@ after the sub is cool. But having off-the-mill function "declarations" that look a lot like function declarations in other more common languages (e.g. sub f($a, $b) ) might help *a lot* in getting people to read perl code. Gonna try that one out for sure!
Re: games: add dots and boxes game
On Tue, Dec 13, 2022 at 02:29:22PM +, Stuart Henderson wrote: > On 2022/12/13 14:27, Stuart Henderson wrote: > > On 2022/12/13 14:13, Janne Johansson wrote: > > > Den tis 13 dec. 2022 kl 14:11 skrev Alireza Arzehgar > > > : > > > > I implemented an interesting game. I thought this game is cool and > > > > could be > > > > fun > > > > for OpenBSD users. I don't know what game will accept on OpenBSD. But I > > > > hope > > > > this patch is interesting. > > > > > > Send it as a port instead. > > > > yep, I don't think we're very likely to add any more to src/games. > > > > > +sprintf(text, "\033[%dm%s\033[0m", 30 + id, msg); > > > > and even if we did, base doesn't assume that every console handles > > ANSI X3.64 colour sequences. there are some style(9) issues too. > > > > ports would be the right place to add this. > > > > (...though it would be better if it didn't assume ANSI there, too...) Do we actually have any tty in wide use that doesn't accept ANSI color sequences these days ? It's nice to be pedantic about it, but be real, the world of actual VT220 and other Wyse thingies is long gone the way of the dodo.
Re: run-regress-* targets in bsd.regress.mk
On Fri, Dec 02, 2022 at 02:08:33PM +0100, Theo Buehler wrote:
> On Fri, Dec 02, 2022 at 01:52:43PM +0100, Marc Espie wrote:
> > On Fri, Dec 02, 2022 at 12:59:02PM +0100, Theo Buehler wrote:
> > > I wanted to override some of the default run-regress-* targets by
> > > declaring them in regress Makefiles. Many tests already do that and
> > > it happens to work as expected.
> > >
> > > However, this relies on behavior in the BUGS section of make. In fact,
> > > espie informs me that gmake behaves the other way around and honors the
> > > last definition (gmake also warns about redefinitions).
> > >
> > > To avoid this, I'd like to have bsd.regress.mk define these default
> > > targets only if they aren't already defined. This matches most targets
> > > in the other *.mk files.
> > >
> > > I kept the .PHONY target outside of the .if block since most regress
> > > Makefiles don't use .PHONY at all although they often should. In fact,
> > > none of the run-regress-* targets I looked at were marked phony.
> > >
> > > Index: share/mk/bsd.regress.mk
> > > ===
> > > RCS file: /cvs/src/share/mk/bsd.regress.mk,v
> > > retrieving revision 1.24
> > > diff -u -p -r1.24 bsd.regress.mk
> > > --- share/mk/bsd.regress.mk 31 Aug 2021 23:33:05 - 1.24
> > > +++ share/mk/bsd.regress.mk 2 Dec 2022 11:54:43 -
> > > @@ -32,8 +32,10 @@ _REGRESS_TMP?=/dev/null
> > > _REGRESS_OUT= | tee -a ${REGRESS_LOG} ${_REGRESS_TMP} 2>&1 > /dev/null
> > >
> > > .for p in ${PROG} ${PROGS}
> > > +. if !target(run-regress-$p)
> > > run-regress-$p: $p
> > > ./$p
> > > +. endif
> > > .PHONY: run-regress-$p
> > > .endfor
> > >
> > >
> > >
> > I don't think it's correct, I would separate the dependency on $p,
> > which we almost certainly want anyway, and only do the
> > . if !target dance for the actual commands.
>
> Sorry, this is a bit too cryptic for me. What does 'separate the
> dependency on $p' mean?
>
> > I haven't used it all that much, but we stole
> > .if commands() from NetBSD back in 2012 precisely to allow this kind of
> > stuff
>
> Do you mean this?
>
> Index: bsd.regress.mk
> ===
> RCS file: /cvs/src/share/mk/bsd.regress.mk,v
> retrieving revision 1.24
> diff -u -p -r1.24 bsd.regress.mk
> --- bsd.regress.mk31 Aug 2021 23:33:05 - 1.24
> +++ bsd.regress.mk2 Dec 2022 12:59:03 -
> @@ -33,7 +33,9 @@ _REGRESS_OUT= | tee -a ${REGRESS_LOG} ${
>
> .for p in ${PROG} ${PROGS}
> run-regress-$p: $p
> +. if !commands(run-regress-$p)
> ./$p
> +. endif
> .PHONY: run-regress-$p
> .endfor
>
>
Yep! Now all it requires is testing it doesn't break a thing.
Re: run-regress-* targets in bsd.regress.mk
On Fri, Dec 02, 2022 at 12:59:02PM +0100, Theo Buehler wrote:
> I wanted to override some of the default run-regress-* targets by
> declaring them in regress Makefiles. Many tests already do that and
> it happens to work as expected.
>
> However, this relies on behavior in the BUGS section of make. In fact,
> espie informs me that gmake behaves the other way around and honors the
> last definition (gmake also warns about redefinitions).
>
> To avoid this, I'd like to have bsd.regress.mk define these default
> targets only if they aren't already defined. This matches most targets
> in the other *.mk files.
>
> I kept the .PHONY target outside of the .if block since most regress
> Makefiles don't use .PHONY at all although they often should. In fact,
> none of the run-regress-* targets I looked at were marked phony.
>
> Index: share/mk/bsd.regress.mk
> ===
> RCS file: /cvs/src/share/mk/bsd.regress.mk,v
> retrieving revision 1.24
> diff -u -p -r1.24 bsd.regress.mk
> --- share/mk/bsd.regress.mk 31 Aug 2021 23:33:05 - 1.24
> +++ share/mk/bsd.regress.mk 2 Dec 2022 11:54:43 -
> @@ -32,8 +32,10 @@ _REGRESS_TMP?=/dev/null
> _REGRESS_OUT= | tee -a ${REGRESS_LOG} ${_REGRESS_TMP} 2>&1 > /dev/null
>
> .for p in ${PROG} ${PROGS}
> +. if !target(run-regress-$p)
> run-regress-$p: $p
> ./$p
> +. endif
> .PHONY: run-regress-$p
> .endfor
>
>
>
I don't think it's correct, I would separate the dependency on $p,
which we almost certainly want anyway, and only do the
. if !target dance for the actual commands.
I haven't used it all that much, but we stole
.if commands() from NetBSD back in 2012 precisely to allow this kind of stuff
non regression tests for some packaging scripts
I'd like to add some non-regression tests for some scripts, most notably
register-plist, which is... let's say not perfect at the moment.
I believe
${PORTSDIR}/regress is probably the simplest least disturbing way to do that.
Since the manpages already exist under src, I could also (somehow) put them
in /usr/src/regress.
Of course, this means requiring a checkout of the ports tree to check things.
What do you people think ?
tweak to dependency checking
I'd like to add a special construct to dependencies, "=" to mean
the exact version of the package we're depending on.
The patch is trivial:
Index: OpenBSD/PackingElement.pm
===
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/PackingElement.pm,v
retrieving revision 1.283
diff -u -p -r1.283 PackingElement.pm
--- OpenBSD/PackingElement.pm 28 Jun 2022 08:15:43 - 1.283
+++ OpenBSD/PackingElement.pm 1 Nov 2022 18:20:37 -
@@ -1079,7 +1079,13 @@ OpenBSD::Auto::cache(spec,
require OpenBSD::Search;
my $self = shift;
- return OpenBSD::Search::PkgSpec->new($self->{pattern})
+ my $src;
+ if ($self->{pattern} eq '=') {
+ $src = $self->{def};
+ } else {
+ $src = $self->{pattern};
+ }
+ return OpenBSD::Search::PkgSpec->new($src)
->add_pkgpath_hint($self->{pkgpath});
});
(This takes advantage of the fact that PkgSpec comparisons
have an implicit = if no operator has been mentionned)
This would result in lines like
@depend archivers/libarchive:=:libarchive-3.6.1p0
meaning "hey I want to depend on the exact version of libarchive
we have".
There are several reasons behind this patch.
- most of the stuff that wants exact version comparison
has to go through hoops to get it.
- register-plist can't flag changes in PKGSPEC as relevant, because...
with exact version comparisons, those change ALL THE TIME. I would
be able to say "hey, you changed PKGSPEC, you want to bump the
dependant port". Because, in the way we do things, PKGSPEC changes
are almost invariably to restrict/move the dependency along so that
we don't end up with broken dependency tree (case in point: the
introduction of gimp-3.x which broke all dependency chains for
gimp-2.x)
The only downside I see to this is that this will be the exact
version, including REVISION, whereas a few ports shun the REVISION
part while making the comparison.
Testing this patch on debug packages is trivial:
Index: bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1580
diff -u -p -r1.1580 bsd.port.mk
--- bsd.port.mk 1 Nov 2022 10:55:54 - 1.1580
+++ bsd.port.mk 3 Nov 2022 07:05:12 -
@@ -1203,7 +1203,7 @@ _pkg_cookie${_S} = ${_PACKAGE_COOKIE${_S
. if ${DEBUG_PACKAGES:M${_S}}
_DBG_PKG_ARGS${_S} := ${PKG_ARGS${_S}}
-_DBG_PKG_ARGS${_S} +=
-P${FULLPKGPATH${_S}}:${FULLPKGNAME${_S}}:${FULLPKGNAME${_S}}
+_DBG_PKG_ARGS${_S} += -P${FULLPKGPATH${_S}}:=:${FULLPKGNAME${_S}}
_DBG_PKG_ARGS${_S} += -DCOMMENT="debug info for ${PKGSTEM${_S}}"
_DBG_PKG_ARGS${_S} += -d"-debug info for ${FULLPKGNAME${_S}}"
# XXX revisit that fullpkgpath later ?
There is also a slightly more involved patch to extend the
*_DEPENDS = some/path>=2.1 glue so that
*_DEPENDS = some/path= works
Index: bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1580
diff -u -p -r1.1580 bsd.port.mk
--- bsd.port.mk 1 Nov 2022 10:55:54 - 1.1580
+++ bsd.port.mk 1 Nov 2022 18:20:54 -
@@ -1602,16 +1602,20 @@ ERRORS += "Fatal: old style depends ${_C
# the C,, part basically does this:
# if the depends contains only pkgpath>=something
# then we rebuild it as STEM->=something:pkgpath
+# also, pkgpath= becomes =:pkgpath
.for _v in BUILD LIB RUN TEST
${_v}_DEPENDS := ${${_v}_DEPENDS:C,^([^:]+/[^:<=>]+)([<=>][^:]+)$,STEM-\2:\1,}
+${_v}_DEPENDS := ${${_v}_DEPENDS:C,^([^:]+/[^:=]+)=[^:]*$,=:\1,}
.endfor
.for _v in BUILD TEST
${_v}_DEPENDS :=
${${_v}_DEPENDS:C,^([^:]+/[^:<=>]+)([<=>][^:]+)(:patch|:configure|:build)$,STEM-\2:\1\3,}
+${_v}_DEPENDS :=
${${_v}_DEPENDS:C,^([^:]+/[^:=]+)=[^:]*)(:patch|:configure|:build)$,=:\1\2,}
.endfor
.for _s in ${MULTI_PACKAGES}
. for _v in RUN LIB
${_v}_DEPENDS${_s} :=
${${_v}_DEPENDS${_s}:C,^([^:]+/[^:<=>]+)([<=>][^:]+)$,STEM-\2:\1,}
+${_v}_DEPENDS${_s} := ${${_v}_DEPENDS${_s}:C,^([^:]+/[^:=]+)=[^:]*$,=:\1,}
. endfor
.endfor
Obviously, this involves a bit of patience, because the pkg_add
change needs to trickle down to snapshots before the other parts
can go in.
Any objection to moving forward with this ?
Re: malloc: prep for immutable pages
On Wed, Oct 05, 2022 at 07:54:41AM -0600, Theo de Raadt wrote: > Marc Espie wrote: > > > On Tue, Oct 04, 2022 at 10:15:51AM -0600, Theo de Raadt wrote: > > > A note on why this chance is coming. > > > > > > malloc.c (as it is today), does mprotects back and forth between RW and > > > R, to protect an internal object. This object is in bss, it is not > > > allocated with mmap. With the upcoming mimmutable change, the bss will > > > become immutable by default, at program load time. mimmutable even > > > prevents > > > changing a RW object to R. > > > > I'm probably missing something here, but for me, traditionally, > > BSS is the "set to 0" section of global variables of a program... which are > > usually going to be changed to some other value. > > > > Or are we talking at cross purposes ? > > If you read the mimmutable diff, it has a manual page, and the answer is in > there. Ah my mistake, I read a bit fast, and I thought the *pages* themselves were immutable. Stupid question time: is there any reason not to allow further changes that would *restrict* the pages further ? A bit like pledge works. Like, say you mark a region "immutable" with RW rights, then later on use mprotect to mark it down RO, and you can never get back ?
Re: malloc: prep for immutable pages
On Tue, Oct 04, 2022 at 10:15:51AM -0600, Theo de Raadt wrote: > A note on why this chance is coming. > > malloc.c (as it is today), does mprotects back and forth between RW and > R, to protect an internal object. This object is in bss, it is not > allocated with mmap. With the upcoming mimmutable change, the bss will > become immutable by default, at program load time. mimmutable even prevents > changing a RW object to R. I'm probably missing something here, but for me, traditionally, BSS is the "set to 0" section of global variables of a program... which are usually going to be changed to some other value. Or are we talking at cross purposes ?
Re: wc(1): add -L flag to write length of longest line
On Fri, Sep 30, 2022 at 02:22:34AM +0200, Joerg Sonnenberger wrote: > On Thu, Sep 29, 2022 at 08:39:16PM +1000, Jonathan Gray wrote: > > wc counts items in files. Finding the longest item indeed sounds > > like a task better suited to awk. > > Finding outliers, means and counting are all parts of the same basic > class of operations. A good implementation of all of them requires > constant space and a fixed time per input character. An implementation > in awk will generally not have that property. Why ? is awk really that bad at managing memory ? Anyway, we have perl in base, and it will definitely fit the bill. Heck, I can do it in shell with constant space and a fixed time per input character. Real world computing 101: complexity is not all that matters, the constant in O(1) is often what will actually kill you. Real world computing 102: any utility left unfettered for long enough will grow a lisp interpreter and go into a mud-fight with emacs.
ports tree FULLPATH convention
This is something I added in 2011 (I think) to bsd.port.subdir.mk
if FULLPATH=Yes, a FULLPKGPATH like
SUBDIR=cat/location
will give you the EMPTY flavor for that location instead of the DEFAULT flavor.
I'm now questionning whether this actually makes senses considering the changes
I was making at the time.
Specifically: after setting up everything for a dependency, I started calling
"make _print-metadata" on each dependency, which yields the triplet:
"FULLPKGNAME PKGSPEC FULLPKGPATH"
(where FULLPKGPATH is normalized, so there are no "default flavors" whatsoever
but everything is fully spelled out everytime instead)
I'm now thinking that this is a useless remnant of a former age: most stuff
(if not all) that asks for dependencies is going to get through this.
The main entry point to look for this is "print-package-args" in bsd.port.mk
whiche yields the run-depends-args dependency, and "lib-depends-args" +
"wantlib-args"
(both go through variables for various reasons, the main one being that
lib-depends-args trims stuff to actually needed libs, whereas check-lib-depends
needs a "fuller" gamut just in case).
The following patch will completely neuter FULLPATH.
There are a few uses of this in the ports tree. The main one being pkg_create.
If this actually breaks, register-plist will be very unhappy in a bulk build.
It's also used in update-plist (FULLPATH=No, lol) and in check-lib-depends
and pkg_outdated, so it might be that some usage will break.
I do remember changing all this over a short period of time, but I can't
seem to recall any reason FULLPATH should still be relevant.
So, if possible I would very much like to get rid of it along with some of
the added complexity.
If I don't get any negative reports under (say) a week, I will consider
committing this and waiting for the shit to hit the fan.
Index: bsd.port.subdir.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.subdir.mk,v
retrieving revision 1.111
diff -u -p -r1.111 bsd.port.subdir.mk
--- bsd.port.subdir.mk 17 Dec 2018 18:06:05 - 1.111
+++ bsd.port.subdir.mk 5 Jul 2022 14:10:09 -
@@ -56,13 +56,6 @@ ARCH ?!= uname -m
ECHO_MSG ?= echo
-FULLPATH ?= No
-.if ${FULLPATH:L} == "yes"
-_FULLPATH = true
-.else
-_FULLPATH = false
-.endif
-
# create a full list of SUBDIRS...
.if empty(PKGPATH)
_FULLSUBDIR := ${SUBDIR}
@@ -113,7 +106,7 @@ _subdir_fragment = \
esac; \
fi; \
${_SKIP_STUFF}; \
- sawflavor=${_FULLPATH}; \
+ sawflavor=false; \
if ${_pflavor_fragment}; then \
eval $${echo_msg} "===\> $$subdir"; \
if ! (eval $$toset exec ${MAKE} $$target); then \
pkg_create tweak for PORTSDIR_PATH
As noticed by landry, going to the ports tree for dependencies can fail
hilariously in case of a PORTSDIR_PATH.
Most specifically, assuming you are building something like firefox-102
in one of your ports directory, when building the debug package,
dependency look-up in pkg_create is going to follow PORTSDIR_PATH, thus
ending up possibly in another firefox directory which contains firefox-100.
And since debug packages dependencies are tight, this will fail every time.
We're already checking for dependency pkgpath matching the pkgpath we're
building, so it's mostly a question of deducing the ports root from the
current directory.
and overriding PORTSDIR_PATH in ask_tree.
(another possibility would be to actively parse the dependency, not go back
to the ports tree, and just set FLAVOR and SUBPACKAGE correctly, but this
is somewhat more complicated and this appears to work).
Note the "Overriding..." print. This is intended as checking that things work
as expected and not intended for commit.
(I've also added an extra check on plist->pkgname and error message display
while debugging that thingy, along with moving the full action tweaks in
the father, so the messages reflect what has actually been run)
Okay ?
Index: OpenBSD/PkgCreate.pm
===
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/PkgCreate.pm,v
retrieving revision 1.182
diff -u -p -r1.182 PkgCreate.pm
--- OpenBSD/PkgCreate.pm28 Jun 2022 09:01:45 - 1.182
+++ OpenBSD/PkgCreate.pm28 Jun 2022 09:59:24 -
@@ -980,6 +980,7 @@ sub really_solve_dependency
$state->progress->message($dep->{pkgpath});
my $v;
+ my $path;
# look in installed packages, but only for different paths
my $p1 = $dep->{pkgpath};
@@ -987,7 +988,16 @@ sub really_solve_dependency
$p1 =~ s/\,.*//;
$p2 =~ s/\,.*//;
$p2 =~ s,^debug/,,;
- if ($p1 ne $p2) {
+ if ($p1 eq $p2) {
+ # try to figure out where we live
+ require Cwd;
+ my $dir = Cwd::getcwd();
+ if (defined $dir) {
+ if ($dir =~ s,/\Q$p1\E$,,) {
+ $path = $dir;
+ }
+ }
+ } else {
# look in installed packages
$v = $self->find_dep_in_installed($state, $dep);
}
@@ -997,7 +1007,7 @@ sub really_solve_dependency
# and in portstree otherwise
if (!defined $v) {
- $v = $self->solve_from_ports($state, $dep, $package);
+ $v = $self->solve_from_ports($state, $dep, $package, $path);
}
return $v;
}
@@ -1032,13 +1042,19 @@ sub to_cache
sub ask_tree
{
- my ($self, $state, $pkgpath, $portsdir, $data, @action) = @_;
+ my ($self, $state, $pkgpath, $portsdir, $path, $data, @action) = @_;
my $make = OpenBSD::Paths->make;
my $errors = OpenBSD::Temp->file;
if (!defined $errors) {
$state->fatal(OpenBSD::Temp->last_error);
}
+ push(@action, 'PORTS_PRIVSEP=No');
+ if (defined $path) {
+ push(@action, "PORTSDIR_PATH=$path");
+ $state->say("Overriding PORTSDIR_PATH=#1 for dependency #2",
+ $path, $pkgpath);
+ }
my $pid = open(my $fh, "-|");
if (!defined $pid) {
$state->fatal("cannot fork: #1", $!);
@@ -1059,7 +1075,6 @@ sub ask_tree
$( = $); $< = $>;
# XXX we're already running as ${BUILD_USER}
# so we can't do this again
- push(@action, 'PORTS_PRIVSEP=No');
$DB::inhibit_exit = 0;
exec $make ('make', @action);
}
@@ -1067,7 +1082,7 @@ sub ask_tree
while(<$fh>) { # XXX avoid spurious errors from child
}
close($fh);
- if ($? != 0) {
+ if ($? != 0 || !defined $plist || !defined $plist->pkgname) {
$state->errsay("child running '#2' failed: #1",
$state->child_error,
join(' ', 'make', @action));
@@ -1084,7 +1099,7 @@ sub ask_tree
sub really_solve_from_ports
{
- my ($self, $state, $dep, $portsdir) = @_;
+ my ($self, $state, $dep, $portsdir, $path) = @_;
my $diskcache = $self->diskcachename($dep);
my $plist;
@@ -1093,6 +1108,7 @@ sub really_solve_from_ports
$plist = OpenBSD::PackingList->fromfile($diskcache);
} else {
$plist = $self->ask_tree($state, $dep->{pkgpath}, $portsdir,
+ $path,
\&OpenBSD::PackingList::PrelinkStuffOnly,
'print-plist-libs-with-depends',
'wantlib_args=no-wantlib-args');
@@ -1113,7 +1129,7 @@ my $cache = {};
sub solve_from_ports
{
- my ($self, $state, $dep, $package) = @_;
+ my ($self, $state, $dep, $package, $pat
Re: m4: gnu-m4 compat draft
On Wed, Jun 08, 2022 at 09:44:19PM +0200, Marc Espie wrote:
> Naddy told me about an app that wants a gnu-m4 extension that
> requires >9 arguments to macros.
>
> I wrote a very quick patch that seems to do the work. There are probably
> lots of kinks to work out, it's been very lightly tested.
> (in particular, I haven't looked at stuff like $* and friends yet, maybe
> they work, maybe they won't)
>
> But if anyone requires this type of functionality, I'd like them to chime
> in.
>
>
> Index: eval.c
> ===
> RCS file: /cvs/src/usr.bin/m4/eval.c,v
> retrieving revision 1.78
> diff -u -p -r1.78 eval.c
> --- eval.c28 Jun 2019 05:35:34 - 1.78
> +++ eval.c8 Jun 2022 13:03:29 -
> @@ -40,6 +40,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -542,6 +543,21 @@ expand_macro(const char *argv[], int arg
> p++;
> p--; /* last character of defn */
> while (p > t) {
> + if (mimic_gnu && isdigit(*p)) {
> + const char *pos = p;
> + int mult = 1;
> + argno = 0;
> + while (isdigit(*pos) && pos > t) {
> + argno += mult * (*pos - '0');
> + mult *= 10;
> + pos--;
> + }
> + if (*pos == ARGFLAG && argno < argc -1) {
> + pbstr(argv[argno + 1]);
> + p = pos-1;
> + continue;
> + }
> + }
> if (*(p - 1) != ARGFLAG)
> PUSHBACK(*p);
> else {
>
>
That code appears to work just fine and we even have a trivial regress test.
In the mean time, naddy@ tells me that the port in question moved away from
using that specific gnu-m4 extension.
On one side, that code is 100% self-contained and safe for non gnu-m4 compat
mode. And it went through a full bulk build without any issue.
On the other side, I don't know if some other stuff might rely on it
eventually. Among other things it is quite possible someone wrote an autoconf
macro somewhere that actually requires over 9 parameters.
So the question is: do I shelve it, or commit it ? I don't really care all
that much either way, I would just love to close the subject.
m4: gnu-m4 compat draft
Naddy told me about an app that wants a gnu-m4 extension that
requires >9 arguments to macros.
I wrote a very quick patch that seems to do the work. There are probably
lots of kinks to work out, it's been very lightly tested.
(in particular, I haven't looked at stuff like $* and friends yet, maybe
they work, maybe they won't)
But if anyone requires this type of functionality, I'd like them to chime
in.
Index: eval.c
===
RCS file: /cvs/src/usr.bin/m4/eval.c,v
retrieving revision 1.78
diff -u -p -r1.78 eval.c
--- eval.c 28 Jun 2019 05:35:34 - 1.78
+++ eval.c 8 Jun 2022 13:03:29 -
@@ -40,6 +40,7 @@
*/
#include
+#include
#include
#include
#include
@@ -542,6 +543,21 @@ expand_macro(const char *argv[], int arg
p++;
p--; /* last character of defn */
while (p > t) {
+ if (mimic_gnu && isdigit(*p)) {
+ const char *pos = p;
+ int mult = 1;
+ argno = 0;
+ while (isdigit(*pos) && pos > t) {
+ argno += mult * (*pos - '0');
+ mult *= 10;
+ pos--;
+ }
+ if (*pos == ARGFLAG && argno < argc -1) {
+ pbstr(argv[argno + 1]);
+ p = pos-1;
+ continue;
+ }
+ }
if (*(p - 1) != ARGFLAG)
PUSHBACK(*p);
else {
@extraunexec usage in pkg_add
I've conducted a quick audit of the 300 or so uses of @extraunexec
in pkg_add.
All but 5 are actually glorified versions of
rm -rf
I propose eventually replacing them with
@extraglob pattern
with several advantages:
- we have file names that we know
- perl itself can expand the glob and decide accordingly to delete
or not, warn or not.
In general, stuff like @extra, @sample should require running (if
necessary) near the end. There were several things simple-minded
with the current approach.
- extra would spew out messages during replacement which is not
always appropriate.
- I need to look again at @sample and sysmerge, it should be possible
to provide a 3-way merge in case things changes.
- more generally, the whole shebang dates back to a simple era, where
adding/deleting packages were atomic operations. @extra actually
needs to take the update into account.
I have a first patch for @extra that takes updates into account:
this will remove spurious deinstallation messages coming from @extra.
The idea is that we register @extra globally during an update/deletion,
so that we only warn about @extra that actually got removed.
(there is more text after this). This patch is currently being tested
with no ill effect, and will probably be committed shortly.
Index: OpenBSD/Delete.pm
===
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/Delete.pm,v
retrieving revision 1.164
diff -u -p -r1.164 Delete.pm
--- OpenBSD/Delete.pm 6 Jun 2022 07:57:21 - 1.164
+++ OpenBSD/Delete.pm 6 Jun 2022 10:36:24 -
@@ -645,16 +645,14 @@ use File::Basename;
sub delete
{
my ($self, $state) = @_;
+ return if defined $state->{current_set}{known_extra}{$self->fullname};
my $realname = $self->realname($state);
if ($state->verbose >= 2 && $state->{extra}) {
$state->say("deleting extra file: #1", $realname);
}
return if $state->{not};
return unless -e $realname or -l $realname;
- if ($state->replacing) {
- $state->log("Remember to update #1", $realname);
- $self->mark_dir($state);
- } elsif ($state->{extra}) {
+ if ($state->{extra}) {
unlink($realname) or
$state->say("problem deleting extra file #1: #2",
$realname, $!);
} else {
@@ -669,8 +667,8 @@ sub delete
{
my ($self, $state) = @_;
return unless $state->{extra};
+ return if defined $state->{current_set}{known_extra}{$self->fullname};
my $realname = $self->realname($state);
- return if $state->replacing;
if ($state->{extra}) {
$self->SUPER::delete($state);
} else {
One reason for moving @extra handling near the end is the actual
usage: @extra (and @extraglob) allow registering package generated
data (or user files) that are not part of the package. Putting this
*after* running tags but *before* removing dirs means we can register
files generated by tags in there! stuff like pkg_check would know
they exist in the file system, but they would (hopefully) have actually
vanished during the end run.
As for @extraunexec, there are two approaches:
- I could "parse" the @extraunexec line manually and replace it
(internally) with a globbing approach. While possible (this is indeed
what I did for the audit) this is brittle. There are already somewat
gratuitous variations of
rm -rf, rm -fr, rm -r 2>/dev/null || true
in the tree. It will take only so long until one pattern is not
recognized and handled the same.
I propose instead a new keyword @extraglob which would replace most
of the rm -rf patterns.
Semantics is not yet fully decided, but I can add the code to
recognize it right now, so that when it becomes actual, most people
already have the keyword in their version of pkg_add.
Index: OpenBSD/PackingElement.pm
===
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/PackingElement.pm,v
retrieving revision 1.281
diff -u -p -r1.281 PackingElement.pm
--- OpenBSD/PackingElement.pm 26 May 2022 21:08:52 - 1.281
+++ OpenBSD/PackingElement.pm 7 Jun 2022 07:36:13 -
@@ -1817,6 +1817,13 @@ sub destate
&OpenBSD::PackingElement::Extra::destate;
}
+package OpenBSD::PackingElement::ExtraGlob;
+our @ISA=qw(OpenBSD::PackingElement::FileObject);
+
+sub keyword() { 'extraglob' }
+sub absolute_okay() { 1 }
+__PACKAGE__->register_with_factory;
+
package OpenBSD::PackingElement::SpecialFile;
our @ISA=qw(OpenBSD::PackingElement::Unique);
some package oddities
Due to some slowdown in updating texlive, I've looked up duplicate files inside packages. Most packages are okay, a few are not... The following list shows the maximum number of times a file appears in a given package (just looking at its checksum) Note sdcc, which stores a crazy number of identical .deps files (all 8 bytes long) I've stopped the list at count 50, but there are still a large number of duplicated files. (we're talking actual copies on the filesystem, pkg_create will recognize symlinks and hardlinks and package them accordingly) Those are not necessarily wrong, but it would be lovely if some of the maintainers could look them up (myself included thanks to stepmania). Right now, updating sdcc is probably way slower than deleting and reinstalling it ;) sdcc-3.8.0p0.tgz: 6402 ansible-5.8.0.tgz: 840 texlive_texmf-full-2021.tgz: 663 google-cloud-sdk-386.0.0.tgz: 588 coq-8.13.2p2.tgz: 580 lumina-1.4.0pl1p6.tgz: 381 mate-utils-1.26.0p0.tgz: 336 ruby27-gettext-3.2.2.tgz: 334 ruby31-gettext-3.2.2.tgz: 334 py3-vsphere-automation-7.0.3.2.tgz: 267 routersploit-3.4.0p2.tgz: 240 py3-opengl-3.1.3b2p3.tgz: 207 py-opengl-3.1.3b2p3.tgz: 207 mate-panel-1.26.0p0.tgz: 193 logstash-7.10.0v0.tgz: 185 qcad-3.24.3.0p2.tgz: 178 stepmania-5.0.12v0.tgz: 178 ruby30-passenger-6.0.4.tgz: 167 ruby31-passenger-6.0.4.tgz: 167 egoboo-2.8.1p0.tgz: 137 tuxpaint-stamps-20211125.tgz: 135 pycharm-2022.1.1.tgz: 134 texlive_texmf-minimal-2021.tgz: 121 javahelp-2.0.05p3.tgz: 121 granite-5.5.0.tgz: 121 py3-jedi-0.18.1.tgz: 120 mariadb-tests-10.6.7p0v1.tgz: 109 mate-control-center-1.26.0p2.tgz: 108 stellarium-0.22.1.tgz: 105 kdoctools-5.91.0.tgz: 103 odoo-15.0.20220427p0.tgz: 102 mate-calc-1.26.0.tgz: 102 kibana-7.10.0.tgz: 102 atril-1.26.0p0.tgz: 102 arduino-esp32-2.0.1.tgz: 101 megaglest-data-3.13.0p1.tgz: 98 warmux-11.04.1p9.tgz: 95 py3-botocore-1.26.0.tgz: 94 py3-pandas-1.3.4p0.tgz: 94 nextcloud-23.0.4.tgz: 92 neverball-data-1.6.0v0.tgz: 92 nextcloud-21.0.9.tgz: 92 nextcloud-22.2.7.tgz: 92 heimdal-devel-docs-7.7.0p0.tgz: 83 mate-power-manager-1.26.0p1.tgz: 82 epic4-2.10.5p3.tgz: 81 fillets-ng-1.0.1p2.tgz: 81 yaru-22.04.4p0.tgz: 80 qt6-qtdeclarative-6.3.0.tgz: 80 py3-sympy-1.7.1p0.tgz: 79 mate-system-monitor-1.26.0p0.tgz: 74 solr-8.11.1p0.tgz: 70 librenms-22.4.1v0.tgz: 68 ghidra-9.1.2p1.tgz: 67 widelands-1.0.tgz: 67 nagios-web-4.4.6p0-chroot.tgz: 63 nagios-web-4.4.6p0.tgz: 63 fs-uae-launcher-3.0.5p0.tgz: 60 os-test-0.41p1.tgz: 58 chromium-101.0.4951.67.tgz: 55 unknown-horizons-2019.1p0.tgz: 54 iridium-2022.04.100.0p1.tgz: 53 py3-scikit-learn-1.0.2.tgz: 53 botan2-2.19.1p1.tgz: 53 electron-8.2.0p5.tgz: 53 libreoffice-java-7.3.3.2v0.tgz: 52 qt6-qtbase-6.3.0.tgz: 51 [...]
Re: Picky, but much more efficient arc4random_uniform!
On Sat, May 21, 2022 at 07:47:40AM -0500, Luke Small wrote: > Perhaps it was rude sending off list stuff to the list. Your email sounded > "less than friendly" and more of a professional challenge that you were > definitely in the works to produce; much like Damien Miller’s challenge to > prove correctness. So, whatever. You're about one step away from getting into my spam rules. Whatever you say, the way you say it, *repeatedly* when all the people I know (me included) tell you to get your facts straight so you don't sound like a wacko, makes reading you a total waste of time.
Re: [PATCH] xenocara: change xterm ProcGetCWD to get cwd via sysctl instead of procfs
On Sun, May 15, 2022 at 08:46:00PM -0600, Theo de Raadt wrote: > You'll turn into a smurf before you finish auditing xterm perfectly. Should be in fortune ;)
Re: Picky, but much more efficient arc4random_uniform!
Random generators have been analyzed for years. Pick up "Concrete mathematics" by Don E. Knuth, read it, then come back with actual science.
pkg_add: better correspondence of file names
In the old pkg_add model (prior to 7.1), we create tmp files anyway, so the
code is fine.
7.1 and later, we try to avoid creating temp files, so a better name matching
correspondence would be cool
What this patch does:
the old code creates a simple hash where we associate ONE packing-list element
with a given sha256 value.
the new code creates a list of entries for an sha256 hash and iterates
over them.
Of course, it's unlikely two entries with the same sha256 will have different
content (though we still check the size, just in case, and -Dchecksum is
really paranoid about it and also really slow).
BUT, even if we find a good match, we will keep going... until we find
the exact same name ! (or settle on the last one we saw if we don't)
[... which means no temp file creation]
It might be quite accidental, but python has lots of __init__.py files with
0 size, and ALSO a significant amount of file with non empty content, but which
are identical (for reasons...? I don't want to know why the python guys think
it's a good idea to ship LOTS of identical files, and I don't mean symlinks
or hardlinks but actual copies)
As a measure point, on the python 3.9 package, without this diff, we
misidentify ~500 names out of 2700, and so churn 500 temp files if we
replace python with itself.
I haven't measured data points for other packages, but I would guess most
python packages are probably similarly afflicted (yeah, not affected,
I think it's a disease :D )
Please test.
Something else might have been fucked up.
Index: OpenBSD/PkgAdd.pm
===
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/PkgAdd.pm,v
retrieving revision 1.130
diff -u -p -r1.130 PkgAdd.pm
--- OpenBSD/PkgAdd.pm 27 Apr 2022 15:27:45 - 1.130
+++ OpenBSD/PkgAdd.pm 5 May 2022 17:55:01 -
@@ -84,7 +84,7 @@ sub hash_files
my ($self, $sha, $state) = @_;
return if $self->{link} or $self->{symlink} or $self->{nochecksum};
if (defined $self->{d}) {
- $sha->{$self->{d}->key} = $self;
+ push @{$sha->{$self->{d}->key}}, $self;
}
}
@@ -94,13 +94,17 @@ sub tie_files
return if $self->{link} or $self->{symlink} or $self->{nochecksum};
# XXX python doesn't like this, overreliance on timestamps
return if $self->{name} =~ m/\.py$/ && !defined $self->{ts};
- if (defined $sha->{$self->{d}->key}) {
- my $tied = $sha->{$self->{d}->key};
- # don't tie if there's a problem with the file
- my $realname = $tied->realname($state);
- return unless -f $realname;
- # and do a sanity check that this file wasn't altered
- return unless (stat _)[7] == $self->{size};
+ if (exists $sha->{$self->{d}->key}) {
+ my ($tied, $realname);
+ for my $c (@{$sha->{$self->{d}->key}}) {
+ # don't tie if there's a problem with the file
+ my $realname = $c->realname($state);
+ next unless -f $realname;
+ # and do a sanity check that this file wasn't altered
+ next unless (stat _)[7] == $self->{size};
+ $tied = $c;
+ last if $tied->name eq $self->name;
+ }
if ($state->defines('checksum')) {
my $d = $self->compute_digest($realname, $self->{d});
# XXX we don't have to display anything here
Re: clang-local.1: document support for source-based code coverage
On Wed, May 04, 2022 at 07:43:35AM -0400, Bryan Steele wrote: > On Wed, May 04, 2022 at 01:20:10PM +0200, Frederic Cambus wrote: > > Hi tech@, > > > > The base system includes the compiler-rt profile library for > > source-based code coverage. > > > > So here is a diff to document support in clang-local.1, the same > > way we document support for the ubsan_minimal sanitizer runtime > > which is also part of compiler-rt. > > > > Comments? OK? > > > > Index: share/man/man1/clang-local.1 > > === > > RCS file: /cvs/src/share/man/man1/clang-local.1,v > > retrieving revision 1.23 > > diff -u -p -r1.23 clang-local.1 > > --- share/man/man1/clang-local.118 Feb 2022 00:39:18 - 1.23 > > +++ share/man/man1/clang-local.14 May 2022 11:03:11 - > > @@ -99,6 +99,15 @@ See the documentation for the > > .Fl fsanitize-minimal-runtime > > flag. > > .It > > +The base system includes the compiler-rt profile library for > > +source-based code coverage. See the documentation for the > > +.Fl fprofile-instr-generate > > +and > > +.Fl fcoverage-mapping > > +flags. > > +Note that llvm-profdata and llvm-cov tools from devel/llvm are > > +required to process coverage data and produce reports. > > +.It > > The > > .Xr malloc 3 , > > .Xr calloc 3 , > > Isn't the purpose of the clang-local(1) to document local changes to the > system compiler, -fsanitize-minimal-runtime feels like a special case as > we do not have the complete sanitizer runtime. What do you suggest as a good location where people will find that information easily ?
Re: kbd -l error message
On Thu, Apr 14, 2022 at 04:28:37PM +0200, Alexander Bluhm wrote:
> Hi,
>
> When kbd -l is executed as regular user, it fails silently.
>
> $ kbd -l
> $ echo $?
> 0
>
> Error handling is a bit tricky. We want the first error if no
> device is available.
>
> $ ./kbd -l
> kbd: open /dev/wskbd[0-9]: Permission denied
> $ echo $?
> 1
>
> ok?
>
> bluhm
>
> Index: sbin/kbd/kbd_wscons.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sbin/kbd/kbd_wscons.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 kbd_wscons.c
> --- sbin/kbd/kbd_wscons.c 22 Jan 2020 06:24:07 - 1.34
> +++ sbin/kbd/kbd_wscons.c 14 Apr 2022 14:21:17 -
> @@ -150,7 +150,7 @@ kbd_list(void)
> {
> int kbds[SA_MAX];
> struct wskbd_encoding_data encs[SA_MAX];
> - int fd, i, kbtype, t;
> + int fd, i, kbtype, t, error = 0;
> chardevice[PATH_MAX];
>
> memset(kbds, 0, sizeof(kbds));
> @@ -162,7 +162,14 @@ kbd_list(void)
> fd = open(device, O_WRONLY);
> if (fd == -1)
> fd = open(device, O_RDONLY);
> - if (fd >= 0) {
> + if (fd == -1) {
> + /* remember the first error number */
> + if (error == 0)
> + error = errno;
> + } else {
> + /* at least one success, do not print error */
> + error = -1;
> +
> if (ioctl(fd, WSKBDIO_GTYPE, &kbtype) == -1)
> err(1, "WSKBDIO_GTYPE");
> switch (kbtype) {
> @@ -207,6 +214,10 @@ kbd_list(void)
> }
> close(fd);
> }
> + }
> + if (error > 0) {
> + errno = error;
> + err(1, "open /dev/wskbd[0-9]");
> }
>
> for (i = 0; i < SA_MAX; i++)
>
>
I was annoyed by this as well, so I welcome better code. It looks 99% okay
for me.
I'm not quite fond of the error reports, though... they could be more specific
about which device is doing what.
- we keep track of the first error, so it should probably talk
about /dev/wskbd0 directly ?
- by comparison, the message for the WSKBDIO_GTYPE doesn't mention the
device name. I think err(1, "WKBDIO_GTYPE on %s", device) might be slightly
more helpful.
Re: PATCH: speed up pkg_add
I got a new slightly different one.
Specifically, the old one has a "fun" failure mode in case
an old package got broken and is missing files.
The heuristics correctly deduces that the files are not
there, so it extracts in-place... then pkg_delete promptly
deletes what it believes to be files from the old package!
so the broken install stays broken.
The fix is fairly obvious: I store a hash of filenames not to
delete in the current_set, instead of doing it only for tied
objects (other objects we don't quite know where they live
in the old plist, nor do we care all that much)
(I could also probably check modes and owners for tied files
since we do a stat anyway, but new files will have to get
utime'd to the new ts, so I doubt I'll see any gain from that,
and it gets way too complicated for its own good)
Index: OpenBSD/Add.pm
===
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/Add.pm,v
retrieving revision 1.187
diff -u -p -r1.187 Add.pm
--- OpenBSD/Add.pm 9 Mar 2022 12:27:51 - 1.187
+++ OpenBSD/Add.pm 16 Mar 2022 10:33:19 -
@@ -466,6 +466,7 @@ sub find_safe_dir
my $fullname = $self->fullname;
my $filename = $state->{destdir}.$fullname;
my $d = dirname($filename);
+ my $orig = $d;
# we go back up until we find an existing directory.
# hopefully this will be on the same file system.
@@ -483,6 +484,12 @@ sub find_safe_dir
if (!-e _ && !$state->{not}) {
$state->make_path($d, $fullname);
}
+ if ($state->{current_set}{simple_update} &&
+ $d eq $orig &&
+ !-e $filename) {
+ $self->{avoid_temp} = $filename;
+ }
+
return $d;
}
@@ -503,6 +510,18 @@ sub create_temp
return ($fh, $tempname);
}
+sub may_create_temp
+{
+ my ($self, $d, $state) = @_;
+ if ($self->{avoid_temp}) {
+ if (open(my $fh, '>', $self->{avoid_temp})) {
+ return ($fh, $self->{avoid_temp});
+ }
+ }
+ delete $self->{avoid_temp};
+ return $self->create_temp($d, $state);
+}
+
sub tie
{
my ($self, $state) = @_;
@@ -513,14 +532,22 @@ sub tie
$self->SUPER::extract($state);
my $d = $self->find_safe_dir($state);
+ my $src = $self->{tieto}->realname($state);
+ my $dest = $self->realname($state);
+ if ($state->{current_set}{simple_update} && $src eq $dest) {
+ $state->say("No name change on tied file #1", $src)
+ if $state->verbose >= 3;
+ $state->{current_set}{dont_delete}{$dest} = 1;
+ $self->{avoid_temp} = 1;
+ return;
+ }
if ($state->{not}) {
$state->say("link #1 -> #2",
$self->name, $d) if $state->verbose >= 3;
} else {
- my ($fh, $tempname) = $self->create_temp($d, $state);
+ my ($fh, $tempname) = $self->may_create_temp($d, $state);
return if !defined $tempname;
- my $src = $self->{tieto}->realname($state);
unlink($tempname);
$state->say("link #1 -> #2", $src, $tempname)
if $state->verbose >= 3;
@@ -528,6 +555,7 @@ sub tie
}
}
+
sub extract
{
my ($self, $state, $file) = @_;
@@ -540,13 +568,16 @@ sub extract
$self->name, $d) if $state->verbose >= 3;
$state->{archive}->skip;
} else {
- my ($fh, $tempname) = $self->create_temp($d, $state);
- if (!defined $tempname) {
+ my ($fh, $filename) = $self->may_create_temp($d, $state);
+ if (!defined $filename) {
$state->{archive}->skip;
return;
}
- $state->say("extract #1 -> #2", $self->name, $tempname)
+ if ($self->{avoid_temp}) {
+ $state->{current_set}{dont_delete}{$filename} = 1;
+ }
+ $state->say("extract #1 -> #2", $self->name, $filename)
if $state->verbose >= 3;
@@ -555,7 +586,7 @@ sub extract
$self->stringize);
}
$file->extract_to_fh($fh);
- $self->may_check_digest($tempname, $state);
+ $self->may_check_digest($filename, $state);
}
}
@@ -576,17 +607,21 @@ sub install
} elsif (defined $self->{symlink}) {
symlink($self->{symlink}, $destdir.$fullname);
} else {
- if (!defined $self->{tempname}) {
- return if $state->allow_nonroot($fullname);
- $state->fatal("No tempname for #1", $fullname);
- }
- rename($self->{tempname}, $destdir.$fullname) or
- $state->fatal("can't move #1 to #2: #3",
-
PATCH: speed up pkg_add
I've changed pkg_add for updates and tied files a while back, which resulted
in creating a lot of temp files. The logic was complicated, and I didn't
want to make it more complex at the time until most of the issues were fixed.
Then we got tags, so that most packing-lists are innocuous these days.
Now is that time, the following patch may speed up some updates and additions
significantly, in case the packing-lists don't contain "random execs".
What this does:
- in case we extract a new file that's not on the filesystem, we can extract
in-place instead of going through a temporary item
- in case we are matching files with identical files during an update, and
the filename doesn't change, we acknowledge we don't need to do anything.
Caveat: this may bite. It is advised to run pkg_check afterwards and
report file system discrepancies.
Index: OpenBSD/Add.pm
===
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/Add.pm,v
retrieving revision 1.187
diff -u -p -r1.187 Add.pm
--- OpenBSD/Add.pm 9 Mar 2022 12:27:51 - 1.187
+++ OpenBSD/Add.pm 15 Mar 2022 10:08:07 -
@@ -466,6 +466,7 @@ sub find_safe_dir
my $fullname = $self->fullname;
my $filename = $state->{destdir}.$fullname;
my $d = dirname($filename);
+ my $orig = $d;
# we go back up until we find an existing directory.
# hopefully this will be on the same file system.
@@ -483,6 +484,12 @@ sub find_safe_dir
if (!-e _ && !$state->{not}) {
$state->make_path($d, $fullname);
}
+ if ($state->{current_set}{simple_update} &&
+ $d eq $orig &&
+ !-e $filename) {
+ $self->{avoid_temp} = $filename;
+ }
+
return $d;
}
@@ -503,6 +510,18 @@ sub create_temp
return ($fh, $tempname);
}
+sub may_create_temp
+{
+ my ($self, $d, $state) = @_;
+ if ($self->{avoid_temp}) {
+ if (open(my $fh, '>', $self->{avoid_temp})) {
+ return ($fh, $self->{avoid_temp});
+ }
+ }
+ delete $self->{avoid_temp};
+ return $self->create_temp($d, $state);
+}
+
sub tie
{
my ($self, $state) = @_;
@@ -513,14 +532,22 @@ sub tie
$self->SUPER::extract($state);
my $d = $self->find_safe_dir($state);
+ my $src = $self->{tieto}->realname($state);
+ my $dest = $self->realname($state);
+ if ($state->{current_set}{simple_update} && $src eq $dest) {
+ $state->say("No name change on tied file #1", $src)
+ if $state->verbose >= 3;
+ $self->{tieto}{dont_delete} = 1;
+ $self->{avoid_temp} = 1;
+ return;
+ }
if ($state->{not}) {
$state->say("link #1 -> #2",
$self->name, $d) if $state->verbose >= 3;
} else {
- my ($fh, $tempname) = $self->create_temp($d, $state);
+ my ($fh, $tempname) = $self->may_create_temp($d, $state);
return if !defined $tempname;
- my $src = $self->{tieto}->realname($state);
unlink($tempname);
$state->say("link #1 -> #2", $src, $tempname)
if $state->verbose >= 3;
@@ -528,6 +555,7 @@ sub tie
}
}
+
sub extract
{
my ($self, $state, $file) = @_;
@@ -540,13 +568,13 @@ sub extract
$self->name, $d) if $state->verbose >= 3;
$state->{archive}->skip;
} else {
- my ($fh, $tempname) = $self->create_temp($d, $state);
- if (!defined $tempname) {
+ my ($fh, $filename) = $self->may_create_temp($d, $state);
+ if (!defined $filename) {
$state->{archive}->skip;
return;
}
- $state->say("extract #1 -> #2", $self->name, $tempname)
+ $state->say("extract #1 -> #2", $self->name, $filename)
if $state->verbose >= 3;
@@ -555,7 +583,7 @@ sub extract
$self->stringize);
}
$file->extract_to_fh($fh);
- $self->may_check_digest($tempname, $state);
+ $self->may_check_digest($filename, $state);
}
}
@@ -576,17 +604,21 @@ sub install
} elsif (defined $self->{symlink}) {
symlink($self->{symlink}, $destdir.$fullname);
} else {
- if (!defined $self->{tempname}) {
- return if $state->allow_nonroot($fullname);
- $state->fatal("No tempname for #1", $fullname);
+ if (defined $self->{avoid_temp}) {
+ delete $self->{avoid_temp};
+ } else {
+ if (!defined $self->{tempname}) {
+ return if $state->allow_nonroot($fullname);
+
Re: atomic read write
I've been reading the newer C standard (n2310.pdf to be specific). That thing is scary. I believe that, in the long haul, we will have to use the Atomic types stuff that's apparently part of the C standard now. Yeah, the switch to multi-threading made the standard way more complicated. Sequence points were so much easier to explain. I'm not quite sure how Atomic types actually relate to SMP, but I'm afraid it's currently the "best" warranty we will be stuck with to try and get "better" atomic writes... I haven't even looked to see what clang does with it.
Re: A program compiled with '-pg' option always gets SEGV on its execution.
On Mon, Feb 21, 2022 at 05:36:16PM +0900, Yuichiro NAITO wrote: > Of course, all programs compiled without '-pg' work fine for me. > I found this issue when I profile my application with gprof(1). > For example, following example C source code fails to execute on OpenBSD 7.0. Profile is partly broken and has been for a while. Compiling with -static and removing any pledge() call allow profiling to work.
Re: fw_update perl glue tweaks
On Mon, Feb 14, 2022 at 11:37:55AM +0100, Marc Espie wrote:
> - you don't need to call unlock_db. When the process dies, the fd dies with
> it, and the lock.
>
> - I've committed glue to PackageInfo.pm to make lock_db self-contained
> (it will pull BaseState in on an "as needed" basis, which also comes in
> handy for other locks prior to having a state)
>
> Index: fw_update.sh
> ===
> RCS file: /cvs/src/usr.sbin/fw_update/fw_update.sh,v
> retrieving revision 1.37
> diff -u -p -r1.37 fw_update.sh
> --- fw_update.sh 11 Feb 2022 00:46:58 - 1.37
> +++ fw_update.sh 14 Feb 2022 10:35:57 -
> @@ -206,13 +206,11 @@ lock_db() {
> perl <<'EOL' |&
> use v5.16;
> use warnings;
> - use OpenBSD::PackageInfo qw< lock_db unlock_db >;
> - use OpenBSD::BaseState;
> + use OpenBSD::PackageInfo qw< lock_db >;
>
> $|=1;
>
> - lock_db(0, 'OpenBSD::BaseState');
> - END { unlock_db }
> + lock_db(0);
> $SIG{TERM} = sub { exit };
>
> say $$;
>
>
Extra note: I generally steer away from END {} blocks in my code in most
cases. Those blocks obviously survive fork... so whenever you need perl
code on both sides, you are asking for trouble.
(that's the reason why the OpenBSD::Temp registers files to clean with
the associated pid. I got bitten by this).
None of the default pkg_* bother to call unlock_db at the end... that hook
is there specifically if you want to peek at the db, and release the lock
before the end, nothing more.
fw_update perl glue tweaks
- you don't need to call unlock_db. When the process dies, the fd dies with
it, and the lock.
- I've committed glue to PackageInfo.pm to make lock_db self-contained
(it will pull BaseState in on an "as needed" basis, which also comes in
handy for other locks prior to having a state)
Index: fw_update.sh
===
RCS file: /cvs/src/usr.sbin/fw_update/fw_update.sh,v
retrieving revision 1.37
diff -u -p -r1.37 fw_update.sh
--- fw_update.sh11 Feb 2022 00:46:58 - 1.37
+++ fw_update.sh14 Feb 2022 10:35:57 -
@@ -206,13 +206,11 @@ lock_db() {
perl <<'EOL' |&
use v5.16;
use warnings;
- use OpenBSD::PackageInfo qw< lock_db unlock_db >;
- use OpenBSD::BaseState;
+ use OpenBSD::PackageInfo qw< lock_db >;
$|=1;
- lock_db(0, 'OpenBSD::BaseState');
- END { unlock_db }
+ lock_db(0);
$SIG{TERM} = sub { exit };
say $$;
Re: fw_update(8): lock pkg database while running
On Wed, Feb 09, 2022 at 07:30:46PM -0800, Andrew Hewus Fresh wrote:
> I was reminded that fw_update(8) updates the package database without
> locking currently. That can cause issues when running it concurrently
> with pkg_add, for example starting `pkg_add -u` in one terminal and
> `sysupgrade` in another.
>
> This diff checks to see if perl is available and if so starts a perl
> co-process that locks the database and then kills it when exiting.
>
> (Figuring out how to wait for the lock to be acquired and find out the
> pid of the co-process was a bit of a challenge, so if someone has a
> suggestion on improving that, I'm open to suggestions)
>
> Comments, OK?
>
>
> Index: usr.sbin/fw_update/fw_update.sh
> ===
> RCS file: /cvs/src/usr.sbin/fw_update/fw_update.sh,v
> retrieving revision 1.36
> diff -u -p -r1.36 fw_update.sh
> --- usr.sbin/fw_update/fw_update.sh 10 Feb 2022 00:29:32 - 1.36
> +++ usr.sbin/fw_update/fw_update.sh 10 Feb 2022 03:22:20 -
> @@ -42,11 +42,13 @@ INSTALL=true
> LOCALSRC=
>
> unset FTPPID
> +unset LOCKPID
> unset FWPKGTMP
> REMOVE_LOCALSRC=false
> cleanup() {
> set +o errexit # ignore errors from killing ftp
> [ "${FTPPID:-}" ] && kill -TERM -"$FTPPID" 2>/dev/null
> + [ "${LOCKPID:-}" ] && kill -TERM -"$LOCKPID" 2>/dev/null
> [ "${FWPKGTMP:-}" ] && rm -rf "$FWPKGTMP"
> "$REMOVE_LOCALSRC" && rm -rf "$LOCALSRC"
> [ -e "${CFILE}" ] && [ ! -s "$CFILE" ] && rm -f "$CFILE"
> @@ -194,6 +196,36 @@ firmware_devicename() {
> echo "$_d"
> }
>
> +lock_db() {
> + [ "${LOCKPID:-}" ] && return 0
> +
> + # The installer doesn't have perl, so we can't lock there
> + [ -e /usr/bin/perl ] || return 0
> +
> + set -o monitor
> + perl <<'EOL' |&
> + use v5.16;
> + use warnings;
> + use OpenBSD::PackageInfo qw< lock_db unlock_db >;
> + use OpenBSD::State;
> +
> + $|=1;
> +
> + lock_db(0, OpenBSD::State->new);
> + END { unlock_db }
> + $SIG{TERM} = sub { exit };
> +
> + say $$;
> + sleep;
> +EOL
> + set +o monitor
> +
> + read -rp LOCKPID
> +
> + return 0
> +}
> +
> +
> installed_firmware() {
> local _pre="$1" _match="$2" _post="$3" _firmware _fw
> set -sA _firmware -- $(
> @@ -401,6 +433,7 @@ set -sA devices -- "$@"
>
> if "$DELETE"; then
> [ "$OPT_F" ] && echo "Cannot use -F and -d" >&2 && usage
> + lock_db
>
> # Show the "Uninstall" message when just deleting not upgrading
> ((VERBOSE)) && VERBOSE=3
> @@ -463,6 +496,8 @@ else
> fi
>
> [ "${devices[*]:-}" ] || exit
> +
> +lock_db
>
> added=''
> updated=''
>
It's one of the cases where you can actually use the newer OpenBSD::BaseState
where I explicitly split the printing and system functions.
e.g.,
use OpenBSD::BaseState;
lock_db(0, 'OpenBSD::BaseState');
should work just as well
Re: perl clang -Wcompound-token-split-by-macro
On Tue, Feb 08, 2022 at 01:59:51PM +, Stuart Henderson wrote: > > > I think we should start updating Devel::PPPort in base. Then we > > > can regenerate other ppport.h in base and ports. Note that some ports > > > need newer Devel::PPPort than we have now. So updating seems > > > unavoidable. > > I do worry a bit that if we regenerate in ports, that some ports might > want an *older* version. (At least the regen might need to be made optional). > > Anyone know if FreeBSD has done anything for this? > That's funny, because basically that glue is supposed to make things *more portable*... so having it not work with a newer version would definitely mean we need to ask upstream to fix their software. eventually, it should settle down. Putting the blame in the right location so that it gets fixed for everyone looks to me to be the best course of action, unless it proves too time-consuming.
Re: perl clang -Wcompound-token-split-by-macro
Or we can automate this with something like this:
Index: perl.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/perl.port.mk,v
retrieving revision 1.32
diff -u -p -r1.32 perl.port.mk
--- perl.port.mk12 Dec 2021 19:25:39 - 1.32
+++ perl.port.mk21 Jan 2022 17:39:18 -
@@ -56,6 +56,11 @@ MODPERL_pre-configure = for f in ${MODPE
${MODPERL_BIN_ADJ} ${WRKSRC}/$${f}; done
.endif
+MODPERL_gen = cd ${WRKDIST} && \
+ if test -f ppport.h; then \
+ perl -MDevel::PPPort -e'Devel::PPPort::WriteFile'; \
+ fi
+
.if ${CONFIGURE_STYLE:L:Mmodbuild}
MODPERL_configure = \
cd ${WRKSRC}; ${SETENV} ${CONFIGURE_ENV} \
Re: perl clang -Wcompound-token-split-by-macro
On Fri, Jan 21, 2022 at 02:12:25PM +0100, Alexander Bluhm wrote:
> Hi,
>
> Since clang 13 each Perl or Perl XS module compile spits out a lot
> of -Wcompound-token-split-by-macro warnings. E.g. p5-Net-SSLeay
> produces 3882 warnings generated. You cannot spot anything useful.
> The problem is burried deeply in the Perl macros and copied to
> everywhere.
>
> If we compile Perl with -Wno-compound-token-split-by-macro it gets
> stored in Config.pm and is used for most modules.
>
> $ perl -MConfig -e 'print $Config{ccflags},"\n"'
> -Wno-compound-token-split-by-macro -fno-strict-aliasing
> -fno-delete-null-pointer-checks -pipe -fstack-protector-strong
> -I/usr/local/include
>
> ok?
I'm wondering whether this warning makes any sense for the ({ gnu C statement
(which isn't really a C token per se)
As for "deep within perl", it comes from a combination of two things:
- perl.h uses STMT_START and STMT_END (the normal ones just
creating do {} while (0) grouping.
- there's a module called Devel::PPPort that generates a ppport.h (normally
the most recent).
using
$ perl -MDevel::PPPort -e'Devel::PPPort::WriteFile'
as documented in the manpage to regen a more recent ppport.h results in
p5-Net-SSLeay producing exactly 0 warning while building.
So I don't really think perl requires any change.
Possibly hacking a bit on ports that use an outdated version of ppport.h
Re: locale in expr(1)
On Mon, Nov 15, 2021 at 05:58:38PM +0100, Ingo Schwarze wrote: > I don't know. A fairly reliable way to create security risks is > complexity. Apart from the erratic run time behaviour that is likely to > trip up sysadmins - LC_COLLATE can change the collation sequence even > among ASCII characters - collation support looks like a very effective > way of putting excessive complexity right into the C library. Yes, but it only affects added functions, so it's not that bad, imo. Contrary to LC_CTYPE, you have to explicitly use the locale functions if you want locale order... it doesn't change the semantics of plain old C comparison. As far as the complexity goes, it's supposed to be a text file containing collate information that you compile into rules... Some rules are really "fun", but I don't think it's that bad... and we could always lock it to actual languages by default.
Re: locale in expr(1)
On Mon, Nov 15, 2021 at 03:43:47PM +0100, Jan Stary wrote:
> Here's a try (see below);
> one sentence one line while here.
>
> I would also replace 'results' with 'result' everywhere,
> but I am not a native speaker.
>
> Jan
>
>
> On Nov 10 18:46:08, h...@stare.cz wrote:
> > On Nov 10 18:15:44, h...@stare.cz wrote:
> > > expr(1) says
> > >
> > > expr1 {=, >, >=, <, <=, !=} expr2
> > >
> > > Returns the results of integer comparison if both arguments
> > > are decimal integers; otherwise, returns the results of
> > > string comparison using the locale-specific collation
> > > sequence. The result of each comparison is 1 if the specified
> > > relation is true, or 0 if the relation is false.
> > >
> > > Looking at expr.c, it boils down to strcoll(), which ignores the locale.
> > > So the statement is technically true, but there isn't really any
> > > "locale-specific collation sequence".
> > >
> > > Would it be simpler to leave the mention of locale completely out?
> > > Or state something similar to what sort(1) or strcoll(3) and other
> > > string-comparing routines say?
> >
> > For example,
> >
> > $ expr č '<' d
> > 0
> >
> > Which locale-specific collation sequence determined that?
> > Byte by byte, it's
> >
> > c48d U+00010d č LATIN SMALL LETTER C HACEK
> > 64U+64 d LATIN SMALL LETTER D
> >
> > and I don't think there is anything more to it.
> > (Although in the Czech alphabet, č comes just before d.)
> >
> > Jan
I don't know what Ingo et al plans wrt collation sequence are, but
it is a relatively innocuous part of locales.
More specifically, the ISO committee introduced functions specifically
to deal with collation sequence, and left the original raw C functions
(strcmp and the like) completely pristine... so what's missing is mostly
an engine to compile location tables into something fairly reasonable.
There is way less of a security risk than, say, LC_CTYPE... ;)
PATCH: make pkg_delete a bit more finicky
We disabled the checksum check because it was rather expensive, but actually
we also store timestamps in packing-lists
This is a first draft at a better check. What this does:
- compare the fs timestamp with the packing-list timestamp. This should be
very cheap in most cases, since we're already inthe directory ready
to delete the file.
- IF the timestamp doesn't match, we go run the checksum before deleting the
file.
There might be some fringe cases I haven't seen, so see this more as a POC
than a finished diff, but I'm curious if there are still many packages
that diddle their installed bits after install.
feedback welcome.
Index: OpenBSD/Delete.pm
===
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/Delete.pm,v
retrieving revision 1.160
diff -u -p -r1.160 Delete.pm
--- OpenBSD/Delete.pm 24 Jul 2019 18:05:26 - 1.160
+++ OpenBSD/Delete.pm 30 Oct 2021 15:57:38 -
@@ -432,7 +432,21 @@ sub is_intact
$state->log("Couldn't delete #1 (no checksum)", $realname);
return 0;
}
- return 1 unless $state->defines('checksum');
+ my $force_checksum = 0;
+ if (defined $self->{ts}) {
+ my @s = stat $realname;
+ if (@s == 0) {
+ $state->say("Couldn't stat #1", $realname);
+ return 0;
+ }
+ if ($s[9] != $self->{ts}) {
+ $force_checksum = 1;
+ $state->say("Found possibly modified file #1",
$realname);
+ }
+ }
+ if (!$force_checksum) {
+ return 1 unless $state->defines('checksum');
+ }
my $d = $self->compute_digest($realname, $self->{d});
return 1 if $d->equals($self->{d});
if ($self->fullname eq $realname) {
Re: Exit status of pkg_add
On Tue, Oct 19, 2021 at 10:42:04AM +0900, Yuichiro NAITO wrote:
> Following patch changes pkg_add to return a error code,
> if a package name is wrong.
>
> diff --git a/usr.sbin/pkg_add/OpenBSD/AddDelete.pm
> b/usr.sbin/pkg_add/OpenBSD/AddDelete.pm
> index 7a968cbf05d..39bee874ff1 100644
> --- a/usr.sbin/pkg_add/OpenBSD/AddDelete.pm
> +++ b/usr.sbin/pkg_add/OpenBSD/AddDelete.pm
> @@ -403,12 +403,13 @@ sub check_root
> sub choose_location
> {
> my ($state, $name, $list, $is_quirks) = @_;
> if (@$list == 0) {
> if (!$is_quirks) {
> $state->errsay("Can't find #1", $name);
> + $state->{bad}++;
> $state->run_quirks(
> sub {
> my $quirks = shift;
> $quirks->filter_obsolete([$name], $state);
> });
> }
>
> Is it OK?
>
> On 10/18/21 16:53, Yuichiro NAITO wrote:
> > Hi, I have a question about exit status of pkg_add command.
> >
> > When I wrote a package install script which included typo in a package name
> > (of course it's my fault), the script didn't stop in spite of `set -e`.
> >
> > Because pkg_add command returns 0 even if a package name is wrong.
> > Is this exit status intended or design policy of pkg_add command?
> >
> > If not, I want a error status getting returned.
> > It will save my time to look for a typo or similar bug.
> >
> > I can't see 'EXIT STATUS' section in the pkg_add manual of OpenBSD 7.0.
> > So, I e-mailed this question.
> >
Basically, there are a few traps in pkg_add wrt exit status.
The main trap is that making pkg_add error out in cases it didn't requires
testing, because various automated situations that already exist might fail.
I will most probably fix that one, assuming there is no fallout.
Re: Thread Local Storage in clang
On Fri, May 28, 2021 at 06:56:42PM +0200, Marc Espie wrote: > On Fri, May 28, 2021 at 07:55:39AM -0700, Chris Cappuccio wrote: > > I tried to compile librdkafka on OpenBSD 6.5 (clang 7.0.1) and clang > > compiled > > the __thread parts with some built-in mechanism. I upgraded the system to > > OpenBSD 6.9 and TLS is no longer supported by the in-tree clang. Was this > > intended to be turned off? Did the 6.5 version even work? Is Thread Local > > Storage support in clang desired in general or no? Marc Espie's clang > > presentation from July 2017 mentions that we do and that Mark Kettenis > > helped make it happen. I'm having a hard time finding much other discussion > > either way. > > looks like some configure issue most probably. > > We never had full support for tls, but instead emulated tls on some platforms. > > This does cause some trouble with some shared libraries, apparently (though > the problem is a bit larger in general) but if emulated tls was broken, most > of the ports tree would fall apart these days. Thinking some more about it, I would suspect the configury stuff in that library to expect native support... tls doesn't work on OpenBSD unless you respect the toolchain. in particular, it won't fly without the support library (the emutls stuff in libc++abi) clang platforms aren't incredibly common. It definitely looks like the most likely explanation.
Re: Thread Local Storage in clang
On Fri, May 28, 2021 at 07:55:39AM -0700, Chris Cappuccio wrote: > I tried to compile librdkafka on OpenBSD 6.5 (clang 7.0.1) and clang compiled > the __thread parts with some built-in mechanism. I upgraded the system to > OpenBSD 6.9 and TLS is no longer supported by the in-tree clang. Was this > intended to be turned off? Did the 6.5 version even work? Is Thread Local > Storage support in clang desired in general or no? Marc Espie's clang > presentation from July 2017 mentions that we do and that Mark Kettenis > helped make it happen. I'm having a hard time finding much other discussion > either way. looks like some configure issue most probably. We never had full support for tls, but instead emulated tls on some platforms. This does cause some trouble with some shared libraries, apparently (though the problem is a bit larger in general) but if emulated tls was broken, most of the ports tree would fall apart these days.
Re: emutls and dlopen(3) problem - Re: patch: make ld.so aware of pthread_key_create destructor - Re: multimedia/mpv debug vo=gpu crash on exit
So, is there a way to get a work-around ? or should we push for proper tls sooner than later ? It always made me queasy having all those programs segfaulting on exit. Not sure it's exploitable, but giving people the impression it's okay to segfault. I thought it might be bad code in the programs, turns out it's our infrastructure that has issues. Could we just postpone unloading dlopen'd stuff during exit somehow, until after running all the __fini code ?
Re: shell manpage tweaks wrt getopt
On Fri, Apr 30, 2021 at 04:54:57PM +0200, Christian Weisgerber wrote: > Marc Espie: > > > Until a patch from naddy, I wasn't even aware of getopts in sh(1) > > Let's start the discussion with this instead. > > This puts the deprecation notice in getopt.1 in a prominent place, > and uses the same snippet in sh.1 and ksh.1. > > Index: bin/ksh/ksh.1 > === > RCS file: /cvs/src/bin/ksh/ksh.1,v > retrieving revision 1.214 > diff -u -p -r1.214 ksh.1 > --- bin/ksh/ksh.1 11 Mar 2021 07:04:12 - 1.214 > +++ bin/ksh/ksh.1 30 Apr 2021 14:40:52 - > @@ -3219,6 +3219,25 @@ resetting > .Ev OPTIND , > may lead to unexpected results. > .Pp > +The following code fragment shows how one might process the arguments > +for a command that can take the option > +.Fl a > +and the option > +.Fl o , > +which requires an argument. > +.Bd -literal -offset indent > +while getopts ao: name > +do > + case $name in > + a) flag=1 ;; > + o) oarg=$OPTARG ;; > + ?) echo "Usage: ..."; exit 2 ;; > + esac > +done > +shift $(($OPTIND - 1)) > +echo "Non-option arguments: " "$@" > +.Ed > +.Pp > .It Xo > .Ic hash > .Op Fl r > Index: bin/ksh/sh.1 > === > RCS file: /cvs/src/bin/ksh/sh.1,v > retrieving revision 1.152 > diff -u -p -r1.152 sh.1 > --- bin/ksh/sh.1 22 May 2019 15:23:23 - 1.152 > +++ bin/ksh/sh.1 30 Apr 2021 14:45:22 - > @@ -508,6 +508,25 @@ is a colon, > .Ev OPTARG > is set to the unsupported option, > otherwise an error message is displayed. > +.Pp > +The following code fragment shows how one might process the arguments > +for a command that can take the option > +.Fl a > +and the option > +.Fl o , > +which requires an argument. > +.Bd -literal -offset indent > +while getopts ao: name > +do > + case $name in > + a) flag=1 ;; > + o) oarg=$OPTARG ;; > + ?) echo "Usage: ..."; exit 2 ;; > + esac > +done > +shift $(($OPTIND - 1)) > +echo "Non-option arguments: " "$@" > +.Ed > .It Ic hash Op Fl r | Ar utility > Add > .Ar utility > Index: usr.bin/getopt/getopt.1 > === > RCS file: /cvs/src/usr.bin/getopt/getopt.1,v > retrieving revision 1.19 > diff -u -p -r1.19 getopt.1 > --- usr.bin/getopt/getopt.1 16 Mar 2018 16:58:26 - 1.19 > +++ usr.bin/getopt/getopt.1 30 Apr 2021 14:25:17 - > @@ -14,6 +14,13 @@ > .Ar optstring > .Va $* > .Sh DESCRIPTION > +The > +.Nm > +utility cannot handle option arguments containing whitespace; > +consider using the standard > +.Ic getopts > +shell built-in instead. > +.Pp > .Nm > is used to break up options in command lines for easy parsing by > shell procedures, and to check for legal options. > Sure, works for me.
Re: shell manpage tweaks wrt getopt
On Fri, Apr 30, 2021 at 03:28:42PM +0100, Jason McIntyre wrote: > my argument boils down to: sh(1) is small and has no examples. adding > them changes the (deliberate) nature of the page. ksh(1) is what you > read when you can;t get to sleep. > > why is it wrong to add your example to ksh(1)? why would that leave the > reader disadvantaged? I would also actually be fairly happy if we changed drastically the way sh(1) and ksh(1) look. To me, sh(1) should be the (more or less) standard shell documentation, AND ksh(1) should contain the differences/extensions. Occasionally useful when you want to write scripts that are usable outside of OpenBSD with /bin/sh as a tagline. Not very likely to happen.
