Re: [PATCH] xenocara: change xterm ProcGetCWD to get cwd via sysctl instead of procfs

2022-05-16 Thread Marc Espie
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!

2022-05-15 Thread Marc Espie
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

2022-05-05 Thread Marc Espie
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

2022-05-04 Thread Marc Espie
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

2022-04-14 Thread Marc Espie
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, ) == -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

2022-03-16 Thread Marc Espie
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

2022-03-15 Thread Marc Espie
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

2022-03-11 Thread Marc Espie
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.

2022-02-21 Thread Marc Espie
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

2022-02-14 Thread Marc Espie
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

2022-02-14 Thread Marc Espie
- 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

2022-02-09 Thread Marc Espie
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

2022-02-08 Thread Marc Espie
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

2022-01-21 Thread Marc Espie
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

2022-01-21 Thread Marc Espie
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)

2021-11-15 Thread Marc Espie
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)

2021-11-15 Thread Marc Espie
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

2021-10-30 Thread Marc Espie
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

2021-10-21 Thread Marc Espie
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

2021-05-28 Thread Marc Espie
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

2021-05-28 Thread Marc Espie
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

2021-05-09 Thread Marc Espie
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

2021-04-30 Thread Marc Espie
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

2021-04-30 Thread Marc Espie
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.



Re: shell manpage tweaks wrt getopt

2021-04-30 Thread Marc Espie
On Fri, Apr 30, 2021 at 03:28:42PM +0100, Jason McIntyre wrote:
> On Fri, Apr 30, 2021 at 04:07:55PM +0200, Marc Espie wrote:
> > On Fri, Apr 30, 2021 at 02:44:01PM +0100, Jason McIntyre wrote:
> > > On Fri, Apr 30, 2021 at 11:54:16AM +0200, Marc Espie wrote:
> > > > Until a patch from naddy, I wasn't even aware of getopts in sh(1)
> > > > 
> > > > Unless I made some mistakes, this translates the example in getopt(1)
> > > > manpage.
> > > > 
> > > > It's likely some stronger wording might be adequate, I suspect some
> > > > of the BUGS section in getopt(1) does not apply to the sh(1) built-in.
> > > > 
> > > 
> > > hi marc.
> > > 
> > > is there a reason why you wanted the example in sh(1) and not ksh(1)?
> > > 
> > > i think the overall structure is that ksh(1) is the full monty, whereas
> > > sh(1) is really a simplified reference to which bits you can use if you
> > > want to keep things portable. to that end there aren;t really any
> > > examples in sh(1). i felt that having a short page was more beneficial
> > > in the long run (especially since we already have a full on ksh(1)
> > > page).
> > > 
> > > having said that, getopts is one of those things that you really won;t
> > > get from reading descriptive text. an example is definitely helpful. so
> > > i'd not be dead against it in sh(1) if there's a good reason.
> > > 
> > > there's also the possibility that it may be more helpful to show it in
> > > getopts(1) itself? i guess there are pros and cons there too.
> > > 
> > > jmc
> > 
> > getopts(1) is not a separate command. Having built-ins in their separate
> > manpage would be a definitive departure from what we've always done.
> > 
> 
> i'm not advocating that. ah i think i see the misunderstanding - when i
> said it could "show it in getopts(1) itself" i meant getopt(1) of
> course. i was not proposing we create a getopts(1) page! sorry about
> that!
> 
> > Some linux distributions do have some of the built-ins in their own
> > man pages so that they're easier to find, which makes all kind of sense
> > to me, even though it's pedantically incorrect.  I think it would help,
> > but you are going to ruffle all kinds of feathers.
> > 
> 
> where a standalone command and sh builtin co-exist (like kill) we
> tend to note in STANDARDS for that command that a shell version
> exists.  in this case though, getopt(1) just references sh(1): we
> couldn;t add a pointer to STANDARDS (since it doesn't have one) so
> i think we just nudged SEE ALSO. it could be that we should be more
> upfront in getopt(1) about the shell built-in getopts.
> 
> > 
> > Also, getopts is POSIX. Documenting it in ksh   would send the wrong signal
> > in my opinion (but if I replace getopt with getopts, am I still writing
> > a standard shell script).
> > 
> 
> well, getopts is already documented in ksh(1). along with all the other
> builtins mandated by posix. so i can;t see how it would "send the wrong
> signal".
> 
> 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?
> 
> > That said, we can easily lift the exact same paragraph in both manpages.
> > 
> 
> i don;t like that idea. these pages are already big enough, and duplicate text
> always, at some point, goes out of sync.
> 
> jmc
> 
> 
My diff includes a nudge from getopt(1) towards getopts.

Now, it needs to be 100% explicit that getopts is now POSIX standard,
and thus a perfectly good (somewhat portable) replacement.



Re: shell manpage tweaks wrt getopt

2021-04-30 Thread Marc Espie
On Fri, Apr 30, 2021 at 02:44:01PM +0100, Jason McIntyre wrote:
> On Fri, Apr 30, 2021 at 11:54:16AM +0200, Marc Espie wrote:
> > Until a patch from naddy, I wasn't even aware of getopts in sh(1)
> > 
> > Unless I made some mistakes, this translates the example in getopt(1)
> > manpage.
> > 
> > It's likely some stronger wording might be adequate, I suspect some
> > of the BUGS section in getopt(1) does not apply to the sh(1) built-in.
> > 
> 
> hi marc.
> 
> is there a reason why you wanted the example in sh(1) and not ksh(1)?
> 
> i think the overall structure is that ksh(1) is the full monty, whereas
> sh(1) is really a simplified reference to which bits you can use if you
> want to keep things portable. to that end there aren;t really any
> examples in sh(1). i felt that having a short page was more beneficial
> in the long run (especially since we already have a full on ksh(1)
> page).
> 
> having said that, getopts is one of those things that you really won;t
> get from reading descriptive text. an example is definitely helpful. so
> i'd not be dead against it in sh(1) if there's a good reason.
> 
> there's also the possibility that it may be more helpful to show it in
> getopts(1) itself? i guess there are pros and cons there too.
> 
> jmc

getopts(1) is not a separate command. Having built-ins in their separate
manpage would be a definitive departure from what we've always done.

Some linux distributions do have some of the built-ins in their own
man pages so that they're easier to find, which makes all kind of sense
to me, even though it's pedantically incorrect.  I think it would help,
but you are going to ruffle all kinds of feathers.


Also, getopts is POSIX. Documenting it in ksh   would send the wrong signal
in my opinion (but if I replace getopt with getopts, am I still writing
a standard shell script).

That said, we can easily lift the exact same paragraph in both manpages.



Re: shell manpage tweaks wrt getopt

2021-04-30 Thread Marc Espie
On Fri, Apr 30, 2021 at 12:03:00PM +0100, Raf Czlonka wrote:
> Hi Mark,
> 
> You and me both ;^)
> 
> Until recently, I thought that getopt(1) was POSIX, whereas it is
> in fact getopts(1), and it is not a shell built-in there, but a
> utility[0].

Nope, it is a shell built-in... the "wording" of posix is a bit strange,
but you can't possibly set variables inside the shell from an outside
command.

It has the same status as read.

and I haven't looked too closely, but I would assume most of the BUG
in getopt(1) do not apply to getopts



shell manpage tweaks wrt getopt

2021-04-30 Thread Marc Espie
Until a patch from naddy, I wasn't even aware of getopts in sh(1)

Unless I made some mistakes, this translates the example in getopt(1)
manpage.

It's likely some stronger wording might be adequate, I suspect some
of the BUGS section in getopt(1) does not apply to the sh(1) built-in.

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.122 May 2019 15:23:23 -  1.152
+++ bin/ksh/sh.130 Apr 2021 09:51:42 -
@@ -508,6 +508,26 @@ is a colon,
 .Ev OPTARG
 is set to the unsupported option,
 otherwise an error message is displayed.
+.Pp
+The following example has identical functionality to the
+example in
+.Xr getopt 1 .
+.Bd -literal -offset indent
+while getopts abo: name
+do
+   case "$name"
+   in
+   a|b)
+   flag="-$name";;
+   o)
+   oarg="$OPTARG";;
+   ?)
+   echo "Usage: ..."
+   exit 2
+   ;;
+   esac
+done
+.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 09:51:42 -
@@ -54,7 +54,7 @@ which requires an argument.
 args=`getopt abo: $*`
 if [ $? -ne 0 ]
 then
-   echo 'Usage: ...'
+   echo "Usage: ..."
exit 2
 fi
 set -- $args
@@ -79,6 +79,11 @@ cmd -a -o arg file file
 cmd -oarg -a file file
 cmd -a -oarg -- file file
 .Ed
+Note that
+.Xr sh 1
+offers the
+.Ic getopts
+built-in with a simpler usage.
 .Sh DIAGNOSTICS
 .Nm
 prints an error message on the standard error output when it



patch: split off some port-modules(5) documentation

2021-02-24 Thread Marc Espie
This is mostly mechanical, split off the largest modules documentation off the 
main
page.

Note that some of this could use some reformatting and reparagraphing and even 
proper
English, but this is a bit daunting until the split-off is done.

okay ?



Index: Makefile
===
RCS file: /cvs/src/share/man/man5/Makefile,v
retrieving revision 1.58
diff -u -p -r1.58 Makefile
--- Makefile29 Nov 2020 20:14:06 -  1.58
+++ Makefile24 Feb 2021 15:07:04 -
@@ -7,8 +7,11 @@ MAN=   acct.5 ar.5 bsd.port.mk.5 bsd.port.
fs.5 fstab.5 genassym.cf.5 group.5 hostname.if.5 hosts.5 installurl.5 \
intro.5 login.conf.5 mandoc.db.5 mixerctl.conf.5 \
mk.conf.5 moduli.5 motd.5 mygate.5 myname.5 netgroup.5 passwd.5 \
-   pf.conf.5 pf.os.5 port-modules.5 printcap.5 protocols.5 \
-   ranlib.5 remote.5 resolv.conf.5 rpc.5 ruby-module.5 \
+   pf.conf.5 pf.os.5 port-modules.5 \
+   cargo-module.5 gnome-module.5 go-module.5 python-module.5 
qmake-module.5 \
+   ruby-module.5 \
+   printcap.5 protocols.5 \
+   ranlib.5 remote.5 resolv.conf.5 rpc.5 \
services.5 shells.5 \
spamd.conf.5 sysctl.conf.5 utmp.5 wsconsctl.conf.5

Index: cargo-module.5
===
RCS file: cargo-module.5
diff -N cargo-module.5
--- /dev/null   1 Jan 1970 00:00:00 -
+++ cargo-module.5  24 Feb 2021 15:07:04 -
@@ -0,0 +1,122 @@
+.\"$OpenBSD$
+.\"
+.\" Copyright (c) 2008 Marc Espie
+.\"
+.\" All rights reserved.
+.\"
+.\" 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 disclaimer in the
+.\"documentation and/or other materials provided with the distribution.
+.\"
+.\" THIS SOFTWARE IS PROVIDED BY THE DEVELOPERS ``AS IS'' AND ANY EXPRESS OR
+.\" IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+.\" OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
+.\" IN NO EVENT SHALL THE DEVELOPERS BE LIABLE FOR ANY DIRECT, INDIRECT,
+.\" INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+.\" NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+.\" DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+.\" THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+.\" (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+.\" THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+.\"
+.Dd $Mdocdate$
+.Dt CARGO-MODULE 5
+.Os
+.Sh NAME
+.Nm cargo-module
+.Nd devel/cargo port module
+.Sh DESCRIPTION
+This manual page documents the behavior of setting
+.Li MODULE=Devel/cargo
+in the
+.Xr ports 7
+tree.
+.Pp
+Automates download and compilation of dependencies of a Rust project using
+.Xr cargo 1 .
+During
+.Cm fetch ,
+static dependencies ("crates") listed in
+.Ev MODCARGO_CRATES
+are downloaded using
+.Ev MODCARGO_DIST_SUBDIR
+as
+.Ev DIST_SUBDIR .
+During
+.Cm post-extract ,
+crates defined in
+.Ev MODCARGO_CRATES
+are moved to the
+.Ev MODCARGO_VENDOR_DIR
+directory.
+During
+.Cm post-patch ,
+crate-metadata are generated using
+.Pa devel/cargo-generate-vendor .
+With
+.Ev CONFIGURE_STYLE
+set to
+.Sq cargo ,
+cargo is configured to use
+.Ev MODCARGO_VENDOR_DIR
+instead of the standard crates-io network source.
+Finally, any crates listed in
+.Ev MODCARGO_CRATES_UPDATE
+are updated.
+.Ev MODCARGO_RUSTFLAGS
+can be used to pass custom flags to all
+.Xr rustc 1
+invocations.
+.Pp
+.Pa lang/rust ,
+.Pa devel/cargo
+and
+.Pa devel/cargo-generate-vendor
+are added to
+.Ev BUILD_DEPENDS .
+By default
+.Ev MASTER_SITES9
+is used to download the crates.
+.Pp
+This module defines:
+.Bl -tag -width MODCARGO_CRATES_UPDATE
+.It MODCARGO_CARGOTOML
+Path to cargo manifest.
+Defaults to
+.Pa ${WRKSRC}/Cargo.toml .
+.It MODCARGO_CRATES
+Crates that will be downloaded by the module.
+.It MODCARGO_CRATES_UPDATE
+List of crates to update, overriding the version listed in Cargo.lock.
+.It MODCARGO_FEATURES
+List of features to be used when building.
+.It MODCARGO_VENDOR_DIR
+Name of the local directory for vendoring crates.
+Defaults to
+.Pa ${WRKSRC}/modcargo-crates .
+.El
+.Pp
+This module adds three
+.Xr make 1
+targets:
+.Bl -tag -width modcargo-gen-crates-licenses
+.It Cm modcargo-metadata
+Rerun the generation of crates' metadata.
+.It Cm modcargo-gen-crates
+Generate the
+.Ev MODCARGO_CRATES
+list from Cargo.

Re: make.1: sync variable substitution bits with NetBSD

2020-12-30 Thread Marc Espie
I do think we want to write something specific for .for loop variables
which are actually very special compared to the rest.

I'm not incredibly happy with the way netbsd explains it, not surprisingly.



Re: WANTLIB problems and possible solution: the libset design

2020-10-13 Thread Marc Espie
On Sun, Oct 11, 2020 at 06:03:55PM +0200, Landry Breuil wrote:
> On Sun, Oct 11, 2020 at 05:03:31PM +0200, Marc Espie wrote:
> > On Sun, Oct 11, 2020 at 04:53:18PM +0200, Christian Weisgerber wrote:
> > > Marc Espie:
> > > 
> > > > The new design:
> > > > 
> > > > The idea behind "libset" is to be able to specify a "set" of wantlib 
> > > > that
> > > > corresponds to our package, AND to just write WANTLIB wrt that libset 
> > > > for
> > > > that specific set of libraries.
> > > 
> > > I'm struggling to understand whether this libset records the libraries
> > > a port depends on, the libraries the port provides, or both.
> > > 
> > > Let's say--slightly simplified from reality--we have devel/gettext
> > > that provides libintl and depends on iconv from converters/libiconv.
> > > What would gettext's LIBSET entry look like?
> > > 
> > > (1) LIBSET = iconv
> > > (2) LIBSET = intl
> > > (3) LIBSET = intl iconv
> > 
> > 
> > 
> > I assume any user of gettext will need both iconv and intl, so
> > 
> > LIBSET = intl iconv
> 
> Hmm, pretty sure some consumers of gettext only have one in their
> WANTLIB and not both, in that case those ones shouldnt you the libset to
> avoid an extra WANTLIB ?
> 

On the off chance I understood your meaning, I think it's better
to err on the side of having too many WANTLIB than too few.

I fail to see an actual downside to updating when iconv gets updated.
gettext will often be updated as well.



Re: WANTLIB problems and possible solution: the libset design

2020-10-13 Thread Marc Espie
On Sun, Oct 11, 2020 at 06:03:55PM +0200, Landry Breuil wrote:
> On Sun, Oct 11, 2020 at 05:03:31PM +0200, Marc Espie wrote:
> > On Sun, Oct 11, 2020 at 04:53:18PM +0200, Christian Weisgerber wrote:
> > > Marc Espie:
> > > 
> > > > The new design:
> > > > 
> > > > The idea behind "libset" is to be able to specify a "set" of wantlib 
> > > > that
> > > > corresponds to our package, AND to just write WANTLIB wrt that libset 
> > > > for
> > > > that specific set of libraries.
> > > 
> > > I'm struggling to understand whether this libset records the libraries
> > > a port depends on, the libraries the port provides, or both.
> > > 
> > > Let's say--slightly simplified from reality--we have devel/gettext
> > > that provides libintl and depends on iconv from converters/libiconv.
> > > What would gettext's LIBSET entry look like?
> > > 
> > > (1) LIBSET = iconv
> > > (2) LIBSET = intl
> > > (3) LIBSET = intl iconv
> > 
> > 
> > 
> > I assume any user of gettext will need both iconv and intl, so
> > 
> > LIBSET = intl iconv
> 
> Hmm, pretty sure some consumers of gettext only have one in their
> WANTLIB and not both, in that case those ones shouldnt you the libset to
> avoid an extra WANTLIB ?

How about you make a full sentence with the correct words 
so I get what you mean ?



Re: WANTLIB problems and possible solution: the libset design

2020-10-11 Thread Marc Espie
On Sun, Oct 11, 2020 at 04:53:18PM +0200, Christian Weisgerber wrote:
> Marc Espie:
> 
> > The new design:
> > 
> > The idea behind "libset" is to be able to specify a "set" of wantlib that
> > corresponds to our package, AND to just write WANTLIB wrt that libset for
> > that specific set of libraries.
> 
> I'm struggling to understand whether this libset records the libraries
> a port depends on, the libraries the port provides, or both.
> 
> Let's say--slightly simplified from reality--we have devel/gettext
> that provides libintl and depends on iconv from converters/libiconv.
> What would gettext's LIBSET entry look like?
> 
> (1) LIBSET = iconv
> (2) LIBSET = intl
> (3) LIBSET = intl iconv



I assume any user of gettext will need both iconv and intl, so

LIBSET = intl iconv



WANTLIB problems and possible solution: the libset design

2020-10-10 Thread Marc Espie
I want to introduce a new mechanism (libsets) to supplement wantlib...
Note that the keyword already exists in pkg_add.

