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.



synch make doc/defines with reality

2019-08-22 Thread Marc Espie
This happened a long time ago...
okay ?

Index: make.1
===
RCS file: /cvs/src/usr.bin/make/make.1,v
retrieving revision 1.128
diff -u -p -r1.128 make.1
--- make.1  31 Jan 2019 10:27:28 -  1.128
+++ make.1  22 Aug 2019 16:37:14 -
@@ -1495,8 +1495,7 @@ uses the following environment variables
 .Ev MACHINE_ARCH ,
 .Ev MACHINE_CPU ,
 .Ev MAKEFLAGS ,
-.Ev MAKEOBJDIR ,
-.Ev MAKEOBJDIRPREFIX ,
+.Ev MAKEOBJDIR
 and
 .Ev PWD .
 .Nm
@@ -1516,10 +1515,6 @@ does not exist
 system makefile
 .It Pa /usr/share/mk
 system makefile directory
-.It Pa /usr/obj
-default
-.Ev MAKEOBJDIRPREFIX
-directory
 .El
 .Sh EXIT STATUS
 If
Index: pathnames.h
===
RCS file: /cvs/src/usr.bin/make/pathnames.h,v
retrieving revision 1.12
diff -u -p -r1.12 pathnames.h
--- pathnames.h 19 Jul 2010 19:46:44 -  1.12
+++ pathnames.h 22 Aug 2019 16:37:14 -
@@ -32,18 +32,10 @@
  * from: @(#)pathnames.h   5.2 (Berkeley) 6/1/90
  */
 
-#ifdef HAS_PATH_H
-# include 
-#endif
-#ifndef _PATH_BSHELL
-# define _PATH_BSHELL  "/bin/sh"
-#endif
+#include 
 #ifndef _PATH_OBJDIR
 #define _PATH_OBJDIR   "obj"
 #endif /* !_PATH_OBJDIR */
-#ifndef _PATH_OBJDIRPREFIX
-#define _PATH_OBJDIRPREFIX "/usr/obj"
-#endif /* !_PATH_OBJDIRPREFIX */
 #ifndef _PATH_DEFSHELLDIR
 #define _PATH_DEFSHELLDIR  "/bin"
 #endif /* !_PATH_DEFSHELLDIR */



Re: Host Header Redirection on openbsd.org

2019-08-05 Thread Marc Espie
Well, the main issue I've seen so far is you flooding my mailboxen with
lots of copies of the same useless mp4 video.

What a douche.



Re: signify: -z implies -q

2019-07-29 Thread Marc Espie
On Fri, Jul 26, 2019 at 05:08:06AM +, Mike Burns wrote:
> On 2019-07-26 03.02.43 +, Mike Burns wrote:
> > Poking around signify and learned that `verifyzdata` calls `verifymsg`
> > with `1` hardcoded in the `quiet` parameter.
> > 
> > I do appreciate that there is a distinction between case 'z' setting
> > `quiet = 1` vs `verifyzdata` passing `1` as an argument, so maybe this
> > diff doesn't quiet capture a truth:
> 
> Now that I've played with signify more, it's clear that mentioning -q is
> quite far from from a complete statement of what -z is all about, Forget
> about this diff, apologies.

Yep. The verify part specifically only passes the gzip through if it's
properly signed, and the signature is block-by-block... so it wouldn't
be able to say anything positive until you reach the end of the 
file anyway.



Re: Remove duplicate pledge(2) from tsort(1)

2019-07-11 Thread Marc Espie
On Thu, Jul 11, 2019 at 01:20:11PM +0100, Ricardo Mestre wrote:
> Hi,
> 
> pledge "stdio rpath" is already called in main(), so we can remove the
> duplicate from parse_args(), along with the pledge commented out from another
> era!
> 
> The second part is about placing pledge "stdio" in main() instead for better
> readability (at least for me).
> 
> No functional change is intended here and regress still pass, comments ok?

Sure, I did notice it a few days ago, but did not have time to fix it.

Thanks

> Index: tsort.c
> ===
> RCS file: /cvs/src/usr.bin/tsort/tsort.c,v
> retrieving revision 1.36
> diff -u -p -u -r1.36 tsort.c
> --- tsort.c   20 May 2017 09:31:19 -  1.36
> +++ tsort.c   11 Jul 2019 12:13:00 -
> @@ -879,10 +879,6 @@ parse_args(int argc, char *argv[], struc
>  
>   files[i] = NULL;
>  
> -/*   if (pledge("stdio rpath", files) == -1) */
> - if (pledge("stdio rpath", NULL) == -1)
> - err(1, "pledge");
> -
>   nodes_init(pairs);
>   order = 0;
>   
> @@ -910,9 +906,6 @@ parse_args(int argc, char *argv[], struc
>   order = read_pairs(stdin, pairs, reverse_flag, "stdin",
>   order, hints_flag == 2);
>   }
> -
> - if (pledge("stdio", NULL) == -1)
> - err(1, "pledge");
>  }
>  
>  static int
> @@ -1003,6 +996,10 @@ main(int argc, char *argv[])
>   err(1, "pledge");
>  
>   parse_args(argc, argv, );
> +
> + if (pledge("stdio", NULL) == -1)
> + err(1, "pledge");
> +
>   return tsort();
>  }
>  