the story so far:

wantlib evolved from lib-depends

basically, you tell a given package that it requires some libs, (WANTLIB)
and pkg_create will register those libs in the package as a set of
@wantlib annotations, thus hardcoding the major/minor of each library.

There's some sanity check as well, both during building packages and
installing them, which is that all wantlib must be "reachable": looking
through all run dependencies (recursively) of the package
the libraries must actually exist in a dependency (recursively) of the
package or in base/x11.

Whenever you update, things must go strictly forward: if the package version
changes, that's enough.

Otherwise, we do look at version numbers of direct dependencies AND at wantlib
version: if things go backward, then we don't update. If things are the
same, we don't update either.

Altogether this is a "signature" for the package version:
- the package version itself
- the version of each direct dependency
- the version of each wantlib


The problem:

In case the set of wantlib changes, you have to bump the revision of all
dependent packages, otherwise register-plist (or pkg_add) will complain
that package signatures are non comparable.


This is a problem if a basic gnome package (say gnomeajasper) adds a 
libfoobar, because suddenly, all users (direct AND indirect) of that 
package MUST be updated to
- include foobar in their WANTLIB
- have their REVISION bumped.


The new design:

The idea behind "libset" is to be able to specify a "set" of wantlib that
corresponds to our package, AND to just write WANTLIB wrt that libset for
that specific set of libraries.

In practice:
- the basic package would get a libset declaration
@libset mylibset-1.0 liba,libb,libc

- direct and indirect users would have +mylibset   in their WANTLIB
(the specific character used is up for debate... (mylibset) would be nice as
well, but it would interfere with everything shell-quoting

this would have several effects:
- when making the package, pkg_create would scan dependent packages for
the libset declaration, and add the corresponding wantlib
- the resulting package signature would ALSO include +mylibset-.10 in their
signature.


For updates, things change slightly
- dependencies must be going forward
- libsets MUST be going forward (you still need to bump REVISION if you add
a whole libset to a package)
- wantlib MUST be going forward, BUT there is a bit of libset comparison:
if a wantlib is ONLY mentioned in a new version of the libset compared to the
old (or vice-versa), then it is no longer an error, and versions are STILL
comparable. (*)


This is more or less the basic principle.  Note that (*) is a bit tricky: this
only concerns wantlib which are exclusively part of a given libset... because
if the wantlib is also mentioned elsewhere, then it still has to be given
properly.


As for libset versions proper, we would go with the exact same version scheme
as for packages.


The reason for this would lie with bsd.port.mk: I think it's likely that
each package would declare ONE single libset at most, so there is zero issue
in naming the libset exactly like the package, so a libset declaration would
be
LIBSET = liba libb libc

in the case above, it would become
LIBSET = liba libb libc libfoobar

and lead to
@libset gnomeajaspec:liba,libb,libc,libfoobar

(note that I've written "lib" everywhere to make things obvious, but as
usual, this should rather be LIBSET = a b c foobar)

(semantics for multi-packages left up to the astute reader).

(even though it's currently redundant and matches pkgnames, I prefer to have 
a separate namespace for libset, just in case we eventually want to add
several libsets for a given package...)


I don't think we need to make libset recursive (e.g., have a libset include
another libset) and also, I have zero idea how to implement that.


As far as I can figure out, to change a libset, you would only need to 
bump the rev of the basic port and add to its LIBSET declaration.

If wanted, this could even be used for base/X11, we would mostly need to 
have a smallish libset declarations for (say) all the X11 libraries that 
are commonly used for something.



Re: pkg_add.1

2020-06-26 Thread Marc Espie
On Wed, Jun 24, 2020 at 05:32:57PM +0100, Jason McIntyre wrote:
> On Tue, Jun 23, 2020 at 07:28:09PM -0400, sven falempin wrote:
> > Dear readers,
> > 
> > It may not be very obvious that 'dry run' mode of pkg_add
> > actually downloads packages.
> > It is a good feature and maybe the pkg_add man could use an EXAMPLES
> > section.
> > 
> 
> hi.
> 
> it might not be obvious, but it is documented:
> 
>-n   Don't actually install a package, just report the
> steps that would be taken if it was.  Will still
> copy packages to PKG_CACHE if applicable.
> 
> if you look through the page for how the PKG_CACHE stuff works, it
> seems clear.
> 
> anyway, i'll defer to espie on whether the diff is wanted or not.
> comments on your text inline:

I don't think it adds much, and it's indeed already documented, and used
for instance by bsd.port.mk



Re: improve pkg_add bandwidth usage with some mirrors

2020-06-18 Thread Marc Espie
On Wed, Jun 17, 2020 at 11:34:20AM +0200, Solene Rapenne wrote:
> I propose a small diff for pkg_add when using http/https mirrors.
> Don't wait 30 seconds for the ftp process to stop when closing
> file handler, send SIGHUP immediately after closing the file handler.
> 
> Running pkg_add -u neovim (already installed and up to date) I got
> those results of bandwidth usage
> 
> using mirror https://mirror.one.com/pub/OpenBSD/
> 62513 kB without diff
> 9050  kB with diff
> 
> using mirror https://ftp.fr.openbsd.org/pub/OpenBSD
> 6530 kB without diff
> 6373 kB with diff
> 
> The 2500 kB difference between the two mirrors with the diff is
> explained by the html directory listing which is different
> between servers. mirror.one.com list is 3800 kB while
> ftp.fr.openbsd.org only 1387 kB.
> 
> I can't explain why but when using mirror.one.com the ftp command will
> continuously fetch packages until completion or stop if ftp is killed
> after 30 seconds. This extra downloaded data is useless.
> 
> I came with the following diff, but maybe the real issue is ftp(1)
> not stopping immediately if output is closed?

Like I said privately to solene, I would very much prefer
that we figure out why things don't die "instantly".

What pkg_add does internally is a pipeline:

ftp | signify|internal gunzip

closing the end file handle should kill the whole chain.
So I need to figure out where it goes wrong, what's the
part that doesn't die "instantly".

The thing to do would be to send explicit SIGPIPE to
either signify or ftp and see whether things work better.



Re: [PATCH] [xenocara] app/xenodm/config/Xsetup_0 - reduce the number of lines

2020-06-16 Thread Marc Espie
On Tue, Jun 16, 2020 at 08:43:02PM +0100, Raf Czlonka wrote:
> Ping.
> 
> CC'ing espie@ as he committed the initial code.
> 
> Cheers,
> 
> Raf
> 
> On Sun, Jun 07, 2020 at 07:30:39PM BST, Raf Czlonka wrote:
> > Hi all,
> > 
> > I've been running openbsd-backgrounds on all of my desktop machines and
> > thought this can be simplified a bit:
> > 
> > - fewer lines to uncomment
> > - easier to automate, i.e. via one liner, script, config management, etc.
> > - still under 80 columns wide
> > 
> > For your consideration.
> > 
> > Cheers,
> > 
> > Raf
> > 
> > Index: app/xenodm/config/Xsetup_0
> > ===
> > RCS file: /cvs/xenocara/app/xenodm/config/Xsetup_0,v
> > retrieving revision 1.6
> > diff -u -p -r1.6 Xsetup_0
> > --- app/xenodm/config/Xsetup_0  29 Jun 2019 13:33:06 -  1.6
> > +++ app/xenodm/config/Xsetup_0  7 Jun 2020 18:29:16 -
> > @@ -6,9 +6,6 @@ xconsole -geometry 480x130-0-0 -daemon -
> >  #  install package openbsd-backgrounds
> >  #  then uncomment:
> >  #
> > -# if test -x /usr/local/bin/openbsd-wallpaper
> > -# then
> > -#  /usr/local/bin/openbsd-wallpaper
> > -# fi
> > +# test -x /usr/local/bin/openbsd-wallpaper && 
> > /usr/local/bin/openbsd-wallpaper
> >  
> >  # sxpm OpenBSD.xpm &
> 
Even though it's longer, I actually prefer using if  for this kind of thing.



Re: userland clock_gettime proof of concept

2020-06-11 Thread Marc Espie
On Thu, Jun 11, 2020 at 07:55:44AM -0600, Theo de Raadt wrote:
> I think this conversation about major numbers, and now minor numbers,
> is incredibly silly.
> 
> Do we have a major number on the open(2) system call?  No, and the main
> reason is because the code isn't a draft, it was completed.
> 
> This needs to support as many architectures as possible, then it goes
> in.
> 
> This addition of major and minor numbers appears to be a distraction.

I think a major is a good idea just in case we decide to change things at
some point later on, to avoid flag days.  It's cheap and easy.

Minor number, I don't see a point.



Re: userland clock_gettime proof of concept

2020-06-11 Thread Marc Espie
On Thu, Jun 11, 2020 at 03:42:27PM +0300, Paul Irofti wrote:
> On Thu, Jun 11, 2020 at 02:05:44PM +0200, Marc Espie wrote:
> > On Thu, Jun 11, 2020 at 01:13:07PM +0300, Paul Irofti wrote:
> > > On 2020-06-11 02:46, Christian Weisgerber wrote:
> > > > Paul Irofti:
> > > > 
> > > > > This iteration of the diff adds bounds checking for tk_user and moves
> > > > > the usertc.c stub to every arch in libc as recommanded by deraadt@.
> > > > > It also fixes a gettimeofday issue reported by cheloha@ and tb@.
> > > > 
> > > > Additionally, it changes struct timekeep in an incompatible way. ;-)
> > > > A userland built before the addition of tk_nclocks is very unhappy
> > > > with a kernel built afterwards.  There is no way to compile across
> > > > this.  You have to (U)pgrade from boot media to install a 
> > > > ftp.openbsd.org
> > > > userland, and then you can re-compile with the new diff.
> > > 
> > > I have not seen this problem and have not built a snapshot to update or go
> > > back. What do you mean by very unhappy? Can you show me the exact steps 
> > > you
> > > have done?
> > > 
> > > > Should we already bump major while the diff matures?
> > > 
> > > I am not a fan of this. I don't like bumping something before it is 
> > > actually
> > > used. It is like an errata before a release.
> > 
> > So what if we end at version 200 ?
> > 
> > we've got a full uint32_t for crying out loud, you're not going to run out
> > of numbers.
> > 
> > Besides, it's something that's entirely invisible to users, even more so
> > than library major/minors.
> 
> This is not about the range available to us.
> 
> If I bump then I will have to also add checks for the revision.
> Otherwise what is the point of the bump?  And then what? Keep old and
> new code around for both revisions? And then, if this endless mail
> thread is ever going to be added to the OpenBSD tree, it will contain
> workarounds for something that was never in the tree to begin with.

Yeah, you do check for the revision, if it's the same, then you use
the timecounter. If it's not, you revert to the syscall.

End of story.

Right now, you can't even bump it if you need, because there is no code
that checks it in the libc, thus is you tweak kernel parts, things *will*
break.

You'd better have the version check in libc  before you even consider
committing this!



Re: userland clock_gettime proof of concept

2020-06-11 Thread Marc Espie
On Thu, Jun 11, 2020 at 01:13:07PM +0300, Paul Irofti wrote:
> On 2020-06-11 02:46, Christian Weisgerber wrote:
> > Paul Irofti:
> > 
> > > This iteration of the diff adds bounds checking for tk_user and moves
> > > the usertc.c stub to every arch in libc as recommanded by deraadt@.
> > > It also fixes a gettimeofday issue reported by cheloha@ and tb@.
> > 
> > Additionally, it changes struct timekeep in an incompatible way. ;-)
> > A userland built before the addition of tk_nclocks is very unhappy
> > with a kernel built afterwards.  There is no way to compile across
> > this.  You have to (U)pgrade from boot media to install a ftp.openbsd.org
> > userland, and then you can re-compile with the new diff.
> 
> I have not seen this problem and have not built a snapshot to update or go
> back. What do you mean by very unhappy? Can you show me the exact steps you
> have done?
> 
> > Should we already bump major while the diff matures?
> 
> I am not a fan of this. I don't like bumping something before it is actually
> used. It is like an errata before a release.

So what if we end at version 200 ?

we've got a full uint32_t for crying out loud, you're not going to run out
of numbers.

Besides, it's something that's entirely invisible to users, even more so
than library major/minors.

Remember the porters motto "when in doubt, bump"



Re: code style questions

2020-06-10 Thread Marc Espie
On Wed, Jun 10, 2020 at 05:38:36PM +1000, Jonathan Gray wrote:
> On Wed, Jun 10, 2020 at 09:19:47AM +0200, Martin Pieuchot wrote:
> > On 09/06/20(Tue) 20:19, jo...@armadilloaerospace.com wrote:
> > > Looking for some guidance to avoid proposing any unpopular diffs.
> > > 
> > > Style(9) says not to use static on file-local functions in the
> > > kernel, because it interferes with the debugger.  They still show up
> > > on some functions today; is this still an issue?
> > 
> > I don't know if it's an issue, but not using 'static' is still a
> > convention.
> 
> It is.  Backtraces in drm code aren't as useful as they could be as
> static is extensively used.
> 
> 
One annoying thing in C is that keywords are grossly overloaded.

If static and static were two different keywords, you could just
#define static

and be done with it.

(are there any static variables in the drm code ?...)



Re: code style questions

2020-06-10 Thread Marc Espie
On Tue, Jun 09, 2020 at 08:19:53PM -0700, jo...@armadilloaerospace.com wrote:
> Also, style(9) says that prototypes should not have variable names
> associated with the types.  I try to use good names in the headers
> for documentation purposes; what is the thinking behind the rule?

function names are part of the API, variable names are not.  Indeed they
are like comment, but deadly comments, because they are still part of
the namespace, hence susceptible to yield issues with macros.

In your example,

> int pcdisplay_copycols(void *id, int row, int srccol, int dstcol, int
> ncols);
> vs:
> int pcdisplay_copycols(void *, int, int, int,int);
> 

first one won't compile if there is a
#define row (getenv("ROW"))
in effect.


Also, even with variable names, it's possible to mix-up parameters with
similar types.

That rule is somewhat controversial though.

I personally tend to prefer actually documenting what the function does in
a comment in the header, and comment how it does it in the source file.



Re: Xwindows keymap weirdness

2020-06-01 Thread Marc Espie
On Mon, Jun 01, 2020 at 03:37:45PM -0600, Anthony J. Bentley wrote:
> Marc Espie writes:
> > > To setup the right alt key as compose, you can either:
> > > 
> > > - run 'setxkbmap -option compose:ralt' somewhere in your session
> > >   startup script
> > > 
> > > - create /etc/X11/xorg.conf.d/90-keyboard.conf containing
> > > 
> > > --- Cut ---
> > > Section "InputClass"
> > > Identifier "Kbd"
> > > MatchDriver "kbd"
> > > Option "XkbOptions" "compose:ralt"
> > > EndSection
> > > --- Cut ---
> >
> > I used to understand that shit back in xmodmap days.
> > I'll admit I'm completely lost with setxkbmap
> >
> > Along the same lines, how can you simply disable caps lock ?
> 
> caps:none
> 
> Personally I run with caps:ctrl_modifier (caps becomes ctrl).
> 
> These variants are documented in xkeyboard-config(7) (needs a large
> terminal to be readable).


Oh wow, this doc is a complete piece of shit.
I finally figured it out the relationship between setxkbmap
and xkeyboard-config thanks to the lines you gave.

It would be sooo enlightening to have a few EXAMPLES in setxkbmap(1)
that does explain how xkeyboard-config(7) actually works.

Last time I looked, I thought I need to create my own description that would
be compiled through xkbcomp(1), that's how misleading this so called
"documentation" is.

Thank you Anthony!



Re: Xwindows keymap weirdness

2020-06-01 Thread Marc Espie
On Mon, Jun 01, 2020 at 03:46:09PM +0200, Matthieu Herrb wrote:
> On Mon, Jun 01, 2020 at 03:28:52PM +0200, Stéphane Aulery wrote:
> > Hello,
> > 
> > Le 01/06/2020 14:55, Matthieu Herrb a écrit :
> > > 
> > > > 
> > > > (I have just tried with a test user with nothing configured besides
> > > > LC_CTYPE=en_US.UTF-8, without which xterm/vim doesn't show proper
> > > > characters)
> > > 
> > > I'm using a real US keyboard with AltGr or the Menu Key (depending on
> > > the actual keyboard) set as Compose and typing full compose sequences
> > > to get diacritics. ieand so on.
> > 
> > I use a Bépo keyboard but this but
> > 
> > Your experience interests me. I use a Bépo keyboard but I plan to switch to
> > a QWERTY + compose keyboard like you do. I hope this will give better
> > compatibility between systems and less software config remapping.
> > 
> > I do not see how to configure this in console.
> 
> I'm only using this under X. the OpenBSD console is plain ASCII and
> has no support for for UTF8 characters, so no need to enter them.
> 
> To setup the right alt key as compose, you can either:
> 
> - run 'setxkbmap -option compose:ralt' somewhere in your session
>   startup script
> 
> - create /etc/X11/xorg.conf.d/90-keyboard.conf containing
> 
> --- Cut ---
> Section "InputClass"
> Identifier "Kbd"
> MatchDriver "kbd"
> Option "XkbOptions" "compose:ralt"
> EndSection
> --- Cut ---

I used to understand that shit back in xmodmap days.
I'll admit I'm completely lost with setxkbmap

Along the same lines, how can you simply disable caps lock ?



Xwindows keymap weirdness

2020-06-01 Thread Marc Espie
I occasionnally use setxkbmap 'us(intl)' in order to have diacritics
as dead keys.

I have LC_CTYPE=en_US.UTF-8

If I type 'c

I get a ç  in programs like firefox or chrome, BUT I get
ć   in xterm ?

why are things different ? which program is right ?

(I have just tried with a test user with nothing configured besides
LC_CTYPE=en_US.UTF-8, without which xterm/vim doesn't show proper characters)



Re: official ports vs DEBUG_PACKAGES

2020-05-30 Thread Marc Espie
On Sat, May 30, 2020 at 05:10:49PM +0200, Jeremie Courreges-Anglas wrote:
> On Sat, May 30 2020, Marc Espie  wrote:
> >> - have some magic I don't know in ELF handling that would allow to either
> >> tweak the default location/introduce ${WRKOBJDIR} in that debug info.
> >
> > Thinking some more about it, that 3rd option is possibly not that
> > far-fetched
> >
> > we do pass most debug-info thru dwz  for shrinkage, so there's one tool
> > that does peek inside dwarf debug and does compaction, I have zero idea how
> > hard it is to tweak paths as well.
> 
> Take a look at -fdebug-prefix-map.
> 
>   https://reproducible-builds.org/docs/build-path/

Nice, thanks :)

So just adding this to CFLAGS/CXXFLAGS whenever DEBUG_PACKAGES is set
should be enough



Re: official ports vs DEBUG_PACKAGES

2020-05-30 Thread Marc Espie
> - have some magic I don't know in ELF handling that would allow to either
> tweak the default location/introduce ${WRKOBJDIR} in that debug info.

Thinking some more about it, that 3rd option is possibly not that
far-fetched

we do pass most debug-info thru dwz  for shrinkage, so there's one tool
that does peek inside dwarf debug and does compaction, I have zero idea how
hard it is to tweak paths as well.



official ports vs DEBUG_PACKAGES

2020-05-29 Thread Marc Espie
In a trace:

> > > #3  0x15e48c95459e in WebVfx::shutdown ()
> > >  at /usr/obj/ports/webvfx-1.2.0/webvfx-1.2.0/webvfx/webvfx.cpp:193

Now, this is NOT the default location for WRKOBJDIR, but we are shipping
packages with debug information...

This could be a bit cumbersome, if in order to debug locally, you have
to make patch NO_DEPENDS=Yes, and then you figure out you have to mimic the
"official" setup for ports, which does not even match the default.

There are about 3 solutions to that:
- change the bulk machines to /usr/ports/pobj
- change the ports default to /usr/obj/ports
- have some magic I don't know in ELF handling that would allow to either
tweak the default location/introduce ${WRKOBJDIR} in that debug info.



Re: locale thread support

2020-05-29 Thread Marc Espie
On Tue, May 26, 2020 at 12:13:02AM +0200, Ingo Schwarze wrote:
> Regarding the FreeBSD headers, i like them even less.  There are both
> terrible design choices and terrible implementation choices.  Regarding
> design, it appears that you *must* #include  after other
> headers - if you include it before, it won't work.  Regarding
> implementation, there is quite repulsive macro abuse in xlocale/_ctype.h;
> but probably that can be unrolled in LibreSSL style.  Either way, none
> of that hinders providing them if doing so yields benefit.

Thinking a bit more about that, two points:
- we do have headers in sys/  that do require a specific order of inclusion,
though that has mostly vanished over the years

- starting from a FreeBSD-like structure and making xlocale.h 
position-independent if we so wish is trivial  and not even that much
work. Each snippet is neatly tucked into an xlocale/  subdirectory,
so including xlocale.h early just requires something like

#ifdef _X_LOCALE_H
#include 
#endif

inside each  header...



Re: locale thread support

2020-05-29 Thread Marc Espie
On Fri, May 29, 2020 at 03:48:41PM +0200, Ingo Schwarze wrote:
> Hi Marc,
> 
> do i understand correctly that you intend to say:  there are
> legitimate reasons to use these xlocale.h functions in portable
> software, in particular in portable libraries, so we should probably
> have them all, or at least most of them?
> 
> I see your point that using functions like isdigit(3) can (at least
> in theory) be dangerous when running on systems that support
> exceedingly weird single-byte 8-bit locales.  (As opposed to only
> supporting one single single-byte locale, which is 7-bit, like we do.)
> 
> I'm not even sure the question matters whether xlocale.h is the best
> possible way to avoid such issues.  To make support desirable, it
> might be sufficient that xlocale.h is one reasonable way and that
> some amounts of useful software actually use it.
> 
> Yours,
>   Ingo

I'm starting to believe that a xlocale-like model makes more sense than
the current model in a lot of cases.

Think about it: a lot of locale goo deals with text processing.
NOT input/output, but data processing.

It seems obvious to me that at least SOME locale information has to be
attached to the data, and so each time you have to process the data, you
have to choose the right locale.

setlocale() is out in modern multi-threaded programs.
both uselocale() and xlocale.h functions are NOT portable to everything.

xlocale.h allows you  to attach the locale to the data

uselocale() requires you to do a back each time you want to process data
if you don't want to disturb other things.   It's hidden globals all over 
again.

It's no accident that libc++ will use xlocale.h functions if they're around.
As every other modern library should.



Re: userland clock_gettime proof of concept

2020-05-29 Thread Marc Espie
On Thu, May 28, 2020 at 05:44:31PM +0300, Paul Irofti wrote:
> Hi,
> 
> Here is a new iteration of the diff which includes support for MD high
> resolution clocks. Currently only implements TSC on amd64. If the
> MD function is not defined, it fallsback to the syscall.
> 
> There is the question of the skew fix, but that will be addressed in a
> separate kernel diff that will not affect the current diff at all.
> 
> I could not find a way to find on which processor the process is running
> on from userland without going through a syscall. If there is one please
> let me know. It would make things easier.
> 
> In the meantime I have also gotten positive feedback from various
> testers that run this on their main machine.
> 
> Anyway, I think we can decide on the struct name and the auxiliary
> vector ID and consider this done.
> 
> Thoughts?
> 
> Paul 

This appears to work just fine here on my desktop.

cpu0: AMD A10-5700 APU with Radeon(tm) HD Graphics, 3417.45 MHz, 15-10-01
cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,POPCNT,AES,XSAVE,AVX,F16C,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,IBS,XOP,SKINIT,WDT,FMA4,TCE,NODEID,TBM,TOPEXT,CPCTR,ITSC,BMI1

(other bits of dmesg available on demand)



Re: locale thread support

2020-05-28 Thread Marc Espie
On Tue, May 26, 2020 at 12:13:02AM +0200, Ingo Schwarze wrote:
> Hi,
> 
> my impression is there are two totally independent topics in this
> thread.
> 
>  1. Whether and how to make things safer, ideally terminating the
> program in a controlled manner, if it uses functions that are
> not thread-safe in an invalid manner.  I'm not addressing that
> topic in this mail.
> 
>  2. Whether we want additional non-standard xlocale.h functions
> in our libc.
> 
> Regarding topic 2, let me say up front that i do not feel strongly
> either way.  It seems to me this is mostly a question for porters.
> If some functions are widely used - even if non-standard - and help
> avoid porting hassle, then sure, let's have them unless they cause
> excessive fuss.  But the latter does not seem likely here.
> 
> There are a number of functions that seem likely useful to me off
> the top of my head, for example: querylocale, nl_langinfo_l,
> MB_CUR_MAX_L, wcwidth_l
> 
> In general, i must say i like the whole xlocale.h business not all
> that much: in a nutshell, it is doubling the number of half the
> functions under the sun, which usually indicates poor design in the
> first place, and then the vast majority of these additional functions
> are almost useless on any operating system and totally useless on
> OpenBSD.  Like, i mean, btowc_l(3)?  Or even the isdigit_l(3) which
> we already have?  You must be choking, Mr. Chisnall!  I don't think
> stuff should be added because it is out there, but only if there is
> at least some porting benefit.
> 
> Regarding the FreeBSD headers, i like them even less.  There are both
> terrible design choices and terrible implementation choices.  Regarding
> design, it appears that you *must* #include  after other
> headers - if you include it before, it won't work.  Regarding
> implementation, there is quite repulsive macro abuse in xlocale/_ctype.h;
> but probably that can be unrolled in LibreSSL style.  Either way, none
> of that hinders providing them if doing so yields benefit.
> 
> See below for a list of functions that i drafted extremely quickly,
> so beware of errors and omissions.  The point isn't to present a
> definite plan.  The point is merely to demonstrate the sheer fatness
> of the animal and to give a first impression of the degree to which
> it stinks and grunts from most of its ends.
> 
> If any of this is needed, do porters already know which functions
> are in particularly high demand?  Do you have any idea how to find
> out which ones are actually useful for porting purposes?

Serendipity

Today, I looked at an older library  for audit purposes.

Library is called udns.

If you look at the code, there's a lot of apparently useless reinvention,
like the code explicitly checks for digits using c >= '0' && c <= '9' patterns
(or isprint for that matter).

Thinking some more, I realized why: this is library code, so it can't assume
it's running under any kind of sensible locale unless it has asserted it 
itself.

In that specific context, isdigit_l and friends make a lot of sense.
Either that, or systematic uselocale wrappers for any function that can be
called from outside.

(yes, I now that isdigit/isspace are not a problem on OpenBSD, but they can
be elsewhere!)



Re: locale thread support

2020-05-22 Thread Marc Espie
On Fri, May 22, 2020 at 02:31:38PM +0200, Mark Kettenis wrote:
> > Date: Fri, 22 May 2020 13:50:44 +0200
> > From: Marc Espie 
> > 
> > I've had to neuter some setlocale() in mlt, since our code is
> > definitely NOT thread-safe.  No biggie, since we do not have support
> > for LC_NUMERIC right now.
> 
> Hmm, setlocale() is explicitly mentionded as not thread-safe in POSIX.
> 
> > I think we might want the thread-specific functions, namely stuff like
> > strtod_l, or sprintf_l  and friends.
> 
> That is .  which isn't actually standardized.  But there is
> a de-facto standard set by Apple.  Most OSes have an implementation by
> now and our libc++ needs it (and uses a stub implementation for now).
> The Darwin and/or FreeBSD man pages are probably the best
> documentation for these interfaces.
> 
> > Even if they do not do anything specific right now with a locale object,
> > at least they would prevent re-entrency issues.
> 
> Yes, implementing these shouldn't be hard.  We already have wctype_l()
> and iswctype_l() (which are standardized) which do care about the
> locale.  But everything else probably doesn't so these functions can
> be simple wrappers around the non-_l functions that simply ignore the
> locale.  It's just work but really doesn't require sepcific skills.
> I'm sure Ingo will be able to give some constructive feedback once a
> diff exists.

Looks like for the header part we can mostly borrow from FreeBSD, and I gather
the libc part shouldn't be that hard (minus locale support of course).

I'd wait to see what Ingo thinks about that.



Re: locale thread support

2020-05-22 Thread Marc Espie
On Fri, May 22, 2020 at 04:02:31PM +0200, Mark Kettenis wrote:
> > From: "Todd C. Miller" 
> > Date: Fri, 22 May 2020 07:23:55 -0600
> > 
> > On Fri, 22 May 2020 14:57:11 +0200, Marc Espie wrote:
> > 
> > > From a security standpoint, is there a "cheap" way to make setlocale 
> > > abort()
> > > instead of trying to do a double free on  when running in a race 
> > > condition ?
> > 
> > We could use _THREAD_PRIVATE_MUTEX as we do in other parts of libc.
> 
> That might eliminate two threads racing eachoither in setlocale(), but
> it wouldn't stop threads that actually access the locale from
> use-after-free type bugs.  Unless you use the lock there as well.  But
> that could have a major performance impact.

Well, I'd be happy to have an abort() in case a race is detected.

As we said, this is 100% unsupported behavior! but it's reasonably hard to 
detect.

Maybe a simple way to poison setlocale is it's called from two different threads
in a given process ?... I have zero idea.

My point is NOT to make broken code work, but to make it obvious it's broken!
because right now, it's a sporadic race... and even though this is complicated, 
I
fear it's a security issue waiting to happen.



Re: locale thread support

2020-05-22 Thread Marc Espie
On Fri, May 22, 2020 at 02:31:38PM +0200, Mark Kettenis wrote:
> > Date: Fri, 22 May 2020 13:50:44 +0200
> > From: Marc Espie 
> > 
> > I've had to neuter some setlocale() in mlt, since our code is
> > definitely NOT thread-safe.  No biggie, since we do not have support
> > for LC_NUMERIC right now.
> 
> Hmm, setlocale() is explicitly mentionded as not thread-safe in POSIX.

>From a security standpoint, is there a "cheap" way to make setlocale abort()
instead of trying to do a double free on  when running in a race condition ?

> > I think we might want the thread-specific functions, namely stuff like
> > strtod_l, or sprintf_l  and friends.
> 
> That is .  which isn't actually standardized.  But there is
> a de-facto standard set by Apple.  Most OSes have an implementation by
> now and our libc++ needs it (and uses a stub implementation for now).
> The Darwin and/or FreeBSD man pages are probably the best
> documentation for these interfaces.
> 
> > Even if they do not do anything specific right now with a locale object,
> > at least they would prevent re-entrency issues.
> 
> Yes, implementing these shouldn't be hard.  We already have wctype_l()
> and iswctype_l() (which are standardized) which do care about the
> locale.  But everything else probably doesn't so these functions can
> be simple wrappers around the non-_l functions that simply ignore the
> locale.  It's just work but really doesn't require sepcific skills.
> I'm sure Ingo will be able to give some constructive feedback once a
> diff exists.


> > Note that the current issue is a time-bomb, because it is a race, so
> > it does only manifest itself as a double free in freegl...
> > 
> > uselocale is fine, but it is not on windows, so highly portable code
> > tends to prefer strtod_l...
> 
> Which is funny since strtod_l is even less standard than uselocale() ;).

Well, I had a look at the current code, they changed it yet again, so it's
probably going to fail in newer, more funny ways.



locale thread support

2020-05-22 Thread Marc Espie
I've had to neuter some setlocale() in mlt, since our code is definitely NOT
thread-safe.  No biggie, since we do not have support for LC_NUMERIC right now.

I think we might want the thread-specific functions, namely stuff like
strtod_l, or sprintf_l  and friends.

Even if they do not do anything specific right now with a locale object,
at least they would prevent re-entrency issues.

Note that the current issue is a time-bomb, because it is a race, so it does 
only
manifest itself as  a double free in freegl...


uselocale is fine, but it is not on windows, so highly portable code tends to
prefer strtod_l...



Re: Mouse movement speed

2020-05-19 Thread Marc Espie
I haven't used wsmoused in ages, but I tend to think ideas like (by default)
scaling to the font size have great merit.

There should be a debugging toggle (maybe) to get raw values.

It should work out-of-the-box preferably. ;)



Re: Mention /etc/examples/ in those config files manpages + FILES short description

2020-05-01 Thread Marc Espie
I do think newcomers will tend to miss examples, even if /etc/examples is
somewhat documented elsewhere.

Also, queries to mandocdb will probably benefit... say "what commands do
have an example configuration file ?"  kind of what you should be able to
ask with advanced queries.



Re: make patch: prefer standard behavior

2020-04-11 Thread Marc Espie
On Thu, Apr 09, 2020 at 06:33:25PM +0200, Marc Espie wrote:
> Our make (bsd make) has "alternate" dependency operators :: and !
> 
> Twenty years ago, we made it so that ambiguous constructs like
> File::Find.3p: File/Find.pm
> 
> would no longer be ambiguous thanks to the added space.
> 
> Now, I believe that !  should be trumped by :  if both are present in the
> same line without intervening spaces.
> 
> This means that standard Makefile lines   won't be interpreted differently
> if run under our make.
> 
> This is what this patch achieves.
> 
> okay? objections ?
> 
> Index: parse.c
> ===
> RCS file: /cvs/src/usr.bin/make/parse.c,v
> retrieving revision 1.132
> diff -u -p -r1.132 parse.c
> --- parse.c   26 Jan 2020 12:41:21 -  1.132
> +++ parse.c   9 Apr 2020 16:30:17 -
> @@ -598,7 +598,10 @@ found_delimiter(const char *s)
>   do {
>   p += strcspn(p, "!:");
>   if (*p == '\0')
> - break;
> + break;
> + /* always prefer : to ! if not space separated */
> + if (*p == ':' && *s == '!')
> + return false;
>   p++;
>   } while (*p != '\0' && !ISSPACE(*p));
>  
> 
> 
Ping again. I know this is somewhat esoteric, but this actually came up on
ports, where go distfiles do have ! in their names.

Very specifically, we have support for ! as a bsd extension (see the manpage,
it forces re-execution).

This does mean that some standard makefiles on other systems will behave
strangely on OpenBSD.

However, ambiguous lines are somewhat unlikely.  What this does is extend
the rule that arbitrates ambiguities.

- if an operator is followed by a space, it will be the operator. So in
a line like

a! somedependencywitha:

then ! is the make operator

- if there is no space, we will always choose the standard operator, so
some!go!module:
will recognize : as the default operator.


The ! BSD make semantics is rarely used, but I've seen it a few times, and
I think this patch is actually fairly safe.  But I need eyes and okays...



make patch: prefer standard behavior

2020-04-09 Thread Marc Espie
Our make (bsd make) has "alternate" dependency operators :: and !

Twenty years ago, we made it so that ambiguous constructs like
File::Find.3p: File/Find.pm

would no longer be ambiguous thanks to the added space.

Now, I believe that !  should be trumped by :  if both are present in the
same line without intervening spaces.

This means that standard Makefile lines   won't be interpreted differently
if run under our make.

This is what this patch achieves.

okay? objections ?

Index: parse.c
===
RCS file: /cvs/src/usr.bin/make/parse.c,v
retrieving revision 1.132
diff -u -p -r1.132 parse.c
--- parse.c 26 Jan 2020 12:41:21 -  1.132
+++ parse.c 9 Apr 2020 16:30:17 -
@@ -598,7 +598,10 @@ found_delimiter(const char *s)
do {
p += strcspn(p, "!:");
if (*p == '\0')
-   break;
+   break;
+   /* always prefer : to ! if not space separated */
+   if (*p == ':' && *s == '!')
+   return false;
p++;
} while (*p != '\0' && !ISSPACE(*p));
 



Re: find(1) -exec + and ARG_MAX

2020-04-09 Thread Marc Espie
On Thu, Apr 09, 2020 at 02:44:14PM +0200, Jeremie Courreges-Anglas wrote:
> On Thu, Apr 09 2020, Jeremie Courreges-Anglas  wrote:
> > find(1) uses ARG_MAX to compute the maximum space it can pass to
> > execve(2).  This doesn't fly if userland and the kernel don't agree, as
> > noticed by some after the recent ARG_MAX bump.
> >
> > --8<--
> > ritchie /usr/src/usr.bin/find$ obj/find /usr/src/ -type f -exec true {} +
> > find: true: Argument list too long
> > find: true: Argument list too long
> > find: true: Argument list too long
> > ^C
> > -->8--
> >
> > While having the kernel and userland out of sync is not a good idea,
> > making find(1) more robust by using sysconf(3) is easy.  This is what
> > xargs(1) already does.
> >
> > ok?
> 
> Committed, thanks.
> 
> > PS: a followup diff will take into account the space taken by the
> > environment
> 
> Right now we don't account for the space used by the environment.
> We get lucky either because of the 5000 max args limit or because
> environment size usually fits in the 4096 bytes safety net.
> 
> The diff below uses the same approach as xargs(1).
> While here, tweak the message in case of (unlikely) sysconf(3) failure.