Re: Qt5's libtool link scripts are unusable

2019-06-21 Thread Marc Espie
On Thu, Jun 20, 2019 at 09:41:23PM +0200, Rafael Sadowski wrote:
> This's what Qt 5.13 doas. They use "<< endl;" instead of "<< "\n";", I
> would like to prefer that. std::endl calls std::flush which synchronizes
> with the underlying storage device.
> 
> RS


I agree with using endl if that's what Qt 5.13 does.

But it's one specific case where you really do not give a fuck about
flushing.  After all you are generating a Makefile. Who cares about
synchronizing after each single line ?



Re: New implementation and interface for strlcpy and strlcat

2019-05-25 Thread Marc Espie
On Fri, May 24, 2019 at 07:22:07PM +0700, Oleg Chumanov wrote:
> First of all, I really want to thank you for your files.
> 
> However, Can you show me an example of these:
> 
> > makes it easier to perform error
> > recovery in the caller. For example, the caller may wish to
> > reallocate the buffer and retry.
> >
> > That would make it a bit harder to do error recovery since you'd
> > need to compute the required length
> 
> I mean, I can do something like this:
> 
>   offset = 0;
>   while ((n = olegcpy(buf + offset, str + offset, buf_size - offset))) {
>   offset   = buf_size - 1;
>   buf_size += n;
>   buf   = realloc(buf, buf_size);
>   }

Of course, you could, but you are totally missing the point of 
strlcpy/strlcat.

Look, the basic idea is that developers make mistakes.

All of them.

Including *you*.

Not all the time, but once is enough, especially where buffer overflows
are concerned.

Your proposed interface (which I renamed olegcpy) is more complex to use 
than regular strlcpy.

Look, you *are* doing pointer arithmetic in your loop.

Oh, btw, your code is wrong... you forgot to check that buf_size didn't
overflow (unlikely, I know), and you *also* forgot to check that realloc
did work.

That's not your point ?

Actually, it *is*. You had to devote brain cells to thinking about your
very smart loop, so you missed a few trivial points.

That's what strlcpy is all about. It's a PLAIN SIMPLE interface designed
for ACTUAL PEOPLE.

Compare that with strlcpy

while (1) {
size_t n = strlcpy(buf, str, buf_size);
if (n < bufsize)
break;
char *newb = realloc(buf, n);
if (!newb) {
free(buf);
/* oh hey, that's an error */
exit(1);
}
buf = newb;
buf_size = n;
}

Oh, hey, I didn't forget to test for actual errors. Because the code
is simpler.

So, yes, I copy the same string twice, once in a blue moon.
Guess what ? FOR REAL PROGRAMS IT DOESN'T MATTER.

*IF* your buffer sizes are carefully chosen, you won't reallocate them
THAT often (especially because the smart thing to do is to devote energy
to actually track average buffer sizes and reuse buffers so you don't
reallocate all the time).

So, in reality, I won't ever realloc *exactly* what strlcpy gives me.
My code looks more like this usually:

while (1) {
size_t n = strlcpy(buf, str, buf_size);
if (n < bufsize)
break;
while (n > bufsize)
bufsize <<= 1U;
char *newb = realloc(buf, bufsize);
if (!newb) {
free(buf);
/* oh hey, that's an error */
exit(1);
}
buf = newb;
}

and I do *love* strlcpy and strlcat because they're simple enough to use
that I can spend my time concentrating on stuff that actually matters,
without too much risk at introducing more bugs in existing programs.




"Premature optimization is the root of all evil" (Don E. Knuth)



chroot vs su vs doas

2019-05-13 Thread Marc Espie
So, in dpb, I've been forking a lot of 'chroot -u user /build'
to build various things, and it works just great.

I was wondering about the benefits of
su ${BUILDUSER} -c 'some quoted command'
vs
chroot -u ${BUILDUSER} / some unquoted command

Superficially, it looks mostly similar.  

The very nice thing about chroot (IMO) being that you can pass the
command verbatim without having to re-quote anything.  The other
difference being that chroot doesn't fork an extra shell, which
might make things more transparent wrt running commands.

I'm also wondering about doas.
By default, it's not configured at all.

But what would it hurt to allow root usage ?
Specifically,

doas -u ${BUILDUSER} some unquoted command

as run by root.  This would not open any security hole, would it ?

Finally, I'm wondering if people would have any use for a chroot'd
option in doas, and whether it's a security issue (again).

Like, people have some hardened doas.conf which only allows running
some commands as root.

Some of these commands are basically game over, as they allow anything
to be run, actually. Specifically, /usr/bin/env, or chroot...

Would
doas -c /rootdir somecmd
be of any use ?



Re: [patch] improve strptime(3) %z timezone parsing

2019-03-17 Thread Marc Espie
On Sun, Mar 17, 2019 at 03:10:04PM -0400, Ted Unangst wrote:
> Hiltjo Posthuma wrote:
> > Not all BSD man pages reference the same RFCs, like RFC5322. The OpenBSD
> > strptime(3) man page (%z) describes military/nautical zones are supported.
> > 
> > - NetBSD supports it (correctly since only a few months ago and it is not in
> >   stable or backported).
> > - FreeBSD and DragonFlyBSD do not support them (strptime returns NULL).
> > - Linux glibc does not support them (strptime returns NULL).
> > - Linux musl libc does not support %z.
> 
> This sounds like we should just drop support as well. I think that's better
> than supporting something that's clearly unreliable.