You drop a bit of the comment in find, specifically the 4K stuff.

I would probably restore it.

Note that there is an explanation for the overflow in the kernel code:
MAXINTERP + MAXPATHLEN

exec does not reallocate args for scripts, but requires the extra space to
set things up.



xorg packaging issue

2020-03-24 Thread Marc Espie
There's some inconsistency:
lists/xserv/mi:./usr/X11R6/share/aclocal/xorg-macros.m4
lists/xshare/mi:./usr/X11R6/lib/pkgconfig/xorg-macros.pc

both should be in xshare

This does explain the failures of tightvnc on some bulk machines.
By default, proot does NOT copy xserv in the chroot, and I believe
it's correct.



Re: tar confused error messages

2020-03-22 Thread Marc Espie
On Sun, Mar 22, 2020 at 01:09:17PM +0100, Denis Fondras wrote:
> On Sun, Mar 22, 2020 at 11:41:55AM +0100, Marc Espie wrote:
> > If tar can't create intermediate directories due to permission
> > issues, the resulting message is confusing:
> > 
> > ./tar xf gcc.tar gcc-8.3.0/include/obstack.h 
> > tar: Unable to create gcc-8.3.0/include/obstack.h: No such file or directory
> > 
> > (here I have gcc-8.3.0 owned by root and no permissions)
> > 
> > The following patch changes this to:
> > 
> > ./tar xf gcc.tar gcc-8.3.0/include/obstack.h 
> > tar: Unable to create gcc-8.3.0/include: Permission denied
> > tar: Unable to create gcc-8.3.0/include/obstack.h: No such file or directory
> > 
> 
> Why not use errno on line 106 of file_subs.c ?
> That would yield "Permission denied" on obstack.h and would avoid having 2 
> error
> messages.

... except that the error is not on obstack.h, but on a specific directory 
creation.



tar confused error messages

2020-03-22 Thread Marc Espie
If tar can't create intermediate directories due to permission
issues, the resulting message is confusing:

./tar xf gcc.tar gcc-8.3.0/include/obstack.h 
tar: Unable to create gcc-8.3.0/include/obstack.h: No such file or directory

(here I have gcc-8.3.0 owned by root and no permissions)

The following patch changes this to:

./tar xf gcc.tar gcc-8.3.0/include/obstack.h 
tar: Unable to create gcc-8.3.0/include: Permission denied
tar: Unable to create gcc-8.3.0/include/obstack.h: No such file or directory

okay ? or a better way to do this ?

Index: extern.h
===
RCS file: /cvs/src/bin/pax/extern.h,v
retrieving revision 1.59
diff -u -p -r1.59 extern.h
--- extern.h13 Sep 2018 12:33:43 -  1.59
+++ extern.h22 Mar 2020 10:39:53 -
@@ -128,7 +128,7 @@ int cross_lnk(ARCHD *);
 int chk_same(ARCHD *);
 int node_creat(ARCHD *);
 int unlnk_exist(char *, int);