By that reckoning, you would never implement anything new... :(



Re: remove date from signify zsig

2019-02-25 Thread Marc Espie
On Mon, Feb 25, 2019 at 05:11:54PM -0500, Ted Unangst wrote:
> Marc Espie wrote:
> > On Mon, Feb 25, 2019 at 03:02:42PM -0500, Ted Unangst wrote:
> > > Andre Stoebe wrote:
> > > > Hi,
> > > > 
> > > > I, too, would like to have a way of signing the gzip archive in a
> > > > reproducible way, so here's a diff that uses -n, similar to gzip(1).
> > > > 
> > > > However, if that's a bad idea, I'm fine with continuing to use an
> > > > unsigned gzip archive and creating a sigfile with signify.
> > > 
> > > Let me think on this for a bit. Seems reasonable, though.
> > 
> > If you want something simpler, just set the date from outside through an
> > env variable, so you'll have a reproducible date line for when you 
> > absolutely
> > need it.
> 
> Like TZ? I don't think there's a way to change the time that way. Is there?

No, but instead of an extra option, a specific env variable ? might make more
sense... or less. I don't know.

I'm surprised this surfaced again, as the subject was broached a few months
ago and dismissed, because yep, we do want the timestamp to mean something
for pkg_add.

Especially relating to our keys having a shelf life.



Re: Harmonize examples in signify man page

2019-02-25 Thread Marc Espie
On Sun, Feb 24, 2019 at 10:09:43PM +0100, Matthias Schmidt wrote:
> Hi,
> 
> while looking at signify's man page I noticed that the names of the
> arguments in the EXAMPLES section change (newkey.sec vs key.sec, etc).
> The diff harmonizes the examples.  It makes it easier to follow the examples. 
> 
> Cheers
> 
>   Matthias
> 
>  diff --git signify.1 signify.1
>  index de3b433e3b0..9f7335352a2 100644
>  --- signify.1
>  +++ signify.1
>  @@ -153,13 +153,13 @@ The message file is too large.
>   .El
>   .Sh EXAMPLES
>   Create a new key pair:
>  -.Dl $ signify -G -p newkey.pub -s newkey.sec
>  +.Dl $ signify -G -p key.pub -s key.sec
>   .Pp
>   Sign a file, specifying a signature name:
>  -.Dl $ signify -S -s key.sec -m message.txt -x msg.sig
>  +.Dl $ signify -S -s key.sec -m message -x message.sig

Your mileage may vary, but this makes things harder to follow for me.

Obviously visually differentiating between creating new keys and using
existing ones makes a lot of sense for me.

Especially since the examples are not meant to be chained together, at
all.  This is a reference manual, not a tutorial.



Re: remove date from signify zsig

2019-02-25 Thread Marc Espie
On Mon, Feb 25, 2019 at 03:02:42PM -0500, Ted Unangst wrote:
> Andre Stoebe wrote:
> > Hi,
> > 
> > I, too, would like to have a way of signing the gzip archive in a
> > reproducible way, so here's a diff that uses -n, similar to gzip(1).
> > 
> > However, if that's a bad idea, I'm fine with continuing to use an
> > unsigned gzip archive and creating a sigfile with signify.
> 
> Let me think on this for a bit. Seems reasonable, though.

If you want something simpler, just set the date from outside through an
env variable, so you'll have a reproducible date line for when you absolutely
need it.



Re: locate.mklocatedb broken with LC_ALL!=C

2019-02-15 Thread Marc Espie
On Fri, Feb 15, 2019 at 07:58:48PM +0100, Giovanni Bechis wrote:
> ping...
> any possible issue with millert@ diff ?
>  Giovanni
> 
> On Sun, Oct 07, 2018 at 08:35:28PM -0600, Todd C. Miller wrote:
> > On Sun, 07 Oct 2018 17:08:06 +0200, Marc Espie wrote:
> > 
> > > Specifically, the only part that cares about
> > > locale is sort, and it's definitely correct in fixing
> > > it's not run on an utf-8 file.
> > 
> > Agreed.  How about the following?
> > 
> >  - todd
> > 
> > Index: usr.bin/locate/locate/mklocatedb.sh
> > ===
> > RCS file: /cvs/src/usr.bin/locate/locate/mklocatedb.sh,v
> > retrieving revision 1.13
> > diff -u -p -u -r1.13 mklocatedb.sh
> > --- usr.bin/locate/locate/mklocatedb.sh 18 Mar 2007 20:13:49 -  
> > 1.13
> > +++ usr.bin/locate/locate/mklocatedb.sh 8 Oct 2018 02:34:52 -
> > @@ -66,7 +66,8 @@ filelist=`mktemp ${TMPDIR=/tmp}/_filelis
> >  }
> >  trap 'rm -f $bigrams $filelist' 0 1 2 3 5 10 15
> >  
> > -if $sortcmd $sortopt > $filelist; then
> > +# Run sort in the C locale or binary data may be interpreted as UTF-8
> > +if LC_ALL=C $sortcmd $sortopt > $filelist; then
> >  $bigram < $filelist | $sort -nr | 
> >  awk -Ft 'BEGIN { ORS = "" } NR <= 128 { print $2 }' > 
> > $bigrams &&
> >  $code $bigrams < $filelist 


Oh, I thought it had been committed ages ago



Re: PKG_SIGN(1) - D argument

2019-02-15 Thread Marc Espie
On Fri, Feb 15, 2019 at 11:55:59AM +0100, Oleg Pahl wrote:
> Hi @all,
> 
> Is it ok, that on man page *PKG_SIGN(1)* there is*[-D name=[value]]*
> argument,
> 
> but there is no info about value of arg. below ... (I mean what kind of
> value's is possible to use with -D)
> 
> I think, that its will be better to put this Info on man page. Not just for
> me =)

There are options intentionally left undocumented in a lot of places.
Sometimes they are not intended for public consumption for instance (non
regress test comes to mind) but they're part of the command syntax anyway.

Have fun finding all the places where this happens, pkg_sign(1) is definitely
not the only command with undocumented options.



Re: firmware.openbsd.org (SHA256)

2019-02-13 Thread Marc Espie
On Wed, Feb 13, 2019 at 04:41:56PM +0100, Oleg Pahl wrote:
> Hi all,
> 
> I use 6.4 Release.
> I install fm on my laptop from http://firmware.openbsd.org/firmware/6.4/
> This URL i found in man page FW_UPDATE(1)
> You can see that ( index.txt ) has one file more then as on server!

It doesn't matter.

Getting a consistent global SHA256 / SHA256.sig  for distributed sets
of packages or firmwares   is  difficult at best.

For precisely that reason, packages are individually signed.

And both pkg_add *and* fw_update will refuse to install anything that's
not signed *by default*.

You can actually check the signature yourself, it's directly in the gzip
header comment (so that you can't pass unsigned data through zlib for
decompressions).

RTFM signify(1)  -z mode



Re: install(1) broken

2019-02-11 Thread Marc Espie
On Sun, Feb 10, 2019 at 03:55:41PM +0100, Marc Espie wrote:
> Something similar to this  perhaps ?
> Not fully tested yet, but it should avoid the race of trying to 
> unlink tempfile several times, and also fix the file name in error messages.

That's now been tested (gone thru a build + xbuild and bulk in progress
that went thru go-bootstrap and go without issues).

Okay ?

This needs re-applying Ingo's patch first, of course, which I can either do
or we coordinate with Ingo.



Index: xinstall.c
===
RCS file: /cvs/src/usr.bin/xinstall/xinstall.c,v
retrieving revision 1.68
diff -u -p -r1.68 xinstall.c
--- xinstall.c  8 Feb 2019 12:53:44 -   1.68
+++ xinstall.c  10 Feb 2019 14:53:49 -
@@ -222,6 +222,7 @@ install(char *from_name, char *to_name, 
struct timespec ts[2];
int devnull, from_fd, to_fd, serrno, files_match = 0;
char *p;
+   char *target_name = tempfile;
 
(void)memset((void *)_sb, 0, sizeof(from_sb));
(void)memset((void *)_sb, 0, sizeof(to_sb));
@@ -311,10 +312,14 @@ install(char *from_name, char *to_name, 
} else {
files_match = 1;
(void)unlink(tempfile);
+   target_name = to_name;
+   (void)close(temp_fd);
}
}
-   (void)close(to_fd);
-   to_fd = temp_fd;
+   if (!files_match) {
+   (void)close(to_fd);
+   to_fd = temp_fd;
+   }
}
 
/*
@@ -333,13 +338,15 @@ install(char *from_name, char *to_name, 
if ((gid != (gid_t)-1 || uid != (uid_t)-1) &&
fchown(to_fd, uid, gid)) {
serrno = errno;
-   (void)unlink(tempfile);
-   errx(1, "%s: chown/chgrp: %s", tempfile, strerror(serrno));
+   if (target_name == tempfile)
+   (void)unlink(tempfile);
+   errx(1, "%s: chown/chgrp: %s", target_name, strerror(serrno));
}
if (fchmod(to_fd, mode)) {
serrno = errno;
-   (void)unlink(tempfile);
-   errx(1, "%s: chmod: %s", tempfile, strerror(serrno));
+   if (target_name == tempfile)
+   (void)unlink(tempfile);
+   errx(1, "%s: chmod: %s", target_name, strerror(serrno));
}
 
/*
@@ -349,7 +356,7 @@ install(char *from_name, char *to_name, 
if (fchflags(to_fd,
flags & SETFLAGS ? fset : from_sb.st_flags & ~UF_NODUMP)) {
if (errno != EOPNOTSUPP || (from_sb.st_flags & ~UF_NODUMP) != 0)
-   warnx("%s: chflags: %s", tempfile, strerror(errno));
+   warnx("%s: chflags: %s", target_name, strerror(errno));
}
 
if (flags & USEFSYNC)



Re: install(1) broken

2019-02-10 Thread Marc Espie
Something similar to this  perhaps ?
Not fully tested yet, but it should avoid the race of trying to 
unlink tempfile several times, and also fix the file name in error messages.

Index: xinstall.c
===
RCS file: /cvs/src/usr.bin/xinstall/xinstall.c,v
retrieving revision 1.68
diff -u -p -r1.68 xinstall.c
--- xinstall.c  8 Feb 2019 12:53:44 -   1.68
+++ xinstall.c  10 Feb 2019 14:53:49 -
@@ -222,6 +222,7 @@ install(char *from_name, char *to_name, 
struct timespec ts[2];
int devnull, from_fd, to_fd, serrno, files_match = 0;
char *p;
+   char *target_name = tempfile;
 
(void)memset((void *)_sb, 0, sizeof(from_sb));
(void)memset((void *)_sb, 0, sizeof(to_sb));
@@ -311,10 +312,14 @@ install(char *from_name, char *to_name, 
} else {
files_match = 1;
(void)unlink(tempfile);
+   target_name = to_name;
+   (void)close(temp_fd);
}
}
-   (void)close(to_fd);
-   to_fd = temp_fd;
+   if (!files_match) {
+   (void)close(to_fd);
+   to_fd = temp_fd;
+   }
}
 
/*
@@ -333,13 +338,15 @@ install(char *from_name, char *to_name, 
if ((gid != (gid_t)-1 || uid != (uid_t)-1) &&
fchown(to_fd, uid, gid)) {
serrno = errno;
-   (void)unlink(tempfile);
-   errx(1, "%s: chown/chgrp: %s", tempfile, strerror(serrno));
+   if (target_name == tempfile)
+   (void)unlink(tempfile);
+   errx(1, "%s: chown/chgrp: %s", target_name, strerror(serrno));
}
if (fchmod(to_fd, mode)) {
serrno = errno;
-   (void)unlink(tempfile);
-   errx(1, "%s: chmod: %s", tempfile, strerror(serrno));
+   if (target_name == tempfile)
+   (void)unlink(tempfile);
+   errx(1, "%s: chmod: %s", target_name, strerror(serrno));
}
 
/*
@@ -349,7 +356,7 @@ install(char *from_name, char *to_name, 
if (fchflags(to_fd,
flags & SETFLAGS ? fset : from_sb.st_flags & ~UF_NODUMP)) {
if (errno != EOPNOTSUPP || (from_sb.st_flags & ~UF_NODUMP) != 0)
-   warnx("%s: chflags: %s", tempfile, strerror(errno));
+   warnx("%s: chflags: %s", target_name, strerror(errno));
}
 
if (flags & USEFSYNC)



Re: install(1) broken

2019-02-10 Thread Marc Espie
On Sat, Feb 09, 2019 at 05:23:09PM -0500, Ted Unangst wrote:
> Marc Espie wrote:
> > hey, your commit to install(1) broke something.
> > 
> > Specifically lang/go-boostrap now produces a broken package which can't
> > be used to build go.
> > 
> > All the go/bootstrap/pkg/tool/openbsd_amd64/*
> > have lost their x bit
> > 
> > Relevant fake install information, it definitely looks like the last line
> > is now a no-op.
> 
> > # These get installed via `find' however we need them to be executable
> > /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 
> > /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/pkg/tool//openbsd_amd64
> > /pobj/go-bootstrap-1.4.20171003/bin/install -c  -m 755 -p 
> > /pobj/go-bootstrap-1.4.20171003/go/pkg/tool//openbsd_amd64/* 
> > /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/pkg/tool//openbsd_amd64
> 
> Yes. This is a weird way to invoke chmod, but that's what it wants.
> 
> I think this was a long standing bug in install? If the files match, we need
> to continue on with to_fd == existing file so that metadata updates work.
> 
> 
> Index: xinstall.c
> ===
> RCS file: /cvs/src/usr.bin/xinstall/xinstall.c,v
> retrieving revision 1.68
> diff -u -p -r1.68 xinstall.c
> --- xinstall.c8 Feb 2019 12:53:44 -   1.68
> +++ xinstall.c9 Feb 2019 22:21:03 -
> @@ -313,8 +313,12 @@ install(char *from_name, char *to_name, 
>   (void)unlink(tempfile);
>   }
>   }
> - (void)close(to_fd);
> - to_fd = temp_fd;
> + if (!files_match) {
> + (void)close(to_fd);
> + to_fd = temp_fd;
> + } else {
> + close(temp_fd);
> + }
>   }
>  
>   /*
I'm afraid this needs to be slightly more complex, probably to put the
remaining code in its own function, or something.

Specifically, the error paths for the fchown and fchmod  will be all bogus
in that case, referring to a tempfile  that's already been removed, and
is not even the target...



Re: install(1) broken

2019-02-09 Thread Marc Espie
On Sat, Feb 09, 2019 at 05:23:09PM -0500, Ted Unangst wrote:
> Marc Espie wrote:
> > hey, your commit to install(1) broke something.
> > 
> > Specifically lang/go-boostrap now produces a broken package which can't
> > be used to build go.
> > 
> > All the go/bootstrap/pkg/tool/openbsd_amd64/*
> > have lost their x bit
> > 
> > Relevant fake install information, it definitely looks like the last line
> > is now a no-op.
> 
> > # These get installed via `find' however we need them to be executable
> > /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 
> > /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/pkg/tool//openbsd_amd64
> > /pobj/go-bootstrap-1.4.20171003/bin/install -c  -m 755 -p 
> > /pobj/go-bootstrap-1.4.20171003/go/pkg/tool//openbsd_amd64/* 
> > /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/pkg/tool//openbsd_amd64
> 
> Yes. This is a weird way to invoke chmod, but that's what it wants.

This actually makes some kind of sense in a broader context.

Specifically, install_flags are a somewhat standard way to enforce
ownership/permissions on a file, whether while copying it from one place
to another... or after moving it.

It's way simpler to bundle everything into a single variable than having
several separate variables to do things.



Re: Update perl to 5.28.1

2019-02-03 Thread Marc Espie
On Sun, Feb 03, 2019 at 02:23:16PM +, Stuart Henderson wrote:
> On 2018/12/24 19:05, Andrew Hewus Fresh wrote:
> > I've finally gotten our local patches applied to perl 5.28.1 and it now
> > could use some testing.  I was trying to get a few more of my
> > architectures working, but I ran out of time and now I have to go visit
> > family for the holidays so it's up to y'all to do some testing for me.
> > 
> > 
> > The infrastructure to do that is still here on github, including
> > instructions on using it:
> > 
> > https://github.com/afresh1/OpenBSD-perl/
> > 
> > 
> > 
> > You can also download a pre-patched perl from here, use it to replace
> > src/gnu/usr.bin/perl and do a normal build to get it installed, do note
> > that any XS modules that were built with the old version of perl will no
> > longer work, so be careful.
> > 
> > http://cvs.afresh1.com/~andrew/OpenBSD-perl-5.28.1.tar.gz
> > 
> > 
> > You can read more on the new features and changes on the metacpan.
> > 
> > https://metacpan.org/release/SHAY/perl-5.28.1
> > 
> > You find them in the perldelta for the versions since 5.24.
> > 
> > https://metacpan.org/pod/release/SHAY/perl-5.28.1/pod/perl5260delta.pod
> > https://metacpan.org/pod/release/SHAY/perl-5.28.1/pod/perl5280delta.pod
> > https://metacpan.org/pod/release/SHAY/perl-5.28.1/pod/perldelta.pod
> > 
> > 
> > One of the important changes, and the one most likely to cause trouble
> > is the removal of "." from the @INC library search list.
> 
> Having fixed a bunch of these, and cwen@ fixing a bunch more, and still
> having 100-odd ports failing to build, I have come round to the idea of
> setting PERL_USE_UNSAFE_INC instead. I wasn't super happy about this but
> the upstream cpan tool seems resigned to doing this for now so we might
> as well follow suit.
> 
> It seems sanest to set this in cpan.port.mk rather than individual Makefiles:
> 
> Index: cpan.port.mk
> ===
> RCS file: /cvs/ports/infrastructure/mk/cpan.port.mk,v
> retrieving revision 1.20
> diff -u -p -r1.20 cpan.port.mk
> --- cpan.port.mk  20 Mar 2016 19:56:44 -  1.20
> +++ cpan.port.mk  3 Feb 2019 14:10:27 -
> @@ -24,6 +24,11 @@ TEST_DEPENDS +=devel/p5-Test-Pod \
>   devel/p5-Test-Pod-Coverage
>  .endif
>  
> +# perl 5.26+ no longer has "." in @INC by default, but it's widely required 
> in
> +# build/test systems. set it locally, as is also done in the upstream cpan 
> tool.
> +CONFIGURE_ENV += PERL_USE_UNSAFE_INC=1
> +TEST_ENV += PERL_USE_UNSAFE_INC=1
> +
>  MODCPAN_POST_INSTALL = ${INSTALL_DATA_DIR} ${MODCPAN_EXAMPLES_DIR}; \
>   cd ${WRKSRC}/${MODCPAN_EXAMPLES_DIST}/ && pax -rw . 
> ${MODCPAN_EXAMPLES_DIR};\
>   chown -R ${SHAREOWN}:${SHAREGRP} ${MODCPAN_EXAMPLES_DIR}
> 
> 
> Applying that diff fixes 80+. Any major concerns about doing that
> (especially espie@, afresh1@)? It hasn't been through a full build
> (I added it after a run to mop up breakage) but I can do that.

No, I think it's sane to do that. Let cpan mop up most of the breakage,
then we'll remove that.



Re: make.1: add missing dependency to example

2019-01-31 Thread Marc Espie
On Wed, Jan 30, 2019 at 01:56:04PM -0600, Scott Cheloha wrote:
> We need to tell ${CC} about b.o explicitly.
> 
> ok?
> 
> -Scott
> 
> P.S. How was BSD $> or GNU $^ or an equivalent *not* standardized as
> an automatic variable?  I get that it isn't a silver bullet, but in
> the relatively common "let's take several  and make a " case it
> is extremely useful and totally missing from the portable syntax.

Well, it's not standardized.

As far as adding extensions to bsd tools, it's always a bit annoying,
because we then foster making things *less* portable to other environments
that don't have it.  So generally, it's more a question of knowing when
it's wide-spread enough (e.g., sed -i).


make is *very* sneaky in that regard... :(

In general, the portable way to do things properly is that as soon as you
have two objects files, you got thru a variable, which is good practice
anyway.




> Index: make.1
> ===
> RCS file: /cvs/src/usr.bin/make/make.1,v
> retrieving revision 1.127
> diff -u -p -r1.127 make.1
> --- make.14 Jul 2018 14:11:49 -   1.127
> +++ make.130 Jan 2019 19:45:15 -
> @@ -464,7 +464,7 @@ from sources a.c and b.c, with header fi
>   ${CC} ${CFLAGS} -c $<
>  
>  prog: a.o b.o
> - ${CC} ${CFLAGS} -o $@ a.o
> + ${CC} ${CFLAGS} -o $@ a.o b.o
>  
>  a.o b.o: a.h
>  
Yep, okay



Re: strlcpy() or strscpy()?

2019-01-27 Thread Marc Espie
On Sun, Jan 27, 2019 at 01:07:05AM -0500, 0sjfoij...@firemail.cc wrote:
> Recently on LCA2019, Joel Sing made a presentation about "Security
> Vulnerability Mitigations"[1]
> (very good, btw). He suggests function strlcpy(3) as a secure API.
> In the same conference, though, Kees Cook ("Making C Less Dangerous in the
> Linux kernel"[2]),
> recommends strscpy() as more secure. So, my question is: what's the best to
> use?
> 
If anything, strscpy() made things more INsecure, by adding another 
confusing variation on a well-defined API that has proven itself.

strscpy reeks of "Not Invented Here", "Misplaced Hubris" and "Design by
committee" syndrome, which happens so very often in Linux-land.

Their only saving grace is darwinian: there are so many lemmings over there
that ludicrous ideas tend to get replaced by better ideas over time.



Re: friendlier doas persist

2019-01-24 Thread Marc Espie
On Wed, Jan 23, 2019 at 05:55:54PM -0500, Ted Unangst wrote:
> Stuart Henderson wrote:
> > On 2019/01/22 21:46, Ted Unangst wrote:
> > > The persist feature in doas (actually the kernel side implementation) has 
> > > some
> > > additional checks. The idea was to prevent accidental usage, but in 
> > > practice
> > > it seems this is making life more difficult than necessary. It's cost 
> > > without
> > > benefit. This diff relaxes the session checks so it should be possible to 
> > > use
> > > doas in ports building, etc.
> > 
> > Fixing this would be very welcome and would stop some ports users from
> > either running with nopass or continuing to use sudo. But it doesn't help:
> > 
> > configure doas to use persist
> > set SUDO=doas in mk.conf
> > cd /usr/ports/math/moo# example simple/quick port
> > make reinstall
> > 
> > Here you're asked for your password twice.
> 
> Is make creating a second process group? I guess it is for job control? Need
> to find a place to stick the verauth flag such that later doas can find it...

No it's not.

Doesn't mean *something else* in the chain isn't.



Re: pkg_add testing

2018-12-16 Thread Marc Espie
On Wed, Dec 12, 2018 at 03:20:49PM +0100, Marc Espie wrote:
> Please test:
> 
> pkg_add -u -DSHORTENED

And Landry found an issue. I forgot to update some code when I tweaked stuff
to add @version support.

Please try again with just committed pkg_add code...

(basically, the older version didn't update some stuff it should have, so
you'll just have slightly more updates than before, shouldn't be harmful



Re: make build as root fails when SUDO=doas

2018-12-14 Thread Marc Espie
On Fri, Dec 14, 2018 at 03:57:12PM -0500, Ted Unangst wrote:
> Marc Espie wrote:
> > Well, apart from the bike-shedding, it seems like the most correct
> > short-term solution.
> > 
> > So I will commit it tomorrow, unless someone has an actual better idea.
> 
> Nobody answered if SUDO_CLEAN is actualy set. I checked. It's not.
> 
> Index: Makefile
> ===
> RCS file: /cvs/src/regress/usr.bin/ssh/Makefile,v
> retrieving revision 1.98
> diff -u -p -r1.98 Makefile
> --- Makefile  22 Nov 2018 08:48:32 -  1.98
> +++ Makefile  14 Dec 2018 20:56:11 -
> @@ -209,7 +209,6 @@ c-${s}:
>  
>  clean: ${CLEAN_SUBDIR}
>   rm -f ${CLEANFILES}
> - test -z "${SUDO}" || ${SUDO} rm -f ${SUDO_CLEAN}
>   rm -rf .putty
>  
>  .include 

Oh, I had a slightly older version.

So, yep, it was just dtucker@  half-committing some clean-up.

Okay to finish fixing it. :)



Re: make build as root fails when SUDO=doas

2018-12-14 Thread Marc Espie
On Wed, Dec 12, 2018 at 11:12:17AM +0100, Alexander Bluhm wrote:
> On Tue, Dec 11, 2018 at 11:00:30PM +0100, Marc Espie wrote:
> > Ah, so actually just
> > rm -f ${SUDO_CLEAN}
> > 
> > should be fine ?
> 
> Regress jumps from root to non-root in a very inconsistent way.  It
> could be improved, but that would be a lot of work.  The result
> will not be perfect as tests have very different needs for permissions.
> So I would recommend to leave it as it is unless someone volunteers
> to refactor all the tests.
> 
> If a test creates a file as root with an ugly ${SUDO} hack, it
> should use the same hack to clean it.  Then the test is self
> contained.  Care should be taken that the make clean hack does not
> affect the make build.
> 
> So I like espie@'s suggestion to try rm first without ${SUDO} for
> people running make build, and then with ${SUDO} for people who
> have left overs from testing.
> 
> bluhm

Well, apart from the bike-shedding, it seems like the most correct
short-term solution.

So I will commit it tomorrow, unless someone has an actual better idea.



pkg_add testing

2018-12-12 Thread Marc Espie
5 years agom I introduced a tweak to pkg_add to forego updates when they
weren't strictly necessary.  Then I made it the default.

This caused some unidentified problems at the time, and the code was
backed out by kili@ soon afterward.

I just *fixed* a weird bug in pkg_add, and there's a good chance it's directly
related: the bug would show up only when packages said "no need to update"
but leading to a shared library a few steps down in the chain of depends;
since the older tweak would lead to fewer updates, it would trigger the
bug more often.

So now is the time for testing that tweak again, see if it can become
default.

Please test:

pkg_add -u -DSHORTENED

will forego updates when the package being updated didn't really change
version (the update would only be triggered because the packages it was
built against were slightly newer, but the shared libraries would be the
same).

If nothing untowards happens, it probably means we nailed that 5 years old
problem...



Re: make build as root fails when SUDO=doas

2018-12-11 Thread Marc Espie
On Tue, Dec 11, 2018 at 10:55:25PM +0100, Claudio Jeker wrote:
> On Tue, Dec 11, 2018 at 02:35:33PM -0700, Theo de Raadt wrote:
> > Ted Unangst  wrote:
> > 
> > > Marc Espie wrote:
> > > > > > - try to remove the files normally first
> > > > > >  rm -f ${SUDO_CLEAN} || test -z "${SUDO}" || ${SUDO} rm -f 
> > > > > > ${SUDO_CLEAN}
> > > > > > 
> > > > > > this should actually fix the issue.
> > > > > > 
> > > > > > Any other directory with that problem ?
> > > > > 
> > > > > that fix the issue and the build continues fine
> > > > 
> > > > So okay from source people ? tedu, guenther, theo, krw ? somebody else ?
> > > 
> > > does anywhere else in the tree do this? aren't most (all) things either 
> > > done
> > > as root or not done as root?
> > 
> > I also don't understand what the point is here.
> > 
> > release(9) shows the correct build process.
> > 
> > you start build as root, to permit the priv-drop security model we
> > designed in 2017.
> > 
> > If on the other hand you build from a regular user below, with doas
> > configured to allow escalation at any point in time, the regular user
> > below CAN ALWAYS BECOME ROOT SO YOU HAVE NO SECURITY MODEL IN MIND AT
> > ALL, while you operate on Makefile and such you downloaded from elsewhere
> > 
> > Such use of sudo/doas is an ANTI-PATTERN
> 
> I think the main issue is that /usr/sr/regress was not moved to the
> priv-drop security model. There is bunch of code which needs root but I
> don't want to run all of regress as user root. 

There is a kind of mixed model there.

Because make build still goes thru regress for obj and cleandir

Yet the rest of the build doesn't!

So, if we agree that it needs to stay the way it currently is, then
the SUDO in that Makefile might trigger while running as root...

... or we could change all the ports tree and rename SUDO to something
else in there so that it doesn't interfere at all.  But I see most porters
not too happy with that choice.

As Mr Morden would say "what do you want ?"...



Re: make build as root fails when SUDO=doas

2018-12-11 Thread Marc Espie
On Tue, Dec 11, 2018 at 02:35:33PM -0700, Theo de Raadt wrote:
> Ted Unangst  wrote:
> 
> > Marc Espie wrote:
> > > > > - try to remove the files normally first
> > > > >  rm -f ${SUDO_CLEAN} || test -z "${SUDO}" || ${SUDO} rm -f 
> > > > > ${SUDO_CLEAN}
> > > > > 
> > > > > this should actually fix the issue.
> > > > > 
> > > > > Any other directory with that problem ?
> > > > 
> > > > that fix the issue and the build continues fine
> > > 
> > > So okay from source people ? tedu, guenther, theo, krw ? somebody else ?
> > 
> > does anywhere else in the tree do this? aren't most (all) things either done
> > as root or not done as root?
> 
> I also don't understand what the point is here.
> 
> release(9) shows the correct build process.
> 
> you start build as root, to permit the priv-drop security model we
> designed in 2017.
> 
> If on the other hand you build from a regular user below, with doas
> configured to allow escalation at any point in time, the regular user
> below CAN ALWAYS BECOME ROOT SO YOU HAVE NO SECURITY MODEL IN MIND AT
> ALL, while you operate on Makefile and such you downloaded from elsewhere
> 
> Such use of sudo/doas is an ANTI-PATTERN

Ah, so actually just
rm -f ${SUDO_CLEAN}

should be fine ?



  1   2   3   4   5   6   >