-int chk_path(char *, uid_t, gid_t);
+int chk_path(char *, uid_t, gid_t, int);
 void set_ftime(const char *, const struct timespec *,
 const struct timespec *, int);
 void fset_ftime(const char *, int, const struct timespec *,
Index: file_subs.c
===
RCS file: /cvs/src/bin/pax/file_subs.c,v
retrieving revision 1.54
diff -u -p -r1.54 file_subs.c
--- file_subs.c 28 Jun 2019 13:34:59 -  1.54
+++ file_subs.c 22 Mar 2020 10:39:53 -
@@ -102,7 +102,7 @@ file_creat(ARCHD *arcn)
file_mode)) >= 0)
break;
oerrno = errno;
-   if (nodirs || 
chk_path(arcn->name,arcn->sb.st_uid,arcn->sb.st_gid) < 0) {
+   if (nodirs || 
chk_path(arcn->name,arcn->sb.st_uid,arcn->sb.st_gid, 0) < 0) {
syswarn(1, oerrno, "Unable to create %s", arcn->name);
return(-1);
}
@@ -316,7 +316,7 @@ mk_link(char *to, struct stat *to_sb, ch
if (linkat(AT_FDCWD, to, AT_FDCWD, from, 0) == 0)
break;
oerrno = errno;
-   if (!nodirs && chk_path(from, to_sb->st_uid, to_sb->st_gid) == 
0)
+   if (!nodirs && chk_path(from, to_sb->st_uid, to_sb->st_gid, 
ign) == 0)
continue;
if (!ign) {
syswarn(1, oerrno, "Could not link to %s from %s", to,
@@ -458,7 +458,7 @@ badlink:
if (++pass <= 1)
continue;
 
-   if (nodirs || chk_path(nm,arcn->sb.st_uid,arcn->sb.st_gid) < 0) 
{
+   if (nodirs || chk_path(nm,arcn->sb.st_uid,arcn->sb.st_gid, 0) < 
0) {
syswarn(1, oerrno, "Could not create: %s", nm);
return(-1);
}
@@ -590,7 +590,7 @@ unlnk_exist(char *name, int type)
  */
 
 int
-chk_path(char *name, uid_t st_uid, gid_t st_gid)
+chk_path(char *name, uid_t st_uid, gid_t st_gid, int ign)
 {
char *spt = name;
char *next;
@@ -643,6 +643,8 @@ chk_path(char *name, uid_t st_uid, gid_t
 * needed directory and continue on
 */
if (mkdir(name, S_IRWXU | S_IRWXG | S_IRWXO) == -1) {
+   if (!ign)
+   syswarn(1, errno, "Unable to create %s", name); 
*spt = '/';
retval = -1;
break;



Re: mention /etc/examples/ in bgpf.conf(5)/bgpd(8)

2020-02-10 Thread Marc Espie
On Sun, Feb 09, 2020 at 01:37:33PM +, Jason McIntyre wrote:
> On Sun, Feb 09, 2020 at 02:27:23PM +0100, Marc Espie wrote:
> > On Sun, Feb 09, 2020 at 12:41:43AM +0100, Sebastian Benoit wrote:
> > > Ingo Schwarze(schwa...@usta.de) on 2020.02.09 00:33:06 +0100:
> > > > Hi,
> > > > 
> > > > Jason McIntyre wrote on Sat, Feb 08, 2020 at 10:15:08PM +:
> > > > 
> > > > > - i'm ok with adding the path to these files to a FILES section
> > > > 
> > > > So, here is a specific patch for bgpf.conf(5) and bgpd(8) such
> > > > that we can agree on a general direction for one case where
> > > > the example file is particularly important.
> > > > 
> > > > Once the direction is agreed, applying it to the relatively
> > > > small number of other cases is likely not difficult.
> > > > 
> > > > OK?
> > > >   Ingo
> > > 
> > > I'm not against this as it only adds to the manpage, but an
> > > alternative could be to just tell users (in afterboot(8)?) that there is
> > > /etc/examples for them to look at and see whats there?
> > > 
> > > Otherwise ok.
> > 
> > I still think it's a good idea to put it in afterboot(8).
> > That was the patch I sent to a few people that obviously triggered Ingo.
> > 
> > As for /etc/examples, I still think that directory has merits.
> > It's WAY easier to copy an example file and uncomment/tweak the parts you
> > need while skimming through the manpage than creating one from scratch.
> > 
> > Also, cut from an EXAMPLE in the manpage is distasteful.
> > 
> > Yeah, it's more info that has to be kept in synch. So what ? It's still
> > useful, so it should stay.
> > 
> > Here's an updated version, taking Theo's remarks into account.
> > 
> 
> hi. i'm fine with your text, but it would be better placed in the
> section "Daemons" (yes i'm aware not all the files are for daemons)
> or a new section in "FURTHER CHANGES".
> 
> jmc

Sorry, I saw your message a bit late.

I don't think it's a good idea.  Specifically, that text should come
in as early as possible.

"FURTHER CHANGES" sound like a section near the end.
"Daemons" also comes very late in the game
(after ntpd, after sysctl.conf, which do have /etc/examples scripts)

also, you should update, as your email setup is broken.
any direct email to you have come back with
: 553 ORCPT address syntax error

which according to Stuart, has been fixed in recent snaps.



Re: mention /etc/examples/ in bgpf.conf(5)/bgpd(8)

2020-02-09 Thread Marc Espie
On Sun, Feb 09, 2020 at 12:41:43AM +0100, Sebastian Benoit wrote:
> Ingo Schwarze(schwa...@usta.de) on 2020.02.09 00:33:06 +0100:
> > Hi,
> > 
> > Jason McIntyre wrote on Sat, Feb 08, 2020 at 10:15:08PM +:
> > 
> > > - i'm ok with adding the path to these files to a FILES section
> > 
> > So, here is a specific patch for bgpf.conf(5) and bgpd(8) such
> > that we can agree on a general direction for one case where
> > the example file is particularly important.
> > 
> > Once the direction is agreed, applying it to the relatively
> > small number of other cases is likely not difficult.
> > 
> > OK?
> >   Ingo
> 
> I'm not against this as it only adds to the manpage, but an
> alternative could be to just tell users (in afterboot(8)?) that there is
> /etc/examples for them to look at and see whats there?
> 
> Otherwise ok.

I still think it's a good idea to put it in afterboot(8).
That was the patch I sent to a few people that obviously triggered Ingo.

As for /etc/examples, I still think that directory has merits.
It's WAY easier to copy an example file and uncomment/tweak the parts you
need while skimming through the manpage than creating one from scratch.

Also, cut from an EXAMPLE in the manpage is distasteful.

Yeah, it's more info that has to be kept in synch. So what ? It's still
useful, so it should stay.

Here's an updated version, taking Theo's remarks into account.

Index: afterboot.8
===
RCS file: /cvs/src/share/man/man8/afterboot.8,v
retrieving revision 1.164
diff -u -p -r1.164 afterboot.8
--- afterboot.8 28 Sep 2018 18:21:31 -  1.164
+++ afterboot.8 9 Feb 2020 13:26:29 -
@@ -60,6 +60,10 @@ command, type:
 Administrators will rapidly become more familiar with
 .Ox
 if they get used to using the high quality manual pages.
+.Pp
+Some base programs and subsystems also come with sample configuration 
+files in
+.Pa /etc/examples .
 .Ss Errata
 By the time that you have installed your system, it is possible that
 bugs in the release have been found.



Re: [patch] /bin/cp: reduce scope variable

2020-02-02 Thread Marc Espie
On Sat, Feb 01, 2020 at 04:57:26PM +0100, Ingo Schwarze wrote:
> Hi Jeremie,
> 
> Jeremie Courreges-Anglas wrote on Sat, Feb 01, 2020 at 01:37:32PM +0100:
> > On Fri, Jan 31 2020, Ingo Schwarze  wrote:
> >> ngc...@gmail.com wrote on Fri, Jan 31, 2020 at 10:14:52PM +0900:
> 
> >>> Reduce scope of a few variables.
> 
> >> No, this contradicts OpenBSD coding style.
> >> We want local variables declared up front in functions
> >> such that you can see at one glance which variables exist.
> 
> > Huh, this is your personal preference and I strongly disagree with
> > making it an "official" stance (again).  Reducing the scope of variables
> > *quite often* helps reasoning about them.
> > 
> > style(9) said something like that, and has been changed three years
> > ago (for good IMO).
> > 
> >   revision 1.68
> >   date: 2016/10/18 18:13:56;  author: millert;  state: Exp;  lines: +2 -4;  
> > commitid: aPPHgmmA4hseZUFx;
> >   Don't tell the programmer not to put variable declarations inside
> >   blocks.  OK guenther@ kettenis@
> 
> Oops.  Sorry for forgetting that the recommendation was dropped and
> for making you dig up the commit and thanks for reminding me.

Since it came out in the open, I also like declaring variables as late
as possible. Specifically, because the old-style (declare variables early,
then give them a value when possible) tends to be unsafe. You forget about
initializing variables, or you initialize them to something that doesn't
make sense.

I also don't think it's more readable to have every variable at the start
of the function.   The less space my eyes have to travel the better.
Especially with functions that run 50 lines long.

Also, reducing the scope of variables tends to be one of the first steps
in untangling old atrocious code: reduce scope, figure out you got two
separate blocks in your function with totally separate variables, and then
you write a new function.

Compilers aren't perfect yet, but we still deal with code that was written
20-30 years ago, back when they were even worse, and a lot of dirty tricks
that were used back then are no longer needed.  Reusing variables, having
the compiler match your scopes in asm exactly, you name it. These days,
scope is only a semantic promise that this variable only matters in THAT
specific block of code. The stronger the promise, the more readable 
the code.



Re: sysupgrade should check for downgrades

2020-01-30 Thread Marc Espie
On Thu, Jan 30, 2020 at 11:14:37AM +, Stuart Henderson wrote:
> Putting this here for discussion... good idea? bad idea? does it need
> more checks for expected file contents?
> 
> Index: sysupgrade.sh
> ===
> RCS file: /cvs/src/usr.sbin/sysupgrade/sysupgrade.sh,v
> retrieving revision 1.37
> diff -u -p -r1.37 sysupgrade.sh
> --- sysupgrade.sh 26 Jan 2020 22:08:36 -  1.37
> +++ sysupgrade.sh 30 Jan 2020 10:56:52 -
> @@ -131,6 +131,7 @@ cd ${SETSDIR}
>  
>  echo "Fetching from ${URL}"
>  unpriv -f SHA256.sig ftp -N sysupgrade -Vmo SHA256.sig ${URL}SHA256.sig
> +unpriv -f BUILDINFO ftp -N sysupgrade -Vmo BUILDINFO ${URL}BUILDINFO
>  
>  _KEY=openbsd-${_KERNV[0]%.*}${_KERNV[0]#*.}-base.pub
>  _NEXTKEY=openbsd-${NEXT_VERSION%.*}${NEXT_VERSION#*.}-base.pub
> @@ -147,11 +148,26 @@ esac
>  unpriv -f SHA256 signify -Ve -p "${SIGNIFY_KEY}" -x SHA256.sig -m SHA256
>  rm SHA256.sig
>  
> +unpriv cksum -qC SHA256 BUILDINFO
> +
>  if cmp -s /var/db/installed.SHA256 SHA256 && ! $FORCE; then
>   echo "Already on latest snapshot."
>   exit 0
>  fi
>  
> +if [[ -r /var/db/installed.BUILDINFO ]] && ! $FORCE; then
> + read _skip _skip _oldbuildtime _skip < /var/db/installed.BUILDINFO
> + read _skip _skip _newbuildtime _skip < BUILDINFO
> + if [[ $_newbuildtime -lt $_oldbuildtime ]]; then
> + echo "Snapshot on mirror is older than installed version!"
> + exit 1
> + fi
> + if [[ $_newbuildtime -eq $_oldbuildtime ]]; then
> + echo "Already on latest snapshot? Mismatch between BUILDINFO 
> and SHA256?"
> + exit 1
> + fi
> +fi
> +
>  # INSTALL.*, bsd*, *.tgz
>  SETS=$(sed -n -e 's/^SHA256 (\(.*\)) .*/\1/' \
>  -e '/^INSTALL\./p;/^bsd/p;/\.tgz$/p' SHA256)
> @@ -187,9 +203,14 @@ Set name(s) = done
>  Directory does not contain SHA256.sig. Continue without verification = yes
>  __EOT
>  
> +# XXX should be done in bsd.rd so that this is present for a clean install 
> too
> +cat <<__EOT > /etc/rc.firsttime
> +cp /home/_sysupgrade/BUILDINFO /var/db/installed.BUILDINFO
> +__EOT
> +
>  if ! ${KEEP}; then
>   CLEAN=$(echo SHA256 ${SETS} | sed -e 's/ /,/g')
> - cat <<__EOT > /etc/rc.firsttime
> + cat <<__EOT >> /etc/rc.firsttime
>  rm -f /home/_sysupgrade/{${CLEAN}}
>  __EOT
>  fi
> 
> 
I think it's a good idea.  A "bad" mirror could intentionally keep
snapshots around waiting for a critical bug to hit us, and then swap back
to an older snapshot that would be vulnerable.  It would probably get
noticed reasonably quick, but not before it could make some damage.



waitpid vs ptrace

2020-01-30 Thread Marc Espie
So, basically, if we start arbitrary commands, then
the classic loop 
/* Wait for the child to exit.  */
while (waitpid(cpid, , 0) == -1 && errno == EINTR)
continue;


is not quite enough.

See the small note in manpages (not only us, but everyone):
 WIFSTOPPED(status)
 True if the process has not terminated, but has stopped and can
 be restarted.  This macro can be true only if the wait call
 specified the WUNTRACED option or if the child process is being
 traced (see ptrace(2)).

this means that somebody could run a command that forks, and then its child
(our grand-child) could use ptrace on its parent, and then we would get
a notification with WIFSTOPPED (assuming the current kernel bug is fixed).

e.g., something like that:

while (1) {
if (waitpid(cpid, , 0) == -1) {
if (errno == EINTR)
continue;
} else {
if (WIFSTOPPED(status))
continue;
}
break;
}

or should we just assume no-one will be nasty enough to do that ?



Re: MAKE: some older keywords

2020-01-21 Thread Marc Espie
So I did a full bulk build with the following diff.
No failure due to old keyword.

This contains exactly:
- abort directly instead of setting pgn->must_make = false
- tag as "SPECIAL_DEPRECATED" all old keywords
- remove the conditionals with those OP. We just keep OP_INVISIBLE  because
it's actually also used by cohorts (those pesky :: dependencies in imake)



diff --git a/compat.c b/compat.c
index fd78d78..af18ce3 100644
--- a/compat.c
+++ b/compat.c
@@ -101,24 +101,13 @@ CompatMake(void *gnp, /* The node to make */
if (pgn == NULL)
pgn = gn;
 
-   if (pgn->type & OP_MADE) {
-   sib = gn;
-   do {
-   sib->mtime = gn->mtime;
-   sib->built_status = UPTODATE;
-   sib = sib->sibling;
-   } while (sib != gn);
-   }
-
switch(gn->built_status) {
case UNKNOWN: 
/* First mark ourselves to be built, then apply whatever
 * transformations the suffix module thinks are necessary.
 * Once that's done, we can descend and make all our children.
 * If any of them has an error but the -k flag was given,
-* our 'must_make' field will be set false again.  This is our
-* signal to not attempt to do anything but abort our
-* parent as well.  */
+* we will abort. */
gn->must_make = true;
gn->built_status = BUILDING;
/* note that, in case we have siblings, we only check all
@@ -132,10 +121,9 @@ CompatMake(void *gnp,  /* The node to make */
sib = sib->sibling;
} while (sib != gn);
 
-   if (!gn->must_make) {
+   if (gn->built_status == ABORTED) {
Error("Build for %s aborted", gn->name);
-   gn->built_status = ABORTED;
-   pgn->must_make = false;
+   pgn->built_status = ABORTED;
return;
}
 
@@ -228,12 +216,10 @@ CompatMake(void *gnp, /* The node to make */
if (DEBUG(MAKE))
printf("update time: %s\n",
time_to_string(>mtime));
-   if (!(gn->type & OP_EXEC)) {
-   pgn->child_rebuilt = true;
-   Make_TimeStamp(pgn, gn);
-   }
+   pgn->child_rebuilt = true;
+   Make_TimeStamp(pgn, gn);
} else if (keepgoing)
-   pgn->must_make = false;
+   pgn->built_status = ABORTED;
else {
print_errors();
exit(1);
@@ -242,22 +228,19 @@ CompatMake(void *gnp, /* The node to make */
case ERROR:
/* Already had an error when making this beastie. Tell the
 * parent to abort.  */
-   pgn->must_make = false;
+   pgn->built_status = ABORTED;
break;
case BUILDING:
Error("Graph cycles through %s", gn->name);
gn->built_status = ERROR;
-   pgn->must_make = false;
+   pgn->built_status = ABORTED;
break;
case REBUILT:
-   if ((gn->type & OP_EXEC) == 0) {
-   pgn->child_rebuilt = true;
-   Make_TimeStamp(pgn, gn);
-   }
+   pgn->child_rebuilt = true;
+   Make_TimeStamp(pgn, gn);
break;
case UPTODATE:
-   if ((gn->type & OP_EXEC) == 0)
-   Make_TimeStamp(pgn, gn);
+   Make_TimeStamp(pgn, gn);
break;
default:
break;
diff --git a/dump.c b/dump.c
index b3820eb..f35cacf 100644
--- a/dump.c
+++ b/dump.c
@@ -117,7 +117,7 @@ TargPrintNode(GNode *gn, bool full)
}
if (full) {
printf("# %d unmade prerequisites\n", gn->children_left);
-   if (! (gn->type & (OP_JOIN|OP_USE|OP_EXEC))) {
+   if (! (gn->type & OP_USE)) {
if (!is_out_of_date(gn->mtime)) {
printf("# last modified %s: %s\n",
  time_to_string(>mtime),
diff --git a/engine.c b/engine.c
index 535d8dd..4d7c128 100644
--- a/engine.c
+++ b/engine.c
@@ -241,7 +241,7 @@ void
 Job_Touch(GNode *gn)
 {
handle_all_signals();
-   if (gn->type & (OP_JOIN|OP_USE|OP_EXEC|OP_OPTIONAL|OP_PHONY)) {
+   if (gn->type & (OP_USE|OP_OPTIONAL|OP_PHONY)) {
/*
 * .JOIN, .USE, and .OPTIONAL targets are "virtual" targets
 * and, as such, shouldn't really be created.
@@ -347,7 +347,7 @@ Make_DoAllVar(GNode *gn)
 

Re: MAKE: some older keywords

2020-01-15 Thread Marc Espie
More specifically, I'm running with this patch

The specific choice of keywords to deprecate is up for grabs, the 
infrastructure for actually being able to error out on these keywords is
probably something I should commit anyhow.



diff --git a/gnode.h b/gnode.h
index 283fead..04e5462 100644
--- a/gnode.h
+++ b/gnode.h
@@ -77,6 +77,8 @@
 #define SPECIAL_NOTHING6U  /* this is used for things we
 * recognize for compatibility but
 * don't do anything with... */
+#define SPECIAL_DEPRECATED 7U  /* this is an old keyword and it will
+* trigger a fatal error. */
 #define SPECIAL_INVISIBLE  8U
 #define SPECIAL_JOIN   9U
 #define SPECIAL_MADE   11U
diff --git a/parse.c b/parse.c
index dfc2abc..555d2cd 100644
--- a/parse.c
+++ b/parse.c
@@ -184,13 +184,13 @@ static struct {
unsigned int special;
unsigned int special_op;
 } specials[] = {
-{ P(NODE_EXEC),SPECIAL_EXEC,   OP_EXEC },
+{ P(NODE_EXEC),SPECIAL_DEPRECATED, OP_EXEC },
 { P(NODE_IGNORE),  SPECIAL_IGNORE, OP_IGNORE },
-{ P(NODE_INCLUDES),SPECIAL_NOTHING,0 },
-{ P(NODE_INVISIBLE),   SPECIAL_INVISIBLE,  OP_INVISIBLE },
-{ P(NODE_JOIN),SPECIAL_JOIN,   OP_JOIN },
-{ P(NODE_LIBS),SPECIAL_NOTHING,0 },
-{ P(NODE_MADE),SPECIAL_MADE,   OP_MADE },
+{ P(NODE_INCLUDES),SPECIAL_DEPRECATED, 0 },
+{ P(NODE_INVISIBLE),   SPECIAL_DEPRECATED, OP_INVISIBLE },
+{ P(NODE_JOIN),SPECIAL_DEPRECATED, OP_JOIN },
+{ P(NODE_LIBS),SPECIAL_DEPRECATED, 0 },
+{ P(NODE_MADE),SPECIAL_DEPRECATED, OP_MADE },
 { P(NODE_MAIN),SPECIAL_MAIN,   0 },
 { P(NODE_MAKE),SPECIAL_MAKE,   OP_MAKE },
 { P(NODE_MAKEFLAGS),   SPECIAL_MFLAGS, 0 },
@@ -198,7 +198,7 @@ static struct {
 { P(NODE_NOTMAIN), SPECIAL_NOTMAIN,OP_NOTMAIN },
 { P(NODE_NOTPARALLEL), SPECIAL_NOTPARALLEL,0 },
 { P(NODE_NO_PARALLEL), SPECIAL_NOTPARALLEL,0 },
-{ P(NODE_NULL),SPECIAL_NOTHING,0 },
+{ P(NODE_NULL),SPECIAL_DEPRECATED, 0 },
 { P(NODE_OPTIONAL),SPECIAL_OPTIONAL,   OP_OPTIONAL },
 { P(NODE_ORDER),   SPECIAL_ORDER,  0 },
 { P(NODE_PARALLEL),SPECIAL_PARALLEL,   0 },
@@ -418,6 +418,11 @@ ParseDoSrc(
 const char *esrc)
 {
GNode *gn = Targ_FindNodei(src, esrc, TARG_CREATE);
+   if (gn->special == SPECIAL_DEPRECATED) {
+   Parse_Error(PARSE_FATAL, "Deprecated keyword found %s\n",
+   gn->name);
+   return;
+   }
if (gn->special_op) {
Array_ForEach(targets, ParseDoSpecial, gn->special_op);
return;
@@ -702,6 +707,13 @@ handle_special_targets(Lst paths)
 
for (i = 0; i < gtargets.n; i++) {
type = gtargets.a[i]->special;
+   if (type == SPECIAL_DEPRECATED) {
+   Parse_Error(PARSE_FATAL, 
+   "Deprecated keyword found %s\n",
+   gtargets.a[i]->name);
+   specType = SPECIAL_ERROR;
+   return 0;
+   }
if (type == SPECIAL_PATH) {
seen_path++;
Lst_AtEnd(paths, find_suffix_path(gtargets.a[i]));



MAKE: simplify compat handling

2020-01-14 Thread Marc Espie
The convoluted logic that resets must_make does not make any sense to me.
It's just as simple to set built_status to ABORTED directly.

Note that in the compat make case, there are two instances of using
must_make left, one in arch.c, which I have yet to understand, and one
in node_failure/list_parents.

diff --git a/compat.c b/compat.c
index fd78d78..9173f44 100644
--- a/compat.c
+++ b/compat.c
@@ -116,9 +116,7 @@ CompatMake(void *gnp,   /* The node to make */
 * transformations the suffix module thinks are necessary.
 * Once that's done, we can descend and make all our children.
 * If any of them has an error but the -k flag was given,
-* our 'must_make' field will be set false again.  This is our
-* signal to not attempt to do anything but abort our
-* parent as well.  */
+* we will abort. */
gn->must_make = true;
gn->built_status = BUILDING;
/* note that, in case we have siblings, we only check all
@@ -132,10 +130,9 @@ CompatMake(void *gnp,  /* The node to make */
sib = sib->sibling;
} while (sib != gn);
 
-   if (!gn->must_make) {
+   if (gn->built_status == ABORTED) {
Error("Build for %s aborted", gn->name);
-   gn->built_status = ABORTED;
-   pgn->must_make = false;
+   pgn->built_status = ABORTED;
return;
}
 
@@ -233,7 +230,7 @@ CompatMake(void *gnp,   /* The node to make */
Make_TimeStamp(pgn, gn);
}
} else if (keepgoing)
-   pgn->must_make = false;
+   pgn->built_status = ABORTED;
else {
print_errors();
exit(1);
@@ -242,12 +239,12 @@ CompatMake(void *gnp, /* The node to make */
case ERROR:
/* Already had an error when making this beastie. Tell the
 * parent to abort.  */
-   pgn->must_make = false;
+   pgn->built_status = ABORTED;
break;
case BUILDING:
Error("Graph cycles through %s", gn->name);
gn->built_status = ERROR;
-   pgn->must_make = false;
+   pgn->built_status = ABORTED;
break;
case REBUILT:
if ((gn->type & OP_EXEC) == 0) {



Re: MAKE: redux patch

2020-01-11 Thread Marc Espie
On Fri, Jan 10, 2020 at 10:52:03PM +0100, Alexander Bluhm wrote:
> On Fri, Jan 10, 2020 at 01:58:47PM +0100, Marc Espie wrote:
> > Bleh, I forgot to synch two patches I already committed. Here's a patch
> > that applies cleanly.
> 
> I did run this make through a full regress.  It seems that make
> regress in /usr/src/regress/usr.sbin/ldapd/ exits with an error.
> 
> http://bluhm.genua.de/regress/results/2020-01-10T14%3A11%3A58Z/logs/usr.sbin/ldapd/make.log
> 
> *** Error 1 in /usr/src/regress/usr.sbin/ldapd (Makefile:44 '.END': @[ -f 
> /usr/src/regress/usr.sbin/ldapd/obj/ldapd.pid ] &&  kill $(cat /us...)
> 
> I have never seen this problem before.
> 
> bluhm

Oh, the test is wrong, and it's now enough to have make complain about it.

Before the patch, errors in .END and .BEGIN were not properly looked at.

See, that -f ... &&   will be *false* if the file doesn't exist.
The way to this kind of test without failing is

if [ -f ... ]; then ... fi


Index: Makefile
===
RCS file: /cvs/src/regress/usr.sbin/ldapd/Makefile,v
retrieving revision 1.9
diff -u -p -r1.9 Makefile
--- Makefile28 Jun 2018 02:47:55 -  1.9
+++ Makefile11 Jan 2020 11:29:35 -
@@ -39,9 +39,10 @@ bootstrap:
 
 .if ! (make(clean) || make(cleandir) || make(obj))
 .END:
-   @[ -f ${.OBJDIR}/ldapd.pid ] &&\
+   @if [ -f ${.OBJDIR}/ldapd.pid ]; then \
${SUDO} kill $$(cat ${.OBJDIR}/ldapd.pid) &&\
-   rm ${.OBJDIR}/ldapd.pid
+   rm ${.OBJDIR}/ldapd.pid; \
+   fi
 .endif
 
 connect: bootstrap



Re: MAKE: redux patch

2020-01-10 Thread Marc Espie
On Thu, Jan 09, 2020 at 01:09:59PM +0100, Marc Espie wrote:
> So my development branch is getting a bit too far
> ahead compared to what's committed.
> 
> Here's a big diff to test.  Comments as to what's going on
> and the changes this contains:
> 
> - buffer changes: add support for "permanent static buffers"
> that don't get reinit'd all the time (that's a mostly trivial
> change)
> 
> - parser change: SPECIAL_TARGET and SPECIAL_SOURCE are not
> really needed (figured out in netbsd)
> - (related): create the special nodes once in a simpler way
> 
> - a bit of reorg: take the code for expand_children out
> of suff.c proper, as this file is always fairly unwieldy.
> This only requires Link_Parent, and find_best_path, which
> are suff-independent.
> 
> - engine change: stop mixing parallel and compat so much.
> In particular, don't run Make_Update when in compat mode (which
> would happen because handle_all_signals goes into the main loop)
> 
> - (related): abstract the engine running into operations in
> 'enginechoice.c' so that it's perfectly clear what part is compatMake
> and what part is not.   Have it report out_of_date and errors in 
> a simple way.  Don't mix up .BEGIN/.END handling.
> 
> - (related): job.c determines whether to use the expensive heuristics
> or not depending on how many jobs we run. 
> 
> - jobrunner cleanup: we have no need for maxjobs, it's enough to move
> stuff around from available into running or error... do the move to 
> error later so that everything is in the same location.
> 
> - with the above changes: the special case of running_one_job is no
> longer needed at all.
> 
> - feature coming from bmake: treat .BEGIN/.END more as normal targets.
> More specifically, just invoke the engine on them if they exist.
> This makes it possible to have
> .BEGIN: somethingelse
> which does make a lot of sense, actually.
> 
> I would really need this to go in so that I can keep pushing forward.
> 
> I think I might take out the "old" parallel engine (which is somewhat
> broken, and frankly, I don't get how it can work) because modifying
> compat to actually queue stuff and build it  is a distinct possibility
> now.  e.g., having an actual parallel engine that works.

Bleh, I forgot to synch two patches I already committed. Here's a patch
that applies cleanly.

diff --git a/Makefile b/Makefile
index 0cd84fc..90747de 100644
--- a/Makefile
+++ b/Makefile
@@ -16,8 +16,8 @@ CFLAGS+=${CDEFS}
 HOSTCFLAGS+=${CDEFS}
 
 SRCS=  arch.c buf.c cmd_exec.c compat.c cond.c dir.c direxpand.c dump.c \
-   engine.c \
-   error.c for.c init.c job.c lowparse.c main.c make.c memory.c parse.c \
+   engine.c enginechoice.c error.c expandchildren.c \
+   for.c init.c job.c lowparse.c main.c make.c memory.c parse.c \
parsevar.c str.c stats.c suff.c targ.c targequiv.c timestamp.c \
var.c varmodifiers.c varname.c
 
diff --git a/arch.c b/arch.c
index 85e8e7e..02c18b5 100644
--- a/arch.c
+++ b/arch.c
@@ -195,11 +195,10 @@ bool
 Arch_ParseArchive(const char **line, Lst nodes, SymTable *ctxt)
 {
bool result;
-   BUFFER expand;
+   static BUFFER expand;
 
-   Buf_Init(, MAKE_BSIZE);
+   Buf_Reinit(, MAKE_BSIZE);
result = parse_archive(, line, nodes, ctxt);
-   Buf_Destroy();
return result;
 }
 
diff --git a/buf.c b/buf.c
index 74cbfe8..931bd7e 100644
--- a/buf.c
+++ b/buf.c
@@ -153,6 +153,15 @@ Buf_printf(Buffer bp, const char *fmt, ...)
bp->inPtr += n;
 }
 
+void
+Buf_Reinit(Buffer bp, size_t size)
+{
+   if (bp->buffer == NULL)
+   Buf_Init(bp, size);
+   else
+   Buf_Reset(bp);
+}
+
 void
 Buf_Init(Buffer bp, size_t size)
 {
diff --git a/buf.h b/buf.h
index 1b56b27..20ea56a 100644
--- a/buf.h
+++ b/buf.h
@@ -106,6 +106,9 @@ extern void Buf_AddChars(Buffer, size_t, const char *);
  * Initializes a buffer, to hold approximately init chars.
  * Set init to 0 if you have no idea.  */
 extern void Buf_Init(Buffer, size_t);
+/* Buf_Reinit(buf, init);
+ * Initializes/reset a static buffer */
+extern void Buf_Reinit(Buffer, size_t);
 /* Buf_Destroy(buf);
  * Nukes a buffer and all its resources.   */
 #define Buf_Destroy(bp) ((void)free((bp)->buffer))
diff --git a/compat.c b/compat.c
index 7ecb75f..fd78d78 100644
--- a/compat.c
+++ b/compat.c
@@ -193,14 +193,12 @@ CompatMake(void *gnp, /* The node to make */
/* copy over what we just did */
gn->built_status = sib->built_status;
 
-   if (gn->built_status != ERROR) {
-   /* If the node was built successfully, mark it so,
+   if (gn->built_status == REBUILT) {
+   /* If the node was built successfully, 
 * update it

MAKE: redux patch

2020-01-09 Thread Marc Espie
So my development branch is getting a bit too far
ahead compared to what's committed.

Here's a big diff to test.  Comments as to what's going on
and the changes this contains:

- buffer changes: add support for "permanent static buffers"
that don't get reinit'd all the time (that's a mostly trivial
change)

- parser change: SPECIAL_TARGET and SPECIAL_SOURCE are not
really needed (figured out in netbsd)
- (related): create the special nodes once in a simpler way

- a bit of reorg: take the code for expand_children out
of suff.c proper, as this file is always fairly unwieldy.
This only requires Link_Parent, and find_best_path, which
are suff-independent.

- engine change: stop mixing parallel and compat so much.
In particular, don't run Make_Update when in compat mode (which
would happen because handle_all_signals goes into the main loop)

- (related): abstract the engine running into operations in
'enginechoice.c' so that it's perfectly clear what part is compatMake
and what part is not.   Have it report out_of_date and errors in 
a simple way.  Don't mix up .BEGIN/.END handling.

- (related): job.c determines whether to use the expensive heuristics
or not depending on how many jobs we run. 

- jobrunner cleanup: we have no need for maxjobs, it's enough to move
stuff around from available into running or error... do the move to 
error later so that everything is in the same location.

- with the above changes: the special case of running_one_job is no
longer needed at all.

- feature coming from bmake: treat .BEGIN/.END more as normal targets.
More specifically, just invoke the engine on them if they exist.
This makes it possible to have
.BEGIN: somethingelse
which does make a lot of sense, actually.

I would really need this to go in so that I can keep pushing forward.

I think I might take out the "old" parallel engine (which is somewhat
broken, and frankly, I don't get how it can work) because modifying
compat to actually queue stuff and build it  is a distinct possibility
now.  e.g., having an actual parallel engine that works.


diff --git a/Makefile b/Makefile
index 0cd84fc..90747de 100644
--- a/Makefile
+++ b/Makefile
@@ -16,8 +16,8 @@ CFLAGS+=${CDEFS}
 HOSTCFLAGS+=${CDEFS}
 
 SRCS=  arch.c buf.c cmd_exec.c compat.c cond.c dir.c direxpand.c dump.c \
-   engine.c \
-   error.c for.c init.c job.c lowparse.c main.c make.c memory.c parse.c \
+   engine.c enginechoice.c error.c expandchildren.c \
+   for.c init.c job.c lowparse.c main.c make.c memory.c parse.c \
parsevar.c str.c stats.c suff.c targ.c targequiv.c timestamp.c \
var.c varmodifiers.c varname.c
 
diff --git a/arch.c b/arch.c
index 85e8e7e..02c18b5 100644
--- a/arch.c
+++ b/arch.c
@@ -195,11 +195,10 @@ bool
 Arch_ParseArchive(const char **line, Lst nodes, SymTable *ctxt)
 {
bool result;
-   BUFFER expand;
+   static BUFFER expand;
 
-   Buf_Init(, MAKE_BSIZE);
+   Buf_Reinit(, MAKE_BSIZE);
result = parse_archive(, line, nodes, ctxt);
-   Buf_Destroy();
return result;
 }
 
diff --git a/buf.c b/buf.c
index 74cbfe8..931bd7e 100644
--- a/buf.c
+++ b/buf.c
@@ -153,6 +153,15 @@ Buf_printf(Buffer bp, const char *fmt, ...)
bp->inPtr += n;
 }
 
+void
+Buf_Reinit(Buffer bp, size_t size)
+{
+   if (bp->buffer == NULL)
+   Buf_Init(bp, size);
+   else
+   Buf_Reset(bp);
+}
+
 void
 Buf_Init(Buffer bp, size_t size)
 {
diff --git a/buf.h b/buf.h
index 1b56b27..20ea56a 100644
--- a/buf.h
+++ b/buf.h
@@ -106,6 +106,9 @@ extern void Buf_AddChars(Buffer, size_t, const char *);
  * Initializes a buffer, to hold approximately init chars.
  * Set init to 0 if you have no idea.  */
 extern void Buf_Init(Buffer, size_t);
+/* Buf_Reinit(buf, init);
+ * Initializes/reset a static buffer */
+extern void Buf_Reinit(Buffer, size_t);
 /* Buf_Destroy(buf);
  * Nukes a buffer and all its resources.   */
 #define Buf_Destroy(bp) ((void)free((bp)->buffer))
diff --git a/compat.c b/compat.c
index 069e52f..fd78d78 100644
--- a/compat.c
+++ b/compat.c
@@ -193,14 +193,12 @@ CompatMake(void *gnp, /* The node to make */
/* copy over what we just did */
gn->built_status = sib->built_status;
 
-   if (gn->built_status != ERROR) {
-   /* If the node was built successfully, mark it so,
+   if (gn->built_status == REBUILT) {
+   /* If the node was built successfully, 
 * update its modification time and timestamp all
 * its parents.
 * This is to keep its state from affecting that of
 * its parent.  */
-   gn->built_status = REBUILT;
-   sib->built_status = REBUILT;
/* This is what Make does and it's actually a good
 * thing, as it allows rules like
 *
@@ 

MAKE: fix -q flag

2020-01-05 Thread Marc Espie
-q is just plain broken in compat mode, it's easy to fix though.

The second issue is that, if a Makefile as an .END target, that one
will always be run, even in query mode, contrary to .BEGIN.

So let's fix that as well.

Fairly obvious patch

Index: compat.c
===
RCS file: /cvs/src/usr.bin/make/compat.c,v
retrieving revision 1.88
diff -u -p -r1.88 compat.c
--- compat.c21 Dec 2019 15:29:25 -  1.88
+++ compat.c5 Jan 2020 13:43:32 -
@@ -266,11 +266,12 @@ CompatMake(void *gnp, /* The node to mak
}
 }
 
-void
+bool
 Compat_Run(Lst targs)  /* List of target nodes to re-create */
 {
GNode *gn = NULL;   /* Current root target */
int   errors;   /* Number of targets not built due to errors */
+   boolout_of_date = false;
 
/* For each entry in the list of targets to create, call CompatMake on
 * it to create the thing. CompatMake will leave the 'built_status'
@@ -291,11 +292,15 @@ Compat_Run(Lst targs) /* List of target
else if (gn->built_status == ABORTED) {
printf("`%s' not remade because of errors.\n",
gn->name);
+   out_of_date = true;
errors++;
+   } else {
+   out_of_date = true;
}
}
 
/* If the user has defined a .END target, run its commands.  */
-   if (errors == 0)
+   if (errors == 0 && !queryFlag)
run_gnode(end_node);
+   return out_of_date;
 }
Index: compat.h
===
RCS file: /cvs/src/usr.bin/make/compat.h,v
retrieving revision 1.3
diff -u -p -r1.3 compat.h
--- compat.h19 Jul 2010 19:46:43 -  1.3
+++ compat.h5 Jan 2020 13:43:32 -
@@ -35,8 +35,9 @@
  * - friendly variable substitution.
  */
 
-/* Compat_Run(to_create);
- * Run the actual make engine, to create targets that need to.  */
-extern void Compat_Run(Lst);
+/* out_of_date = Compat_Run(to_create);
+ * Run the actual make engine, to create targets that need to,
+ * return true if any target is out of date. */
+extern bool Compat_Run(Lst);
 
 #endif
Index: main.c
===
RCS file: /cvs/src/usr.bin/make/main.c,v
retrieving revision 1.123
diff -u -p -r1.123 main.c
--- main.c  22 Apr 2019 18:32:09 -  1.123
+++ main.c  5 Jan 2020 13:43:32 -
@@ -804,7 +804,7 @@ main(int argc, char **argv)
if (compatMake)
/* Compat_Init will take care of creating all the
 * targets as well as initializing the module.  */
-   Compat_Run();
+   outOfDate = Compat_Run();
else {
/* Traverse the graph, checking on all the targets.  */
outOfDate = Make_Run();
Index: make.c
===
RCS file: /cvs/src/usr.bin/make/make.c,v
retrieving revision 1.76
diff -u -p -r1.76 make.c
--- make.c  21 Dec 2019 15:31:54 -  1.76
+++ make.c  5 Jan 2020 13:43:32 -
@@ -572,7 +572,8 @@ Make_Run(Lst targs) /* the initial list
(void)MakeStartJobs();
}
 
-   problem = Job_Finish();
+   if (!queryFlag)
+   problem = Job_Finish();
 
/*
 * Print the final status of each target. E.g. if it wasn't made



MAKE: remove redundant test

2020-01-04 Thread Marc Espie
The only way errorJobs ever gets filled is by engine.c
around line 680 with a snippet that reads


if (!keepgoing) {
if (!silent)
printf("\n");
job->next = errorJobs;
errorJobs = job;
/* XXX don't free the command */
return;
}


Index: job.c
===
RCS file: /cvs/src/usr.bin/make/job.c,v
retrieving revision 1.144
diff -u -p -r1.144 job.c
--- job.c   31 Dec 2019 13:59:14 -  1.144
+++ job.c   4 Jan 2020 12:14:50 -
@@ -546,8 +547,7 @@ postprocess_job(Job *job)
} else if (job->exit_type != JOB_EXIT_OKAY && keepgoing)
free(job);
 
-   if (errorJobs != NULL && !keepgoing &&
-   aborting != ABORT_INTERRUPT)
+   if (errorJobs != NULL && aborting != ABORT_INTERRUPT)
aborting = ABORT_ERROR;
 
if (aborting == ABORT_ERROR && DEBUG(QUICKDEATH))



Re: Add -R alias to -r for scp(1)

2020-01-02 Thread Marc Espie
On Thu, Jan 02, 2020 at 11:19:30AM +0100, Ingo Schwarze wrote:
> Hi Kurt,
> 
> Kurt Mosiejczuk wrote on Wed, Jan 01, 2020 at 08:21:04PM -0500:
> 
> > cp(1) uses -R for recursive copy. scp(1) uses -r. This diff adds -R
> > as an alias for -r to scp(1) for those assuming consistency with cp(1).
> 
> even if cp -R and scp -r did the same thing - which, if i understand
> tedu@ correctly, is not the case - i would still dislike introducing
> an alias.
> 
> Aliases make documentation longer for no benefit and sometimes
> also cause confusion.  They also block namespace for no benefit
> (or rather, option space in this case).
> 
> While it is nice when different commands can use the same options
> for similar purposes, that is not always possible.  And even when
> it would have been possible when designing the tools in the past,
> if a different decision was made back then, remaining consistent
> with previous versions of the same command is usually worth more
> than symmetry with a different command.
> 
> Sometimes, aliases are needed because different other operating
> systems use different names and we want to be compatible with both.
> But whenever they can be avoided, they should.
> 
> When working on documentation, i'm actively trying to deprecate
> aliases where that is possible.
> 
> Yours,
>   Ingo

Contrary point: I'm with Kurt on that one.  Very often, I do mix up -r
and -R, and I do hate it.  Especially since our cp *does* have a -r which is
subtly different.

Not having -R in scp means you can do all kinds of fuckups.

Once in three times, I type scp -R and go "oh fuck" when it doesn't work.

And if I use scp enough, I'm also likely to use cp -r  by mistake.

Are we likely to actually remove cp -r so the second mistake 
doesn't happen ?



PATCH: fix race condition in binutils-2.17 build

2020-01-02 Thread Marc Espie
We don't normally hit this thanks to the "expensive job" heuristics
(meaning that we don't start the second target while the first is
building), but still there is a race, and if for whatever reason we
remove that heuristics we hit it fairly hard.

The patch is fairly trivial, just run both targets separately.
Example of a crash and analysis follows.

Note that the problem only occurs in binutils-2.17. The former versions
did have one less level of indirection in their Makefiles.

Index: Makefile.bsd-wrapper
===
RCS file: /cvs/src/gnu/usr.bin/binutils-2.17/Makefile.bsd-wrapper,v
retrieving revision 1.19
diff -u -p -r1.19 Makefile.bsd-wrapper
--- Makefile.bsd-wrapper21 Dec 2019 21:40:00 -  1.19
+++ Makefile.bsd-wrapper2 Jan 2020 10:13:01 -
@@ -64,6 +64,7 @@ CONFIGTARGET+=--without-gnu-as
 .endif
 
 all:   config.status
+.for t in all info
SUBDIRS='${SUBDIRS}' \
  CONFIGURE_HOST_MODULES='${CONFIGURE_HOST_MODULES}' \
  ${MAKE} ${XCFLAGS} \
@@ -74,7 +75,8 @@ all:  config.status
BSDSRCDIR=${BSDSRCDIR} \
ALL_MODULES="${ALL_MODULES}" \
ALL_HOST_MODULES='${ALL_HOST_MODULES}' \
-   INFO_HOST_MODULES='${INFO_HOST_MODULES}' all info
+   INFO_HOST_MODULES='${INFO_HOST_MODULES}' $t
+.endfor
 
 .include 
 
@@ -115,6 +117,7 @@ config.status: do-config
 
 # Need to pass SUBDIRS because of install-info
 install: maninstall
+.for t in install install-info
${INSTALL} -d -o ${BINOWN} -g ${BINGRP} \
${DESTDIR}${PREFIX}/libdata/ldscripts
SUBDIRS='${INST_SUBDIRS}' ${MAKE} DESTDIR='${DESTDIR}' \
@@ -126,7 +129,8 @@ install: maninstall
INSTALL_PROGRAM='${INSTALL} -c ${INSTALL_STRIP} -o ${BINOWN} -g 
${BINGRP} -m ${BINMODE}' \
INSTALL_DATA='${INSTALL} -c -o ${DOCOWN} -g ${DOCGRP} -m 
${NONBINMODE}' \
INSTALL_INFO_HOST_MODULES='${INSTALL_INFO_HOST_MODULES}' \
- install install-info
+ $t
+.endfor
 .if ${LINKER_VERSION:L} == "bfd"
rm -f ${DESTDIR}${PREFIX}/bin/ld
ln ${DESTDIR}${PREFIX}/bin/ld.bfd ${DESTDIR}${PREFIX}/bin/ld


Tail of a crashing build:
checking for LC_MESSAGES... (cached) yes
checking for LC_MESSAGES... (cached) yes
checking whether NLS is requested... no
checking whether NLS is requested... no
updating cache ./config.cache
updating cache ./config.cache
creating ./config.status
creating ./config.status
mv: rename conftest.tail to conftest.vals: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
creating Makefile
creating Makefile
./config.status[812]: syntax error: `fi' unexpected
./config.status[812]: syntax error: `fi' unexpected
*** Error 1 in gnu/usr.bin/binutils-2.17/obj (Makefile:5406 'configure-intl': 
@r=`${PWDCMD-pwd}`; export r;  s=`cd /usr/src/gnu/usr.bin/binu...)
*** Error 1 in gnu/usr.bin/binutils-2.17/obj (Makefile:5406 'configure-intl': 
@r=`${PWDCMD-pwd}`; export r;  s=`cd /usr/src/gnu/usr.bin/binu...)
*** Error 2 in target 'all'
*** Error 2 in gnu/usr.bin/binutils-2.17/obj (Makefile:747 'do-info': 
@r=`${PWDCMD-pwd}`; export r;  s=`cd /usr/src/gnu/usr.bin/binutils-2.1...)
*** Error 2 (Makefile:634 'all': @r=`${PWDCMD-pwd}`; export r;  s=`cd 
/usr/src/gnu/usr.bin/binutils-2.17; ${PWDCMD-pwd}`; export s;  if [ -f...)
*** Error 2 in gnu/usr.bin/binutils-2.17 (Makefile.bsd-wrapper:77 'all')

You'll notice the duplicate lines. It's actually libintl running configure
twice at the same time.  Initially, I thought "oh let's configure libintl
up-front". Unfortunately this is not enough, and leads to a different crash.


Now, for the generated Makefile. In binutils prior to 2.17, both all and
info go straight info building, so they're not affected.

binutils-2.17 has the following snippets:

all:
@: $(MAKE); $(unstage)
@r=`${PWD_COMMAND}`; export r; \
s=`cd $(srcdir); ${PWD_COMMAND}`; export s; \
if [ -f stage_last ]; then \
  $(MAKE) $(TARGET_FLAGS_TO_PASS) all-host all-target; \
else \
  $(MAKE) $(RECURSE_FLAGS_TO_PASS) all-host all-target; \
fi

(the : $(MAKE) is the fun way automake/autoconf disables maintenance lines)

and

info: do-info
do-info:
@: $(MAKE); $(unstage)
@r=`${PWD_COMMAND}`; export r; \
s=`cd $(srcdir); ${PWD_COMMAND}`; export s; \
$(MAKE) $(RECURSE_FLAGS_TO_PASS) info-host \
  info-target

so, if they're run at the same time, they will *both* recurse and run
a separate make... 

looking a bit further, 

MAKE: plug memory leak

2019-12-31 Thread Marc Espie
If we look at job_handle_status, we see there's one case where it
doesn't place the job on the errorJob list, but doesn't try to reclaim
the memory.

Index: job.c
===
RCS file: /cvs/src/usr.bin/make/job.c,v
retrieving revision 1.143
diff -u -p -r1.143 job.c
--- job.c   30 Dec 2019 11:01:16 -  1.143
+++ job.c   31 Dec 2019 11:46:57 -
@@ -543,7 +544,8 @@ postprocess_job(Job *job)
job->node->built_status = REBUILT;
Make_Update(job->node);
free(job);
-   }
+   } else if (job->exit_type != JOB_EXIT_OKAY && keepgoing)
+   free(job);
 
if (errorJobs != NULL && !keepgoing &&
aborting != ABORT_INTERRUPT)



MAKE: have the "expensive job" logic perform better

2019-12-31 Thread Marc Espie
So, a job is the series of shell commands run by make

a:
cmd1
expensive_cmd2
cmd3

so the granularity of expensive should be at the command level,
not the job level.

So, instead of running the code that may start new jobs at
the end of remove_job, let's run it after each change of state.

This should let parallel make perform slightly better, by firing
up jobs sooner in case of several targets with multiple commands.

Index: job.c
===
RCS file: /cvs/src/usr.bin/make/job.c,v
retrieving revision 1.143
diff -u -p -r1.143 job.c
--- job.c   30 Dec 2019 11:01:16 -  1.143
+++ job.c   31 Dec 2019 10:00:39 -
@@ -148,6 +148,7 @@ static void may_continue_job(Job *);
 static void continue_job(Job *);
 static Job *reap_finished_job(pid_t);
 static bool reap_jobs(void);
+static void may_continue_heldback_jobs();
 
 static void loop_handle_running_jobs(void);
 static bool expensive_job(Job *);
@@ -746,9 +747,14 @@ remove_job(Job *job)
 {
nJobs--;
postprocess_job(job);
+}
+
+static void
+may_continue_heldback_jobs()
+{
while (!no_new_jobs) {
if (heldJobs != NULL) {
-   job = heldJobs;
+   Job *job = heldJobs;
heldJobs = heldJobs->next;
if (DEBUG(EXPENSIVE))
fprintf(stderr, "[%ld] cheap -> release %s\n",
@@ -803,6 +809,7 @@ reap_jobs(void)
job_handle_status(job, status);
determine_job_next_step(job);
}
+   may_continue_heldback_jobs();
}
/* sanity check, should not happen */
if (pid == -1 && errno == ECHILD && runningJobs != NULL)



Re: cdio(1): remove CDDB support

2019-12-29 Thread Marc Espie
On Sat, Dec 28, 2019 at 07:48:45PM -0500, Bryan Steele wrote:
> With FreeDB announcing[0] that the service will be shutting down as of
> March 31st of 2020, and the only other alternative (MusicBrainz) already
> having shutdown their freedb/cddb gateway in favour of their own API
> early this year, it likely makes sense to remove support from cdio(1).
> 
> CDDB is used to retrieve music CD metadata over the Internet, e.g:
> Artist and Track names.
> 
> I left in support for the "cdid" command as it may be useful for
> archival(?) purposes, if not that can go too.
> 
> Cc: espie@ as he wrote this code, & maybe he still has music on CDs.

How about we wait until the service actually shuts down ?

Who knows, maybe someone else will pick up ?

It's fairly sad, as this means you would have (again) to annotate mp3s
you make from your own CDs by hand.



Re: [PATCH] add support for versions with '-' before a/b/rc

2019-12-13 Thread Marc Espie
On Fri, Dec 13, 2019 at 02:19:26PM +0100, Matija Skala wrote:
> Dne petek, 13. december 2019 ob 13:34:57 CET je Marc Espie napisal(a):
> > On Fri, Dec 13, 2019 at 08:45:08AM +0100, Jasper Lievisse Adriaanse wrote:
> > > Hello Matija,
> > >
> > > Could you please provide a testcase for inclusion in
> > > src/regress/usr.bin/pkg-config too? Also, is there a particular port or
> > > pkg-config file in the wild that you ran into which exhibits this
> > > particular pattern?
> > I'm going to ask more strongly: is this something that mainstream pkg-config
> > does ?
> pkgconf does do this. Not sure about pkg-config
> >
> > Also, pedantically, the change is wrong.  You should always quote non-alpha
> > characters when they stand for themselves. Yes, that includes \-
> >
> Right, here a new patch
> diff --git a/regress/usr.bin/pkg-config/pcdir/alpha2.pc b/regress/usr.bin/pkg-
> config/pcdir/alpha2.pc
> new file mode 100644
> index 0..7958d3717
> --- /dev/null
> +++ b/regress/usr.bin/pkg-config/pcdir/alpha2.pc
> @@ -0,0 +1,4 @@
> +Name: alpha suffix test
> +Description: pkg-config(1) regress file
> +Version: 0.1.0-alpha2
> +Libs: -lalpha2
> diff --git a/regress/usr.bin/pkg-config/pcdir/beta2.pc b/regress/usr.bin/pkg-
> config/pcdir/beta2.pc
> new file mode 100644
> index 0..cc1959848
> --- /dev/null
> +++ b/regress/usr.bin/pkg-config/pcdir/beta2.pc
> @@ -0,0 +1,3 @@
> +Name: beta suffix test
> +Description: pkg-config(1) regress file
> +Version: 0.1.0-beta2
> diff --git a/regress/usr.bin/pkg-config/pcdir/rc2.pc b/regress/usr.bin/pkg-
> config/pcdir/rc2.pc
> new file mode 100644
> index 0..499242745
> --- /dev/null
> +++ b/regress/usr.bin/pkg-config/pcdir/rc2.pc
> @@ -0,0 +1,3 @@
> +Name: rc suffix test
> +Description: pkg-config(1) regress file
> +Version: 0.1.0-rc2
> diff --git a/usr.bin/pkg-config/pkg-config b/usr.bin/pkg-config/pkg-config
> index c1aab9576..1102696af 100644
> --- a/usr.bin/pkg-config/pkg-config
> +++ b/usr.bin/pkg-config/pkg-config
> @@ -674,13 +674,13 @@ sub compare
>   # is there a valid non-numeric suffix to deal with later?
>   # accepted are (in order): a(lpha) < b(eta) < rc < ' '.
>   # suffix[0] is the 'alpha' part, suffix[1] is the '1' part in 'alpha1'.
> - if ($a =~ s/(rc|beta|b|alpha|a)(\d+)$//) {
> + if ($a =~ s/\-?(rc|beta|b|alpha|a)(\d+)$//) {
>   say_debug("valid suffix $1$2 found in $a$1$2.");
>   $suffix_a[0] = $1;
>   $suffix_a[1] = $2;
>   }
> 
> - if ($b =~ s/(rc|beta|b|alpha|a)(\d+)$//) {
> + if ($b =~ s/\-?(rc|beta|b|alpha|a)(\d+)$//) {
>   say_debug("valid suffix $1$2 found in $b$1$2.");
>   $suffix_b[0] = $1;
>   $suffix_b[1] = $2;
> 
> 
> 
> 
Fair enough. I'm okay with it then.



Re: [PATCH] add support for versions with '-' before a/b/rc

2019-12-13 Thread Marc Espie
On Fri, Dec 13, 2019 at 08:45:08AM +0100, Jasper Lievisse Adriaanse wrote:
> Hello Matija,
> 
> Could you please provide a testcase for inclusion in 
> src/regress/usr.bin/pkg-config too?
> Also, is there a particular port or pkg-config file in the wild that you ran 
> into which exhibits this particular pattern?

I'm going to ask more strongly: is this something that mainstream pkg-config
does ?

Also, pedantically, the change is wrong.  You should always quote non-alpha
characters when they stand for themselves. Yes, that includes \-

> > On 12 Dec 2019, at 18:28, Matija Skala  wrote:
> > 
> > From fa66eb42d0bd2fec7b364644684e6a4cc9ae9baa Mon Sep 17 00:00:00 2001
> > From: Matija Skala 
> > Date: Thu, 28 Nov 2019 19:24:42 +0100
> > Subject: [PATCH] add support for versions with '-' before a/b/rc
> > 
> > ---
> > usr.bin/pkg-config/pkg-config | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/usr.bin/pkg-config/pkg-config b/usr.bin/pkg-config/pkg-config
> > index 6dfbd3224eb..c050e9b058e 100644
> > --- a/usr.bin/pkg-config/pkg-config
> > +++ b/usr.bin/pkg-config/pkg-config
> > @@ -674,13 +674,13 @@ sub compare
> > # is there a valid non-numeric suffix to deal with later?
> > # accepted are (in order): a(lpha) < b(eta) < rc < ' '.
> > # suffix[0] is the 'alpha' part, suffix[1] is the '1' part in 'alpha1'.
> > -   if ($a =~ s/(rc|beta|b|alpha|a)(\d+)$//) {
> > +   if ($a =~ s/-?(rc|beta|b|alpha|a)(\d+)$//) {
> > say_debug("valid suffix $1$2 found in $a$1$2.");
> > $suffix_a[0] = $1;
> > $suffix_a[1] = $2;
> > }
> > 
> > -   if ($b =~ s/(rc|beta|b|alpha|a)(\d+)$//) {
> > +   if ($b =~ s/-?(rc|beta|b|alpha|a)(\d+)$//) {
> > say_debug("valid suffix $1$2 found in $b$1$2.");
> > $suffix_b[0] = $1;
> > $suffix_b[1] = $2;
> > 
> > 
> 
> 



Re: patch: make sdiff tty-aware

2019-12-12 Thread Marc Espie
On Thu, Dec 12, 2019 at 02:51:18AM -0700, Theo de Raadt wrote:
> Marc Espie  wrote:
> 
> > On Wed, Dec 11, 2019 at 01:15:40PM -0700, Theo de Raadt wrote:
> > > I'm not sure I understand the goal of the signal handler.
> > > 
> > > sdiff is moving forward through the file, only.  If you are in a pager,
> > > you want to increase the width for the later output not yet visible?
> > > the normal way one does that in programs which don't backtrack and
> > > re-output, is by restarting the program.
> > > 
> > > You don't want a mix of shorter, then longer.  If you are using a pager,
> > > I suspect at least one of the next lines is already be buffered out.  So
> > > I don't understand how this will create output which looks correct.
> > > 
> > > I think you should remove the signal handler.
> > 
> > Well, in the sysmerge use-case, you see the first line showing the
> > $OpenBSD$ prompt usually. That's often when you realize your terminal is
> > really small, and you will miss the next lines. The $OpenBSD$ line is
> > reasonably unambiguous, as the rev number is fairly prominent.
> > 
> > The next ones will need the space.
> > 
> > Admittedly, it could be smarter, and react during actual user interaction,
> > or offer to redraw the last block with a specific command ?
> 
> That you have to dig for such a narrow example really demonstrates
> the pointlessness of the signal handler.

I didn't have to dig... it's the specific usecase I have for sdiff3!



Re: patch: make sdiff tty-aware

2019-12-12 Thread Marc Espie
On Wed, Dec 11, 2019 at 01:15:40PM -0700, Theo de Raadt wrote:
> I'm not sure I understand the goal of the signal handler.
> 
> sdiff is moving forward through the file, only.  If you are in a pager,
> you want to increase the width for the later output not yet visible?
> the normal way one does that in programs which don't backtrack and
> re-output, is by restarting the program.
> 
> You don't want a mix of shorter, then longer.  If you are using a pager,
> I suspect at least one of the next lines is already be buffered out.  So
> I don't understand how this will create output which looks correct.
> 
> I think you should remove the signal handler.

Well, in the sysmerge use-case, you see the first line showing the
$OpenBSD$ prompt usually. That's often when you realize your terminal is
really small, and you will miss the next lines. The $OpenBSD$ line is
reasonably unambiguous, as the rev number is fairly prominent.

The next ones will need the space.

Admittedly, it could be smarter, and react during actual user interaction,
or offer to redraw the last block with a specific command ?



patch: make sdiff tty-aware

2019-12-11 Thread Marc Espie
This patch allows sdiff to auto-detect tty width,
by passing -w auto

More importantly, sdiff will adjust its variables on the fly for
subsequent lines (I don't know if redrawing the current
lines when the tty changes is advisable).

It's pretty straightforward, tested thru sysmerge.

Doesn't seem it's standardized anywhere, I'm not sure having a functionality 
that's
not in gnu-sdiff would be an issue ?


Index: sdiff.c
===
RCS file: /cvs/src/usr.bin/sdiff/sdiff.c,v
retrieving revision 1.37
diff -u -p -r1.37 sdiff.c
--- sdiff.c 28 Sep 2018 18:21:52 -  1.37
+++ sdiff.c 11 Dec 2019 16:42:25 -
@@ -17,12 +17,14 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 
 
 #include "common.h"
 #include "extern.h"
@@ -43,7 +45,9 @@ struct diffline {
 };
 
 static void astrcat(char **, const char *);
+static void compute_width(void);
 static void enqueue(char *, char, char *);
+static void find_width(void);
 static char *mktmpcpy(const char *);
 static void freediff(struct diffline *);
 static void int_usage(void);
@@ -55,12 +59,16 @@ static void printd(FILE *, size_t);
 static void println(const char *, const char, const char *);
 static void processq(void);
 static void prompt(const char *, const char *);
+static void recompute_width(void);
 __dead static void usage(void);
+static void window_changed(int);
 static char *xfgets(FILE *);
 
 SIMPLEQ_HEAD(, diffline) diffhead = SIMPLEQ_HEAD_INITIALIZER(diffhead);
 size_t  line_width;/* width of a line (two columns and divider) */
 size_t  width; /* width of each column */
+size_t  wflag = WIDTH;
+volatile sig_atomic_t got_signal = 0;
 size_t  file1ln, file2ln;  /* line number of file1 and file2 */
 int Iflag = 0; /* ignore sets matching regexp */
 int lflag; /* print only left column for identical lines */
@@ -153,11 +161,47 @@ FAIL:
exit(2);
 }
 
+static void
+window_changed(int sig __attribute__((__unused__)))
+{
+   got_signal = 1;
+}
+
+static void
+find_width(void)
+{
+   struct winsize ws;
+
+   if (ioctl(0, TIOCGWINSZ, ) != 0) {
+   wflag = WIDTH;
+   signal(SIGWINCH, SIG_DFL);
+   } else
+   wflag = ws.ws_col;
+}
+
+static void
+recompute_width(void)
+{
+   find_width();
+   compute_width();
+}
+
+static void
+compute_width(void)
+{
+   /* Subtract column divider and divide by two. */
+   width = (wflag - 3) / 2;
+   /* Make sure line_width can fit in size_t. */
+   if (width > (SIZE_MAX - 3) / 2)
+   errx(2, "width is too large: %zu", width);
+   line_width = width * 2 + 3;
+}
+
 int
 main(int argc, char **argv)
 {
FILE *diffpipe, *file1, *file2;
-   size_t diffargc = 0, wflag = WIDTH;
+   size_t diffargc = 0;
int ch, fd[2], status;
pid_t pid;
const char *outfile = NULL;
@@ -236,6 +280,11 @@ main(int argc, char **argv)
diffargv[diffargc++] = "-w";
break;
case 'w':
+   if (strcmp(optarg, "auto") == 0) {
+   signal(SIGWINCH, window_changed);
+   find_width();
+   break;
+   }
wflag = strtonum(optarg, WIDTH_MIN,
INT_MAX, );
if (errstr)
@@ -273,7 +322,7 @@ main(int argc, char **argv)
if (unveil(_PATH_BSHELL, "x") == -1)
err(2, "unveil");
}
-   if (pledge("stdio rpath wpath cpath proc exec", NULL) == -1)
+   if (pledge("stdio rpath wpath cpath proc exec tty", NULL) == -1)
err(2, "pledge");
 
/*
@@ -302,12 +351,7 @@ main(int argc, char **argv)
/* Add NULL to end of array to indicate end of array. */
diffargv[diffargc++] = NULL;
 
-   /* Subtract column divider and divide by two. */
-   width = (wflag - 3) / 2;
-   /* Make sure line_width can fit in size_t. */
-   if (width > (SIZE_MAX - 3) / 2)
-   errx(2, "width is too large: %zu", width);
-   line_width = width * 2 + 3;
+   compute_width();
 
if (pipe(fd))
err(2, "pipe");
@@ -522,6 +566,11 @@ static void
 println(const char *s1, const char div, const char *s2)
 {
size_t col;
+
+   if (got_signal) {
+   got_signal = 0;
+   recompute_width();
+   }
 
/* Print first column.  Skips if s1 == NULL. */
col = 0;



Re: grep -R with no path

2019-12-02 Thread Marc Espie
On Mon, Dec 02, 2019 at 08:31:02AM +, Miod Vallat wrote:
> grep(1), when invoked with the -R option but no path, displays a
> "recursive search of stdin" warning and acts as if -R had not been
> specified.
> 
> GNU grep, in that case, will perform a recursive search in the current
> directory, i.e. uses an implicit "." path if none is given.
> 
> This is IMO a much better behaviour. What about the following diff?
> 
> Index: grep.c
> ===
> RCS file: /OpenBSD/src/usr.bin/grep/grep.c,v
> retrieving revision 1.62
> diff -u -p -r1.62 grep.c
> --- grep.c7 Oct 2019 20:04:00 -   1.62
> +++ grep.c2 Dec 2019 08:27:09 -
> @@ -473,8 +473,12 @@ main(int argc, char *argv[])
>   ++argv;
>   }
>  
> - if (Rflag && argc == 0)
> - warnx("warning: recursive search of stdin");
> + if (Rflag && argc == 0) {
> + /* default to . if no path given */
> + static char *dot_argv[] = { ".", NULL };
> + argv = dot_argv;
> + argc = 1;
> + }
>   if (Eflag)
>   cflags |= REG_EXTENDED;
>   if (Fflag)
> 
> 
I concur, I like it better as well.
-R isn't in POSIX anyway, so we don't have to worry about standard.



change to bsd.port.mk to help debug packages a bit

2019-11-27 Thread Marc Espie
This should get rid of the weird error (don't know how to make
.../all/debug-*.tgz) when switching from !DEBUG_PACKAGES to DEBUG_PACKAGES.

Basically, this introduces a "build two targets at once" in bsd.port.mk.

make has some glue to figure out whether this is a "duplicate the work" or
"one target builds two things" (already used for yacc and friends), by
looking for variations on $@... which includes $@D for better or worse, hence
the small tweak.

This should work on all settings, including running make -jN package
(which doesn't make a lot of sense, but whatever).

Please check/test

Index: bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1505
diff -u -p -r1.1505 bsd.port.mk
--- bsd.port.mk 26 Nov 2019 11:49:02 -  1.1505
+++ bsd.port.mk 27 Nov 2019 16:49:05 -
@@ -1183,6 +1183,7 @@ _EXCLUDE_DEBUG_PLISTS = ${_WRKDEBUG} ${_
 PKG_ARGS${_S} += -A'${PKG_ARCH${_S}}'
 
 _pkg${_S} = ${_PKGFILE${_S}}
+_pkg_cookie${_S} = ${_PACKAGE_COOKIE${_S}}
 
 .  if ${DEBUG_PACKAGES:M${_S}}
 _DBG_PKG_ARGS${_S} := ${PKG_ARGS${_S}}
@@ -1194,6 +1195,7 @@ _DBG_PKG_ARGS${_S} += -DFULLPKGPATH=debu
 _DBG_PKG_ARGS${_S} += -f ${_WRKDEBUG}/${PLIST${_S}:T}
 _EXCLUDE_DEBUG_PLISTS += ${_WRKDEBUG}/${PLIST${_S}:T}
 _pkg${_S} += ${_DBG_PKGFILE${_S}}
+_pkg_cookie${_S} += ${_DBG_PACKAGE_COOKIE${_S}}
 .  endif
 
 # Finish filling out package command, and package dependencies
@@ -2073,8 +2075,8 @@ ${_CACHE_REPO}${_p}:
 
 # The real package
 
-${_PACKAGE_COOKIE${_S}}:
-   @${_PBUILD} install -d ${PACKAGE_REPOSITORY_MODE} ${@D} ${_TMP_REPO}
+${_pkg_cookie${_S}}:
+   @${_PBUILD} install -d ${PACKAGE_REPOSITORY_MODE} ${_PKG_REPO${_S}} 
${_TMP_REPO}
 .  if ${FETCH_PACKAGES:L} != "no" && 
!defined(_TRIED_FETCHING_${_PACKAGE_COOKIE${_S}})
@${_INSTALL_CACHE_REPO} ${_CACHE_REPO}
@cd ${.CURDIR}; gotit=true; for p in ${_pkg${_S}}; do \



Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior

2019-11-27 Thread Marc Espie
On Wed, Nov 27, 2019 at 06:21:01PM +0200, Lauri Tirkkonen wrote:
> All that said I do understand if there is reluctance to merge the
> jobserver stuff since it doesn't actually help the current situation in
> most cases. Nevertheless it has been personally beneficial to me in
> identifying areas of improvement, even if those are nothing new to
> OpenBSD developers.

Oh, I'm not opposed to merging a jobserver once it's roughly as stable
as what we have.

My concerns are:
- it should work with all of src and xenocara (once that's done, 
I can do ports)
- it should not interfere with other jobservers in other makes.
- it does not replace the expensive jobs heuristics.
- it does not render make harder to maintain and evolve.

Especially since, as I've said before, there's no actual guarantee anywhere
in POSIX that shells will cooperate with it.

I don't have enough time to do everything I want, but it's well known that
there are still issues with make -j compared to sequential make.

If you look at cvs history, I've completely rewritten job.c a few times.
And I had a presentation at eurobsdcon about what's still missing a few
years ago. I've been swamped with changes in dpb/bsd.port.mk/pkg_add...
recently, but it's still something I want to get back to eventually !

One reason that work goes slowly is because make was a seriously entwined
series of things that all depend on each other... It still is, but slightly
less so, and thus anything that's not squeaky clean is unlikely to get in.

(hence me wanting the jobserver stuff to be reasonably self-contained
and not interfering with the rest of make)



Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior

2019-11-27 Thread Marc Espie
On Wed, Nov 27, 2019 at 05:31:48PM +0200, Lauri Tirkkonen wrote:
> I'll give you some more background that I maybe should have given
> earlier already: in my hobby OS Unleashed, we use bmake and earlier did
> some (slightly hacky) modifications to subdir.mk to enable paralellizing
> jobs in multiple subdirs at the same time. I don't remember the exact
> numbers since it's been quite some time, but I was able to cut the base
> build time by *a lot* with just that change -- during the build there
> were previously a lot of idle CPUs. Those results served as motivation
> for me to start looking into doing similar things in OpenBSD, and having
> a jobserver is sort of the first step there.

I did experiment with something similar a while back:

reorganizing a large part of usr.bin or usr.sbin to just be one
single variation of bsd.prog.mk with multiple progs and multiple object
files... works just fine for, say 95% of the binaries in those directories

(considering there are lots of directories with one single C file or two
C files, this sounds like a gain for make -jN, right ?)

End result: no measurable gain.   That on 4 cpu boxes at the time. 
There are SMP issues to solve before this actually yields any 
useful result...



Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior

2019-11-27 Thread Marc Espie
On Fri, Nov 15, 2019 at 03:29:29PM +0200, Lauri Tirkkonen wrote:
> On Fri, Nov 15 2019 15:24:57 +0200, Lauri Tirkkonen wrote:
> > Your points are valid and I agree with them completely. There are
> > clearly problems with lock contention,
> 
> and I should mention here that I would probably not have observed the
> magnitude of these problems had I not made make start more jobs at once
> in the first place. So while the make/mk diffs need more work, we've
> already identified more areas of improvement as a result of trying them
> out. But one thing at a time. :)

Seriously, as much as what you're doing is interesting, you've not identified
anything new in THAT specific area.  We've run enough build configurations 
to know about those scalability issues for years.

It's cool that you found new races that are hidden by expensive jobs, though,
that much I fully agree with.



Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior

2019-11-27 Thread Marc Espie
On Wed, Nov 27, 2019 at 12:42:48PM +0200, Lauri Tirkkonen wrote:
> In a lesson to always proof-read *before* sending your message:
> 
> On Wed, Nov 27 2019 12:31:51 +0200, Lauri Tirkkonen wrote:
> >  - a full release build with the jobserver enabled passed after
> >disabling gnu/usr.bin/binutils
> 
> this should, in fact, read "gnu/usr.bin/binutils-2.17". binutils builds
> fine.

We would need to investigate this.

Instead of disabling them, send logs



Re: debug packages: let strip do the stripping

2019-11-25 Thread Marc Espie
On Mon, Nov 25, 2019 at 04:11:53PM +0200, Paul Irofti wrote:
> Hi,
> 
> Few people complained (hi landry@!) that stripped binaries are slightly
> larger now than they used to be when debug packages are enabled.
> 
> My investigations show that this is because objcopy --strip-debug is
> less efficient than plain strip(1) which is what we use for non-debug
> packages.
> 
> Reintroducing strip(1) does not affect current debug packages behaviour
> in my experience. The link to the debug symbols is still there and
> egdb(1) still loads it automatically and displays all the debug info.
> 
> OK?
> 
> Paul
> 
> Index: bin/build-debug-info
> ===
> RCS file: /cvs/ports/infrastructure/bin/build-debug-info,v
> retrieving revision 1.22
> diff -u -p -u -p -r1.22 build-debug-info
> --- bin/build-debug-info  19 Nov 2019 15:49:30 -  1.22
> +++ bin/build-debug-info  25 Nov 2019 14:06:34 -
> @@ -263,7 +263,7 @@ print {$self->{mk}} << 'EOPREAMBLE';
>  OBJCOPY_RULE = ${INSTALL_DATA_DIR} ${@D} && \
>  echo "> Copy debug info from $? to $@" && \
>  objcopy --only-keep-debug $? $@ && \
> -objcopy --strip-debug $? && \
> +strip $? && \
>  objcopy --add-gnu-debuglink=$@ $? && \
>  touch $@
>  
> 
Okay



Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior

2019-11-14 Thread Marc Espie
On Thu, Nov 14, 2019 at 05:30:34PM +0200, Lauri Tirkkonen wrote:
> > > @@ -347,10 +360,9 @@ print_errors(void)
> > >  static void
> > >  setup_signal(int sig)
> > >  {
> > > - if (signal(sig, SIG_IGN) != SIG_IGN) {
> > > - (void)signal(sig, notice_signal);
> > > - sigaddset(, sig);
> > > - }
> > > + sigaction(sig, &(struct sigaction) { .sa_handler = notice_signal },
> > > + NULL);
> > > + sigaddset(, sig);
> > >  }
> > Why did you change this ? this is gratuitous.
> 
> Actually, it's not gratuitous, but it's subtle. signal() enables
> SA_RESTART, but we depend on EINTR returns in a couple places, so I did
> this. I should've mentioned this originally, sorry.

Err, thinking some more. You can't do that. This affects ALL of make's code
and not just this file.  Some parts are belt-and-suspenders type and include
checks for EINTR, but NOWHERE near every part.

In particular, you *lose* stdio.h entirely.



Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior

2019-11-14 Thread Marc Espie
On Thu, Nov 14, 2019 at 05:30:34PM +0200, Lauri Tirkkonen wrote:
> Yeah, you're right, it reads like crap, I'll rewrite it. The gist of the
> thing is that 'expensive' heuristic is now only used to decide whether
> or not to print the failed command.

I object to that. Keep the "expensive" heuristics around. 
The current -jn should still work *without* the jobserver mechanism.

> > >  static void
> > > -loop_handle_running_jobs()
> > > +loop_handle_running_jobs(void)
> > >  {
> > >   while (runningJobs != NULL)
> > >   handle_running_jobs();
> > >  }
> > Yet another gratuitous change.  There is already a perfectly fine prototype
> > that asserts (void).
> 
> A function prototype with empty parentheses does not assert (void) in C
> (but does in C++). However, you're correct about it not being related to
> this change.
See above. There are prototypes for ALL static functions in that file.
You don't need the void in the function definition when the function
declaration (a few hundred lines above) already has it.

> This was also a bit concerning to me, but that's what bmake/gmake also
> are doing (albeit with more fd's). Eventually the potential gains
> outweighed my concerns: every SUBDIR in bsd.subdir.mk is currently built
> serially, which has quite a large impact on how many jobs are running at
> any one time.
> 
> An alternative could be to use unix sockets with paths instead and pass
> the path through the environment, but I'm not convinced I like that any
> better.
There is the huge problem of where you put your paths.  NFS comes in the
way AND socket names are limited to 256 characters.



Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior

2019-11-14 Thread Marc Espie
On Tue, Nov 05, 2019 at 03:22:08PM +, Lauri Tirkkonen wrote:
> Hi,
> 
> make has the concept of 'expensive' jobs: if the command line includes
> the word "make" in it, the command is considered expensive and other
> jobs are held until that job finishes. It does this to avoid exponential
> behavior when parallelizing with -j.
> 
> Some other makes (eg. bmake and gmake) use a jobserver, ie. a pair of
> pipes between child makes and the top-level make: the top-level make
> provides job tokens to children via these pipes, which are returned when
> the jobs complete. This ensures that recursive makes only run a certain
> number of jobs in total at any one time, while achieving greater
> resource utilisation (no need to wait for an 'expensive' job to finish
> before starting others).
> 
> This diff implements a similar jobserver in make. Like bmake and gmake,
> job tokens are provided by the top-level make (called "jobserver
> master"), used by children ("jobserver slaves") when they run commands,
> and returned when those commands finish. Like bmake, the internal,
> undocumented command line option to tell (possibly indirect, via
> MAKEFLAGS)) children about the file descriptor to use is -J, but unlike
> bmake and gmake, we use socketpair(AF_UNIX, SOCK_STREAM) instead of
> pipes, and therefore only need to pass one file descriptor to children
> instead of two.

Sorry, I missed your initial message (thanks to naddy@ for telling me about it)

A jobserver was considered in make before implementing expensive.

The main reason it was not done that way is that you can't guarantee a
file descriptor will make it through the intermediate shell.  

In fact, POSIX doesn't guarantee anything, and a lot of shells actively close
file descriptors they don't know about.

Your example is not complicated enough, as your commands mean NO 
shell will be spawned (everything will pass directly through to 
submakes, which is BTW a large reason for implementing -C, which cuts 
down on the number of intermediate processes).

I'm not sure I like added complexity in that part of make.

> I haven't yet built the entire tree with this, just cautiously probing
> the waters first to see if there is interest :)

I'd like some actual numbers on real world applications: what does it change
for the actual build through the full source tree.   Areas like perl are where
the job explosion occurs and where the expensive heuristics got refined.


> Tested with the following Makefile structure:
> 
>   $ cat recursive/Makefile
>   .PHONY: a b c d
> 
>   all: a b c d
> 
>   a:
>   make -C a
>   b:
>   make -C a
>   c:
>   make -C a
>   d:
>   make -C a
>   $ cat recursive/a/Makefile
>   all: 1 2 3 4
> 
>   1:
>   sleep 1
>   2:
>   sleep 2
>   3:
>   sleep 3
>   4:
>   sleep 4
> 
> the speedup is as follows with -j4:
> 
> before:
>   make -C recursive -j4  0.01s user 0.10s system 0% cpu 16.097 total
> after:
>   make -C recursive -j4  0.02s user 0.15s system 1% cpu 11.082 total

Comments on the patch proper:

> ---
>  usr.bin/make/job.c  | 317 +++-
>  usr.bin/make/job.h  |   7 +-
>  usr.bin/make/main.c |  31 -
>  usr.bin/make/main.h |   3 +
>  4 files changed, 290 insertions(+), 68 deletions(-)
> 
> diff --git a/usr.bin/make/job.c b/usr.bin/make/job.c
> index ebbae5306da..b36aaeacac8 100644
> --- a/usr.bin/make/job.c
> +++ b/usr.bin/make/job.c
> @@ -90,11 +90,15 @@
>   *   Job_WaitWait for all running jobs to finish.
>   */
>  
> +#include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -115,6 +119,7 @@
>  #include "memory.h"
>  #include "make.h"
>  #include "buf.h"
> +#include "main.h"
>  
>  static int   aborting = 0;   /* why is the make aborting? */
>  #define ABORT_ERROR  1   /* Because of an error */
> @@ -123,12 +128,15 @@ static int  aborting = 0;   /* why is the make 
> aborting? */
>  
>  static int   maxJobs;/* The most children we can run at once */
>  static int   nJobs;  /* Number of jobs already allocated */
> -static bool  no_new_jobs;/* Mark recursive shit so we shouldn't start
> -  * something else at the same time
> -  */
I don't think ditching expensive entirely is a good idea anyhow.
What happens if the jobserver fails ?  I see it goes to err all the time.
BTW, going to err is bad. Make handles processes. Most errors MUST do 
something sane, like waiting for processes to end in many many cases.

> +
> +struct jobserver {
> + int master; /* master deposits job tokens into this fd */
> + int slave;  /* slaves consume job tokens from this fd */
> + volatile sig_atomic_t tokens;   /* 

Re: _pbuild user to have priority=5

2019-11-06 Thread Marc Espie
On Sat, Nov 02, 2019 at 02:35:28PM +0100, Solene Rapenne wrote:
> On Sat, Nov 02, 2019 at 01:18:53PM +, Stuart Henderson wrote:
> > On 2019/11/01 19:16, Theo de Raadt wrote:
> > > Ted Unangst  wrote:
> > > 
> > > > Theo de Raadt wrote:
> > > > > What about all the other users who aren't in staff?
> > > > > 
> > > > > I think the approach is right.  Push non-interactive down.
> > > > 
> > > > The same then for src build user?
> > > 
> > > Well, that's different.  Most of us building the src tree are waiting
> > > eagerly for it to finish aren't we?
> > 
> > That's the same for ports building!
> > 
> 
> if you don't do anything else than compiling ports, that shouldn't be
> slower.
> If you are doing something else (GUI user, web server, community server
> with people connected doing IRC) , then you don't get angry due to
> unresponsive system.
> 
> Lowering staff priority would only help the one user case.

I agree with solene on that one.

This is actually useful even if you're just building ports, because
you get a more responsive text-editor and stuff like that which is useful
when you're fixing things that broke while dpb is still going.

I see a noticeable difference in vim showing me syntax coloring correctly
while dpb is running.

Source is somewhat different. make build/release is sequential by nature,
as you can't really fix a part while something else is still building.



Re: assert() missing in flex

2019-09-16 Thread Marc Espie
On Mon, Sep 16, 2019 at 05:07:21PM +0800, Michael Mikonos wrote:
> Hello,
> 
> When building flex on clang 8.0.1 (i386) I noticed that assert()
> expands to nothing. This happens because of a fallback declaration
> of assert() in flexdef.h when HAVE_ASSERT_H is not set. Instead of
> changing flexdef.h the following patch follows the existing pattern
> in Makefile of adding -DHAVE_SOMETHING. Does this look OK?
> 
> - Michael
> 
> 
> Index: Makefile
> ===
> RCS file: /cvs/src/usr.bin/lex/Makefile,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 Makefile
> --- Makefile  30 Apr 2017 20:30:39 -  1.17
> +++ Makefile  16 Sep 2019 08:55:42 -
> @@ -12,7 +12,7 @@
>  # To bootstrap lex, cp initscan.c to scan.c and run make.
>  
>  PROG=   lex
> -CFLAGS+=-I. -I${.CURDIR} -DHAVE_CONFIG_H
> +CFLAGS+=-I. -I${.CURDIR} -DHAVE_CONFIG_H -DHAVE_ASSERT_H
>  SRCS= buf.c ccl.c dfa.c ecs.c filter.c gen.c main.c misc.c \
> nfa.c options.c parse.y regex.c scan.l scanflags.c \
> scanopt.c skel.c sym.c tables.c tables_shared.c \

Nope, you want to fix config.h



make patch: let MAKEOBJDIR be more powerful

2019-08-29 Thread Marc Espie
After some musings, I realized I just had to reorder a few things to
make MAKEOBJDIR way more powerful (and possibly useful)

The idea here is to init vars early, which is easy, and to set up
.CURDIR, MACHINE, MACHINE_ARCH, MACHINE_CPU, so that
MAKEOBJDIR can actually become a full-blown make expression.

e.g., with this something like

MAKEOBJDIR='${.CURDIR}:S/src/obj/}'

will work.

any takers ?


Index: init.c
===
RCS file: /cvs/src/usr.bin/make/init.c,v
retrieving revision 1.7
diff -u -p -r1.7 init.c
--- init.c  2 Oct 2012 10:29:30 -   1.7
+++ init.c  29 Aug 2019 11:01:19 -
@@ -48,8 +48,6 @@ Init(void)
 * can be processed correctly */
Parse_Init();   /* Need to initialize the paths of #include
 * directories */
-   Var_Init(); /* As well as the lists of variables for
-* parsing arguments */
Arch_Init();
Suff_Init();
 }
Index: main.c
===
RCS file: /cvs/src/usr.bin/make/main.c,v
retrieving revision 1.123
diff -u -p -r1.123 main.c
--- main.c  22 Apr 2019 18:32:09 -  1.123
+++ main.c  29 Aug 2019 11:01:19 -
@@ -558,12 +558,16 @@ setup_CURDIR_OBJDIR(struct dirs *d)
 * and modify the paths for the Makefiles appropriately.  The
 * current directory is also placed as a variable for make scripts.
 */
-   if ((path = getenv("MAKEOBJDIR")) == NULL) {
+   Var_Set(".CURDIR", d->current);
+   if ((path = getenv("MAKEOBJDIR")) == NULL)
path = _PATH_OBJDIR;
-   } 
+   /* if there's a $ in there, allow substitution */
+   else if (strchr(path, '$'))
+   path = Var_Subst(path, NULL, false);
d->object = chdir_verify_path(path, d);
if (d->object == NULL)
d->object = d->current;
+   Var_Set(".OBJDIR", d->object);
 }
 
 /*
@@ -656,6 +660,11 @@ main(int argc, char **argv)
bool read_depend = true;/* false if we don't want to read .depend */
 
MainParseChdir(argc, argv);
+   Var_Init(); /* do this early for OBJDIR */
+   Var_Set("MACHINE", machine);
+   Var_Set("MACHINE_ARCH", machine_arch);
+   Var_Set("MACHINE_CPU", machine_cpu);
+
setup_CURDIR_OBJDIR();
 
esetenv("PWD", d.object);
@@ -687,8 +696,6 @@ main(int argc, char **argv)
 
if (d.object != d.current)
Dir_AddDir(defaultPath, d.current);
-   Var_Set(".CURDIR", d.current);
-   Var_Set(".OBJDIR", d.object);
Parse_setcurdir(d.current);
Targ_setdirs(d.current, d.object);
 
@@ -702,9 +709,6 @@ main(int argc, char **argv)
Var_Set(".MAKE", argv[0]);
Var_Set(MAKEFLAGS, "");
Var_Set("MFLAGS", "");
-   Var_Set("MACHINE", machine);
-   Var_Set("MACHINE_ARCH", machine_arch);
-   Var_Set("MACHINE_CPU", machine_cpu);
 
/*
 * First snag any flags out of the MAKEFLAGS environment variable.



  1   2   3   4   5   6   >