Re: [PATCH] Add -executable, -readable, and -writable options to /usr/bin/find

[Andreas Kusalananda Kähäri]

On Sun, Apr 02, 2023 at 10:39:00AM +0200, Kusalananda Kähäri wrote:
> On Sat, Apr 01, 2023 at 08:18:56AM +0200, Solène Rapenne wrote:
> > Le Fri, 31 Mar 2023 19:40:45 -0700,
> > Jared Harper  a écrit :
> > 
> > > I have put together a patch that adds -executable, -readable, and
> > > -writable to /usr/bin/find.
> > > 
> > > When I first started working on this patch, I implemented the access
> > > check by checking the stat of the file like so:
> > > 
> > 
> > this doesn't add much value IMO, we already have -perm that can be used
> > to return paths matching the permission, or only a bit.
> > 
> > find . -executable can be written find . -type f -perm -100
> > find . -writable can   be written find . -type f -perm -200
> > find . -readable can   be written find . -type f -perm -400
> > 
> > on linux, those flags make sense to have because they also take care of
> > ACLs, while their -perm doesn't. OpenBSD doesn't have ACLs.
> 
> 
> Note that e.g. -executable tests whether the current item is readable

s/readable/executable


duh

-- 
Andreas (Kusalananda) Kähäri
SciLifeLab, NBIS, ICM
Uppsala University, Sweden

.

Re: [PATCH] Add -executable, -readable, and -writable options to /usr/bin/find

[Andreas Kusalananda Kähäri]

On Sat, Apr 01, 2023 at 08:18:56AM +0200, Solène Rapenne wrote:
> Le Fri, 31 Mar 2023 19:40:45 -0700,
> Jared Harper  a écrit :
> 
> > I have put together a patch that adds -executable, -readable, and
> > -writable to /usr/bin/find.
> > 
> > When I first started working on this patch, I implemented the access
> > check by checking the stat of the file like so:
> > 
> 
> this doesn't add much value IMO, we already have -perm that can be used
> to return paths matching the permission, or only a bit.
> 
> find . -executable can be written find . -type f -perm -100
> find . -writable can   be written find . -type f -perm -200
> find . -readable can   be written find . -type f -perm -400
> 
> on linux, those flags make sense to have because they also take care of
> ACLs, while their -perm doesn't. OpenBSD doesn't have ACLs.


Note that e.g. -executable tests whether the current item is readable
by the current user, a test that is quite difficult to implement using
-perm (you would have to involve -user and possibly -group too, as well
as $LOGNAME).

With -executable as an example, you could avoid entering directories
that you don't have access to in a convenient way

find . -type d ! -executable -prune -o ...

GNU extensions to standard utilities are almost exclusively about
providing convenient shortcuts and convinient functionality to the lazy
user.  Usually, this extra functionality is found in other standard
tools, or it can trivially be implemented by a short script written
in the shell or using awk etc., so their extensions often seems to be
running counter to the Unix philsophy of "do one thing and do it well".
But then again, "GNU is not Unix".

However, in this particular case, the extension is *useful* and not
at all trivial to implement with existing predicates in find(1) or by
calling other utilities.  The above example could be implemented with
"find . -type d ! -exec cd {} \; -prune -o ..." on systems where cd(1)
is an external utility, which it is not on OpenBSD, so we would use
something silly like "! -exec sh -c 'cd "$1"' sh {} \;" instead, unless
we got either really creative or just ignored the fact that we can't
enter that directory and redirect all diagnostic messages to /dev/null.


Cheers,

-- 
Andreas (Kusalananda) Kähäri
SciLifeLab, NBIS, ICM
Uppsala University, Sweden

.

Re: [PATCH] Add -executable, -readable, and -writable options to /usr/bin/find

[Anthony J. Bentley]

Stuart Henderson writes:
> On 2023/04/01 11:27, Jared Harper wrote:
> > For some reason I haven't received the email from Solène (even after
> > requesting it re-sent on lists.openbsd.org; nor is it in spam; I will
> > look further into this issue), so I'm adding my reply in-line here:
>
> Solène's domain publishes a DMARC p=reject record, so sites doing
> strict DMARC checks will reject it when sent via mailing lists,
> unless those lists rewrite the From address.

I don't think that alone necessarily results in a DMARC failure, but
combined with the list server stripping out the original DKIM signature
it definitely does.

Re: [PATCH] Add -executable, -readable, and -writable options to /usr/bin/find

[Stuart Henderson]

On 2023/03/31 19:40, Jared Harper wrote:
> However, I had some difficulty finding the source of access(2) so I could not
> vet by assumptions.

see sys_access() in sys/kern/vfs_syscalls.c

Re: [PATCH] Add -executable, -readable, and -writable options to /usr/bin/find

[Stuart Henderson]

On 2023/04/01 11:27, Jared Harper wrote:
> For some reason I haven't received the email from Solène (even after
> requesting it re-sent on lists.openbsd.org; nor is it in spam; I will
> look further into this issue), so I'm adding my reply in-line here:

Solène's domain publishes a DMARC p=reject record, so sites doing
strict DMARC checks will reject it when sent via mailing lists,
unless those lists rewrite the From address.

> I suppose then that this point of confusion (the ambiguous nature of
> what -executable, et al, actually does) is a good reason not to continue
> on this patch.

fwiw I didn't find it ambiguous, and it seems like it could be useful,
although I don't think it's something I've ever tried to do myself.

We have one patch in ports for -executable (in munin; the patch makes it
use GNU find instead).

Re: [PATCH] Add -executable, -readable, and -writable options to /usr/bin/find

[Steffen Nurpmeso]

Jared Harper wrote in
 :
 |On Sat, Apr 1, 2023 at 12:06 AM Theo Buehler  wrote:
 ...
 |I'm not sure these are equivalent. My (limited?) understanding is these
 |examples are checking whether the file's owner has the specified
 |permissions.
 |
 |The intention for my patch is to return true if the caller of find has
 |the specified permissions.

Ya it rather equals test(1)'s -r, -w, -x in that spirit.
"access(2) is dead, do not use it" comes to mind.


Btw and off-topic, i just converted a program to use pledge and
unveil, and i think the combination is a really, really great
thing.  (Aand especially in conjunction with that special syslog
system call.)  For programmers to use, futhermore.  (What is
missing is drawing the lottery prize, unfortunately.)

--steffen
|
|Der Kragenbaer,The moon bear,
|der holt sich munter   he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)

Re: [PATCH] Add -executable, -readable, and -writable options to /usr/bin/find

[Jared Harper]

On Sat, Apr 1, 2023 at 12:06 AM Theo Buehler  wrote:
>
> While I agree with Solène that we don't necessarily want or need this
> patch, I think it is nicely done. I myself don't find the -perm primary
> very intuitive and I find its manual hard to decipher, so your patch
> would save me a few minutes time every so often.

Understood. It makes sense that OpenBSD does not have the special cases
that Linux does so the call to access(2) adds additional overhead
without covering any more cases.

> There are a few things that go against style(9), most obviously the
> 4-space tabs (also in the head -c patch) as well as using a function
> call for initialization. The whitespace issue also means that the
> patches won't apply to the source tree.
>
> See sys_access() in sys/kern/vfs_syscalls.c
>
> We try to add features only if there is a good reason to do so. For
> non-POSIX options to standard utilities the bar is quite high. You might
> have more luck with getting patches reviewed and accepted if you try to
> find bugs and fix them.

I'm not sure why I haven't seen style(9) before. Obviously this is an
oversight on my part. Thank you for the pointers.

For some reason I haven't received the email from Solène (even after
requesting it re-sent on lists.openbsd.org; nor is it in spam; I will
look further into this issue), so I'm adding my reply in-line here:

> this doesn't add much value IMO, we already have -perm that can be used
> to return paths matching the permission, or only a bit.
>
> find . -executable can be written find . -type f -perm -100
> find . -writable can   be written find . -type f -perm -200
> find . -readable can   be written find . -type f -perm -400

I'm not sure these are equivalent. My (limited?) understanding is these
examples are checking whether the file's owner has the specified
permissions.

The intention for my patch is to return true if the caller of find has
the specified permissions.

Here is a contrived example that showcases the difference:

home$ touch a.txt && chmod 744 a.txt && whoami
jared

Using -perm, the www user finds a.txt because it's executable by the
file owner:

home$ doas -u www find . -type f -perm -100
./a.txt

Using -executable, the www user finds nothing because it is not
executable by the caller.

home$ doas -u www /usr/src/usr.bin/find/find . -type f -executable

Using -executable, the jared user find a.txt because it is executable by
the caller.

home$ /usr/src/usr.bin/find/find . -type f -executable
./a.txt

Based on my reading of -perm, this functionality is not possible.

I suppose then that this point of confusion (the ambiguous nature of
what -executable, et al, actually does) is a good reason not to continue
on this patch.

> on linux, those flags make sense to have because they also take care of
> ACLs, while their -perm doesn't. OpenBSD doesn't have ACLs.

Thanks for the explanation.

Re: [PATCH] Add -executable, -readable, and -writable options to /usr/bin/find

[Theo Buehler]

On Fri, Mar 31, 2023 at 07:40:45PM -0700, Jared Harper wrote:
> I have put together a patch that adds -executable, -readable, and
> -writable to /usr/bin/find.

While I agree with Solène that we don't necessarily want or need this
patch, I think it is nicely done. I myself don't find the -perm primary
very intuitive and I find its manual hard to decipher, so your patch
would save me a few minutes time every so often.

There are a few things that go against style(9), most obviously the
4-space tabs (also in the head -c patch) as well as using a function
call for initialization. The whitespace issue also means that the
patches won't apply to the source tree.

> However, I had some difficulty finding the source of access(2) so I could not
> vet by assumptions.

See sys_access() in sys/kern/vfs_syscalls.c

> Any guidance is greatly appreciated. I have been finding small-footprint
> patches like this one to become familiar with OpenBSD.

We try to add features only if there is a good reason to do so. For
non-POSIX options to standard utilities the bar is quite high. You might
have more luck with getting patches reviewed and accepted if you try to
find bugs and fix them.

Re: [PATCH] Add -executable, -readable, and -writable options to /usr/bin/find

[Solène Rapenne]

Le Fri, 31 Mar 2023 19:40:45 -0700,
Jared Harper  a écrit :

> I have put together a patch that adds -executable, -readable, and
> -writable to /usr/bin/find.
> 
> When I first started working on this patch, I implemented the access
> check by checking the stat of the file like so:
> 

this doesn't add much value IMO, we already have -perm that can be used
to return paths matching the permission, or only a bit.

find . -executable can be written find . -type f -perm -100
find . -writable can   be written find . -type f -perm -200
find . -readable can   be written find . -type f -perm -400

on linux, those flags make sense to have because they also take care of
ACLs, while their -perm doesn't. OpenBSD doesn't have ACLs.

[PATCH] Add -executable, -readable, and -writable options to /usr/bin/find

[Jared Harper]

I have put together a patch that adds -executable, -readable, and
-writable to /usr/bin/find.

When I first started working on this patch, I implemented the access
check by checking the stat of the file like so:

int
is_permission(const FTSENT *entry, mode_t umode, mode_t gmode,
  mode_t omode)
{
struct stat *stat = entry->fts_statp;

if (getuid() == stat->st_uid && (stat->st_mode & umode) != 0)
return 1;
if (getgid() == stat->st_gid && (stat->st_mode & gmode) != 0)
return 1;
if ((stat->st_mode & omode) != 0)
return 1;

return 0;
}

int is_readable(const FTSENT *entry)
{
return is_permission(entry, S_IRUSR, S_IRGRP, S_IROTH);
}

int is_writable(const FTSENT *entry)
{
return is_permission(entry, S_IWUSR, S_IWGRP, S_IWOTH);
}

int is_executable(const FTSENT *entry)
{
return is_permission(entry, S_IXUSR, S_IXGRP, S_IXOTH);
}

This appears to work just fine. However, when I read the Linux manpage
for find, they use access(2):

This test makes use of the access(2) system call, and so can be
fooled by NFS servers which do UID mapping (or root-squashing),
since many systems implement access(2) in the client's kernel and so
cannot make use of the UID mapping information held on the server.
Because this test is based only on the result of the access(2)
system call, there is no guarantee that a file for which this test
succeeds can actually be executed.

Because I'm unfamiliar with OpenBSD I worried that maybe there was more to
access(2) than the checks I created above, I implemented my patch using it.
However, I had some difficulty finding the source of access(2) so I could not
vet by assumptions.

Any guidance is greatly appreciated. I have been finding small-footprint
patches like this one to become familiar with OpenBSD.

Index: extern.h
===
RCS file: /cvs/src/usr.bin/find/extern.h,v
retrieving revision 1.22
diff -u -p -u -p -r1.22 extern.h
--- extern.h3 Jan 2017 21:31:16 -1.22
+++ extern.h1 Apr 2023 02:15:15 -
@@ -58,6 +58,7 @@ PLAN*c_depth(char *, char ***, int);
 PLAN*c_empty(char *, char ***, int);
 PLAN*c_exec(char *, char ***, int);
 PLAN*c_execdir(char *, char ***, int);
+PLAN*c_executable(char *, char ***, int);
 PLAN*c_flags(char *, char ***, int);
 PLAN*c_follow(char *, char ***, int);
 PLAN*c_fstype(char *, char ***, int);
@@ -78,9 +79,11 @@ PLAN*c_perm(char *, char ***, int);
 PLAN*c_print(char *, char ***, int);
 PLAN*c_print0(char *, char ***, int);
 PLAN*c_prune(char *, char ***, int);
+PLAN*c_readable(char *, char ***, int);
 PLAN*c_size(char *, char ***, int);
 PLAN*c_type(char *, char ***, int);
 PLAN*c_user(char *, char ***, int);
+PLAN*c_writable(char *, char ***, int);
 PLAN*c_xdev(char *, char ***, int);
 PLAN*c_openparen(char *, char ***, int);
 PLAN*c_closeparen(char *, char ***, int);
@@ -88,5 +91,5 @@ PLAN*c_mtime(char *, char ***, int);
 PLAN*c_not(char *, char ***, int);
 PLAN*c_or(char *, char ***, int);

-extern int ftsoptions, isdelete, isdepth, isoutput, isxargs;
+extern int ftsoptions, isdelete, isdepth, isoutput, isxargs, accessmode;
 extern int mayexecve;
Index: find.1
===
RCS file: /cvs/src/usr.bin/find/find.1,v
retrieving revision 1.101
diff -u -p -u -p -r1.101 find.1
--- find.1    31 Mar 2022 17:27:24 -1.101
+++ find.11 Apr 2023 02:15:15 -
@@ -264,6 +264,9 @@ The filename substituted for the string
 .Qq {}
 is not qualified.
 .Pp
+.It Fl executable
+True if the file is executable by the user running the command.
+.Pp
 .It Xo
 .Ic -flags
 .Oo - Oc Ns Ar flags
@@ -450,6 +453,9 @@ primary has no effect if the
 .Fl d
 option was specified.
 .Pp
+.It Fl readable
+True if the file is readable by the user running the command.
+.Pp
 .It Ic -size Ar n Ns Op Cm c
 True if the file's size, rounded up, in 512-byte blocks is
 .Ar n .
@@ -491,6 +497,9 @@ If
 is numeric and there is no such user name, then
 .Ar uname
 is treated as a user ID.
+.Pp
+.It Fl writable
+True if the file is writable by the user running the command.
 .Pp
 .It Ic -xdev
 This primary always evaluates to true.
Index: find.h
===
RCS file: /cvs/src/usr.bin/find/find.h,v
retrieving revision 1.18
diff -u -p -u -p -r1.18 find.h
--- find.h3 Jan 2017 21:31:16 -1.18
+++ find.h1 Apr 2023 02:15:15 -
@@ -37,12 +37,12 @@
 enum ntype {
 N_AND = 1, /* must start > 0 */
 N_AMIN, N_ANEWER, N_ATIME, N_CLOSEPAREN, N_CMIN, N_CNEWER, N_CTIME,
-N_DELETE, N_DEPTH, N_EMPTY, N_EXEC, N_EXECDIR, N_EXPR,
+N_DELETE, N_DEPTH,

Error on incorrect find -type argument.

[tlevine]

Dear colleagues,

find's -type flag considers only the first character of the next
argument. These commands consequently print regular file paths (only).

  find . -type folder
  find . -type f,d
  find . -type flødebølle

I propose that they produce an error instead.

I also noted, there is no src/regress/usr.bin/find. Is this on purpose?
If not, I shall propose some.

With great honor,
Tom



Index: usr.bin/find/function.c
===
RCS file: /cvs/src/usr.bin/find/function.c,v
retrieving revision 1.50
diff -u -p -r1.50 function.c
--- usr.bin/find/function.c 23 Nov 2020 06:21:52 -  1.50
+++ usr.bin/find/function.c 13 Oct 2022 06:04:41 -
@@ -1508,6 +1508,9 @@ c_type(char *typestring, char ***ignored
 
ftsoptions &= ~FTS_NOSTAT;
 
+   if (strlen(typestring)>1)
+   errx(1, "-type: %s: type must be one letter", typestring);
+
switch (typestring[0]) {
case 'b':
mraise an eask = S_IFBLK;

/usr.bin/mg patch: Make find-file completion case-insensitive

[Sina Samavati]

Hello,
This is the first time I am sending a patch to the OpenBSD project,
so please let me know if I should read any docs for future patches.

I ditched GNU Emacs in favour of Mg, but I needed certain changes on Mg for 
personal use.
Below is a patch I have been using for several days; it makes makes find-file 
completion case-insensitive.
Please feel free to reject, suggest and/or apply changes if necessary.

Thank you,
Sina

--- a/echo.c
+++ b/echo.c
@@ -571,13 +571,13 @@ complt(int flags, int c, char *buf, size_t nbuf, int 
cpos, int *nx)
nxtra = HUGE;

for (; lh != NULL; lh = lh->l_next) {
-   if (memcmp(buf, lh->l_name, cpos) != 0)
+   if (strncasecmp(buf, lh->l_name, cpos) != 0)
continue;
if (nhits == 0)
lh2 = lh;
++nhits;
if (lh->l_name[cpos] == '\0')
-   nxtra = -1; /* exact match */
+   nxtra = 0; /* exact match */
else {
bxtra = getxtra(lh, lh2, cpos, wflag);
if (bxtra < nxtra)
@@ -594,12 +594,15 @@ complt(int flags, int c, char *buf, size_t nbuf, int 
cpos, int *nx)
 * Being lazy - ought to check length, but all things
 * autocompleted have known types/lengths.
 */
-   if (nxtra < 0 && nhits > 1 && c == ' ')
-   nxtra = 1; /* ??? */
-   for (i = 0; i < nxtra && cpos < nbuf; ++i) {
-   buf[cpos] = lh2->l_name[cpos];
-   eputc(buf[cpos++]);
+   for (i = 0; i < cpos + nxtra && cpos + nxtra < nbuf; i++) {
+   buf[i] = lh2->l_name[i];
+   if (i < cpos) {
+   ttputc('\b');
+   ttcol--;
+   }
}
+   buf[i] = '\0';
+   eputs(buf);
/* XXX should grow nbuf */
ttflush();
free_file_list(wholelist);

--- a/fileio.c
+++ b/fileio.c
@@ -516,7 +517,7 @@ make_file_list(char *buf)

while ((dent = readdir(dirp)) != NULL) {
int isdir;
-   if (strncmp(cp, dent->d_name, len) != 0)
+   if (strncasecmp(cp, dent->d_name, len) != 0)
continue;
isdir = 0;
if (dent->d_type == DT_DIR) {

Re: find -exec util {} arg + confusion

[Todd C . Miller]

On Sat, 21 Nov 2020 17:02:05 +0100, Alexander Hall wrote:

> So this is it. Any other objections? OK?

OK millert@

 - todd

Re: find -exec util {} arg + confusion

[Alexander Hall]

On Tue, Nov 17, 2020 at 01:11:42PM +0100, Paul de Weerd wrote:
> On Tue, Nov 17, 2020 at 01:06:05AM +0100, Alexander Hall wrote:

...

> | The more I read and think about it, I feel the original error message is
> | actually correct in that there is no terminating ";" or "+", since the
> | required condition for it is not fullfilled...
> 
> I still think the error is confusing for the user who, familiar with
> 'find -exec command {} arg \;', might assume the same would work for +.
> Now, your diff seems like a better approach, given your argument.

So this is it. Any other objections? OK?

/Alexander


Index: function.c
===
RCS file: /cvs/src/usr.bin/find/function.c,v
retrieving revision 1.49
diff -u -p -r1.49 function.c
--- function.c  9 Apr 2020 15:07:49 -   1.49
+++ function.c  21 Nov 2020 15:58:40 -
@@ -564,7 +564,7 @@ c_exec(char *unused, char ***argvp, int 
 */
for (ap = argv = *argvp, brace = 0;; ++ap) {
if (!*ap)
-   errx(1, "%s: no terminating \";\" or \"+\"",
+   errx(1, "%s: no terminating \";\" or \"{} +\"",
isok ? "-ok" : "-exec");
lastbrace = brace;
brace = 0;

Re: find -exec util {} arg + confusion

[Andreas Kusalananda Kähäri]

On Mon, Nov 16, 2020 at 09:15:11AM +0100, Paul de Weerd wrote:
> Hi Andreas,
> 
> On Mon, Nov 16, 2020 at 08:53:36AM +0100, Andreas Kusalananda Kähäri wrote:
> | On Thu, Nov 12, 2020 at 08:51:22PM +0100, Paul de Weerd wrote:
> | > Hi all,
> | > 
> | > I misread find(1) and did:
> | > 
> | > [weerdpom] $ find path/to/cam -name \*.JPG -exec cp {} path/to/store +
> | > find: -exec no terminating ";" or "+"
> | 
> | Not really what you're asking for, but...
> | 
> | What you seem to want to do can be done with
> | 
> | find path/to/cam -name '*.JPG' -exec sh -c 'cp "$@" path/to/store' sh 
> {} +
> 
> Thanks, I've solved this in the past with small scripts in my homedir
> or going to `find | xargs -J`.  I'll add your suggestion to my list.
> 
> | Or, with GNU coreutils installed,
> | 
> | find path/to/cam -name '*.JPG' -exec gcp -t path/to/store {} +
> 
> Ugh, installing GNU stuff for something like this... :)  Besides, the
> problem is more generic than just cp(1).  Do all GNU tools that (by
> default) have the target argument as the last argument support -t?  I
> mean, I know cp, mv and ln do, but do they all?
> 

That was just given as a suggestion for *if* you happened to have
coreutils installed.  Personally, I would opt for the "-exec sh -c"
variant as it's portable to any sh shell and find/cp implementation (I'm
often working on several different Unices).

The coreutils' info documentation says that the non-standard convenience
option -t (or --target-directory) is implemented for cp, install, ln,
and mv.

-- 
Andreas (Kusalananda) Kähäri
SciLifeLab, NBIS, ICM
Uppsala University, Sweden

.

Re: find -exec util {} arg + confusion

[Andreas Kusalananda Kähäri]

On Thu, Nov 19, 2020 at 03:26:28PM -0800, Jordan Geoghegan wrote:
> 
> 
> On 11/16/20 12:15 AM, Paul de Weerd wrote:
> > Hi Andreas,
> > 
> > On Mon, Nov 16, 2020 at 08:53:36AM +0100, Andreas Kusalananda Kähäri wrote:
> > | On Thu, Nov 12, 2020 at 08:51:22PM +0100, Paul de Weerd wrote:
> > | > Hi all,
> > | >
> > | > I misread find(1) and did:
> > | >
> > | > [weerdpom] $ find path/to/cam -name \*.JPG -exec cp {} path/to/store +
> > | > find: -exec no terminating ";" or "+"
> > |
> > | Not really what you're asking for, but...
> > |
> > | What you seem to want to do can be done with
> > |
> > |   find path/to/cam -name '*.JPG' -exec sh -c 'cp "$@" path/to/store' sh 
> > {} +
> > 
> > Thanks, I've solved this in the past with small scripts in my homedir
> > or going to `find | xargs -J`.  I'll add your suggestion to my list.
> > 
> > | Or, with GNU coreutils installed,
> > |
> > |   find path/to/cam -name '*.JPG' -exec gcp -t path/to/store {} +
> > 
> > Ugh, installing GNU stuff for something like this... :)  Besides, the
> > problem is more generic than just cp(1).  Do all GNU tools that (by
> > default) have the target argument as the last argument support -t?  I
> > mean, I know cp, mv and ln do, but do they all?
> > 
> > | I quite like Alexander's proposed smaller diff.
> > 
> > Making it more clear to the user that + must follow {} is the thing
> > I'd like to achieve, so yeah :)  No strong feelings over his vs my
> > diff.
> > 
> > Cheers,
> > 
> > Paul
> > 
> 
> Hi Paul,
> 
> I've stumbled on this issue a number of times myself. The easiest workaround
> I've found is to do something like this:
> 
> find /tmp/1 -type f -name "*" -print0 | xargs -0 -I{} cp {} /tmp/2/
> 
> No GNU stuff needed.

This would execute cp once for each found name rather than for batches
of found names.  Also, your -name test is a no-op as it matches every
name.


-- 
Andreas (Kusalananda) Kähäri
SciLifeLab, NBIS, ICM
Uppsala University, Sweden

.

Re: find -exec util {} arg + confusion

[Jordan Geoghegan]

On 11/16/20 12:15 AM, Paul de Weerd wrote:

Hi Andreas,

On Mon, Nov 16, 2020 at 08:53:36AM +0100, Andreas Kusalananda Kähäri wrote:
| On Thu, Nov 12, 2020 at 08:51:22PM +0100, Paul de Weerd wrote:
| > Hi all,
| >
| > I misread find(1) and did:
| >
| > [weerdpom] $ find path/to/cam -name \*.JPG -exec cp {} path/to/store +
| > find: -exec no terminating ";" or "+"
|
| Not really what you're asking for, but...
|
| What you seem to want to do can be done with
|
|   find path/to/cam -name '*.JPG' -exec sh -c 'cp "$@" path/to/store' sh 
{} +

Thanks, I've solved this in the past with small scripts in my homedir
or going to `find | xargs -J`.  I'll add your suggestion to my list.

| Or, with GNU coreutils installed,
|
|   find path/to/cam -name '*.JPG' -exec gcp -t path/to/store {} +

Ugh, installing GNU stuff for something like this... :)  Besides, the
problem is more generic than just cp(1).  Do all GNU tools that (by
default) have the target argument as the last argument support -t?  I
mean, I know cp, mv and ln do, but do they all?

| I quite like Alexander's proposed smaller diff.

Making it more clear to the user that + must follow {} is the thing
I'd like to achieve, so yeah :)  No strong feelings over his vs my
diff.

Cheers,

Paul



Hi Paul,

I've stumbled on this issue a number of times myself. The easiest 
workaround I've found is to do something like this:


find /tmp/1 -type f -name "*" -print0 | xargs -0 -I{} cp {} /tmp/2/

No GNU stuff needed.

Regards,

Jordan

Re: find -exec util {} arg + confusion

[Alexander Hall]

On November 17, 2020 1:11:42 PM GMT+01:00, Paul de Weerd  
wrote:
>On Tue, Nov 17, 2020 at 01:06:05AM +0100, Alexander Hall wrote:
>| On Mon, Nov 16, 2020 at 09:04:53AM +0100, Paul de Weerd wrote:
>| > Hi Alexander,
>| > 
>| > On Sun, Nov 15, 2020 at 10:22:32PM +0100, Alexander Hall wrote:
>| > | I googled for "POSIX find", and hit this:
>| > | 
>| > |
>https://pubs.opengroup.org/onlinepubs/009695399/utilities/find.html
>| > | 
>| > | => "Only a plus sign that follows an argument containing the two
>| > | characters "{}" shall punctuate the end of the primary
>expression.
>| > | Other uses of the plus sign shall not be treated as special."
>| > 
>| > Yep, I also found that when looking into this.  It's unforunate, as
>it
>| > implies you can't use `-exec {} ... +` with e.g. ln, mv or cp, but
>oh
>| > well.
>| > 
>| > (also, nitpicking, 'an argument containing the two characters "{}"'
>| > includes an argument like "hh}hh{hh", which I'm pretty sure is not
>| > what they mean)
>| > 
>| > | What you do in your diff is exactly that, treating it special.
>| > 
>| > I'm not sure I agree.  I make sure I do not treat it special unless
>|^^
>| > it's at the end of the argument to 'exec'.  Can you elaborate on
>what
>|   ^
>| > you mean here?
>| 
>| If it is not following {}, then it should not be treated as such. You
>| are assuming (or guessing) it was meant to be that special '+'.
>| 
>| Carefully crafted example of failing quoting of ';':
>| 
>| $ obj/find . -exec echo + ;# Just print a '+' for every entry
>| find: -exec: "+" should follow {}
>
>In this case, I would say the error is correct: the + *is* at the end
>of the argument to exec, and for -exec to work with +, it should
>follow {}.
>
>Since you failed to escape the semi-colon, the shell (not find)
>discards it (uses it to separate find from the next command), so find
>never sees the ';', you're actually doing `find . -exec echo +`; the
>plus doesn't follow the requisite {}.

1. The failed escaping of ";" was intentional, to hint that the intent really 
was not to use "+" as the delimiter.
2. A "+" not following a "{}" is by itself no indication that it was intended 
to end the -exec part. You are making assumptions, which in the case above 
would be wrong.

Thus, the only thing we can know for sure is that there just simply is no 
terminating ";" or "+".

I'd be fine with changing the "+" to "{} +" though. I did try separating them 
into "{}" "+" to not indicate they should be one single argument, but I still 
believe the former is easier on the eye and gives a good enough hint.

>
>I could change the diff to see if there is no {} at all (as in your
>carefully crafted example), and change the output based on that again.

Not sure exactly what you mean, but I think you'd just be assuming again.

>However, using the diff that you proposed earlier in this thread (that
>results in 'no terminating ";" or "{} +"') is probably the best way
>forward (less code, still clear what the issue is).

Yeah. I strongly agree.

/Alexander


>
>| The more I read and think about it, I feel the original error message
>is
>| actually correct in that there is no terminating ";" or "+", since
>the
>| required condition for it is not fullfilled...
>
>I still think the error is confusing for the user who, familiar with
>'find -exec command {} arg \;', might assume the same would work for +.
>Now, your diff seems like a better approach, given your argument.
>
>Paul

Re: find -exec util {} arg + confusion

[Paul de Weerd]

On Tue, Nov 17, 2020 at 01:06:05AM +0100, Alexander Hall wrote:
| On Mon, Nov 16, 2020 at 09:04:53AM +0100, Paul de Weerd wrote:
| > Hi Alexander,
| > 
| > On Sun, Nov 15, 2020 at 10:22:32PM +0100, Alexander Hall wrote:
| > | I googled for "POSIX find", and hit this:
| > | 
| > | https://pubs.opengroup.org/onlinepubs/009695399/utilities/find.html
| > | 
| > | => "Only a plus sign that follows an argument containing the two
| > | characters "{}" shall punctuate the end of the primary expression.
| > | Other uses of the plus sign shall not be treated as special."
| > 
| > Yep, I also found that when looking into this.  It's unforunate, as it
| > implies you can't use `-exec {} ... +` with e.g. ln, mv or cp, but oh
| > well.
| > 
| > (also, nitpicking, 'an argument containing the two characters "{}"'
| > includes an argument like "hh}hh{hh", which I'm pretty sure is not
| > what they mean)
| > 
| > | What you do in your diff is exactly that, treating it special.
| > 
| > I'm not sure I agree.  I make sure I do not treat it special unless
|^^
| > it's at the end of the argument to 'exec'.  Can you elaborate on what
|   ^
| > you mean here?
| 
| If it is not following {}, then it should not be treated as such. You
| are assuming (or guessing) it was meant to be that special '+'.
| 
| Carefully crafted example of failing quoting of ';':
| 
| $ obj/find . -exec echo + ;# Just print a '+' for every entry
| find: -exec: "+" should follow {}

In this case, I would say the error is correct: the + *is* at the end
of the argument to exec, and for -exec to work with +, it should
follow {}.

Since you failed to escape the semi-colon, the shell (not find)
discards it (uses it to separate find from the next command), so find
never sees the ';', you're actually doing `find . -exec echo +`; the
plus doesn't follow the requisite {}.

I could change the diff to see if there is no {} at all (as in your
carefully crafted example), and change the output based on that again.
However, using the diff that you proposed earlier in this thread (that
results in 'no terminating ";" or "{} +"') is probably the best way
forward (less code, still clear what the issue is).

| The more I read and think about it, I feel the original error message is
| actually correct in that there is no terminating ";" or "+", since the
| required condition for it is not fullfilled...

I still think the error is confusing for the user who, familiar with
'find -exec command {} arg \;', might assume the same would work for +.
Now, your diff seems like a better approach, given your argument.

Paul

-- 
>[<++>-]<+++.>+++[<-->-]<.>+++[<+
+++>-]<.>++[<>-]<+.--.[-]
 http://www.weirdnet.nl/ 

Re: find -exec util {} arg + confusion

[Alexander Hall]

On Mon, Nov 16, 2020 at 09:04:53AM +0100, Paul de Weerd wrote:
> Hi Alexander,
> 
> On Sun, Nov 15, 2020 at 10:22:32PM +0100, Alexander Hall wrote:
> | I googled for "POSIX find", and hit this:
> | 
> | https://pubs.opengroup.org/onlinepubs/009695399/utilities/find.html
> | 
> | => "Only a plus sign that follows an argument containing the two
> | characters "{}" shall punctuate the end of the primary expression.
> | Other uses of the plus sign shall not be treated as special."
> 
> Yep, I also found that when looking into this.  It's unforunate, as it
> implies you can't use `-exec {} ... +` with e.g. ln, mv or cp, but oh
> well.
> 
> (also, nitpicking, 'an argument containing the two characters "{}"'
> includes an argument like "hh}hh{hh", which I'm pretty sure is not
> what they mean)
> 
> | What you do in your diff is exactly that, treating it special.
> 
> I'm not sure I agree.  I make sure I do not treat it special unless
   ^^
> it's at the end of the argument to 'exec'.  Can you elaborate on what
  ^
> you mean here?

If it is not following {}, then it should not be treated as such. You
are assuming (or guessing) it was meant to be that special '+'.

Carefully crafted example of failing quoting of ';':

$ obj/find . -exec echo + ;# Just print a '+' for every entry
find: -exec: "+" should follow {}

The more I read and think about it, I feel the original error message is
actually correct in that there is no terminating ";" or "+", since the
required condition for it is not fullfilled...

/Alexander

> 
> Thanks!
> 
> Paul
> 
> -- 
> >[<++>-]<+++.>+++[<-->-]<.>+++[<+
> +++>-]<.>++[<>-]<+.--.[-]
>  http://www.weirdnet.nl/ 
> 

Re: find -exec util {} arg + confusion

[Paul de Weerd]

Hi Andreas,

On Mon, Nov 16, 2020 at 08:53:36AM +0100, Andreas Kusalananda Kähäri wrote:
| On Thu, Nov 12, 2020 at 08:51:22PM +0100, Paul de Weerd wrote:
| > Hi all,
| > 
| > I misread find(1) and did:
| > 
| > [weerdpom] $ find path/to/cam -name \*.JPG -exec cp {} path/to/store +
| > find: -exec no terminating ";" or "+"
| 
| Not really what you're asking for, but...
| 
| What you seem to want to do can be done with
| 
|   find path/to/cam -name '*.JPG' -exec sh -c 'cp "$@" path/to/store' sh 
{} +

Thanks, I've solved this in the past with small scripts in my homedir
or going to `find | xargs -J`.  I'll add your suggestion to my list.

| Or, with GNU coreutils installed,
| 
|   find path/to/cam -name '*.JPG' -exec gcp -t path/to/store {} +

Ugh, installing GNU stuff for something like this... :)  Besides, the
problem is more generic than just cp(1).  Do all GNU tools that (by
default) have the target argument as the last argument support -t?  I
mean, I know cp, mv and ln do, but do they all?

| I quite like Alexander's proposed smaller diff.

Making it more clear to the user that + must follow {} is the thing
I'd like to achieve, so yeah :)  No strong feelings over his vs my
diff.

Cheers,

Paul

-- 
>[<++>-]<+++.>+++[<-->-]<.>+++[<+
+++>-]<.>++[<>-]<+.--.[-]
 http://www.weirdnet.nl/ 

Re: find -exec util {} arg + confusion

[Paul de Weerd]

Hi Alexander,

On Sun, Nov 15, 2020 at 10:22:32PM +0100, Alexander Hall wrote:
| I googled for "POSIX find", and hit this:
| 
| https://pubs.opengroup.org/onlinepubs/009695399/utilities/find.html
| 
| => "Only a plus sign that follows an argument containing the two
| characters "{}" shall punctuate the end of the primary expression.
| Other uses of the plus sign shall not be treated as special."

Yep, I also found that when looking into this.  It's unforunate, as it
implies you can't use `-exec {} ... +` with e.g. ln, mv or cp, but oh
well.

(also, nitpicking, 'an argument containing the two characters "{}"'
includes an argument like "hh}hh{hh", which I'm pretty sure is not
what they mean)

| What you do in your diff is exactly that, treating it special.

I'm not sure I agree.  I make sure I do not treat it special unless
it's at the end of the argument to 'exec'.  Can you elaborate on what
you mean here?

Thanks!

Paul

-- 
>[<++>-]<+++.>+++[<-->-]<.>+++[<+
+++>-]<.>++[<>-]<+.--.[-]
 http://www.weirdnet.nl/ 

Re: find -exec util {} arg + confusion

[Andreas Kusalananda Kähäri]

On Thu, Nov 12, 2020 at 08:51:22PM +0100, Paul de Weerd wrote:
> Hi all,
> 
> I misread find(1) and did:
> 
> [weerdpom] $ find path/to/cam -name \*.JPG -exec cp {} path/to/store +
> find: -exec no terminating ";" or "+"

Not really what you're asking for, but...

What you seem to want to do can be done with

find path/to/cam -name '*.JPG' -exec sh -c 'cp "$@" path/to/store' sh 
{} +

Or, with GNU coreutils installed,

find path/to/cam -name '*.JPG' -exec gcp -t path/to/store {} +

> 
> That was somewhat surprising - there is a terminating "+".  The error
> really is that the "+" doesn't follow immediately after the "{}"
> (which the manpage of course told me).  Although it would be nice to
> be able to use tools like cp and mv to -exec with +, I suspect there
> to be dragons.  So I'm proposing to change the error to point this out
> to the unsuspecting user.
> 
> Cheers,
> 
> Paul 'WEiRD' de Weerd
> 
> Index: function.c
[cut]


I quite like Alexander's proposed smaller diff.

-- 
Andreas (Kusalananda) Kähäri
SciLifeLab, NBIS, ICM
Uppsala University, Sweden

.

Re: find -exec util {} arg + confusion

[Alexander Hall]

On Sun, Nov 15, 2020 at 07:19:07PM +0100, Paul de Weerd wrote:
> Hi all,
> 
> It was pointed out to me off-list that I introduced a regression for
> the case that has '+' as one of its arguments, e.g.:
> 
>   [weerd@pom] $ find /var/empty -exec echo + {} +
>   find: -exec: "+" should follow {}
> 
> Updated diff fixes that case:
> 
>   [weerd@pom] $ ./find /var/empty -exec echo cp {} /tmp/dest +
>   find: -exec: "+" should follow {}
>   [weerd@pom] $ ./find /var/empty -exec echo + {} +
>   + /var/empty
> 
> Any thoughts or concerns?  Other regressions?
> 
> Thanks,
> 
> Paul

I googled for "POSIX find", and hit this:

https://pubs.opengroup.org/onlinepubs/009695399/utilities/find.html

=> "Only a plus sign that follows an argument containing the two
characters "{}" shall punctuate the end of the primary expression.
Other uses of the plus sign shall not be treated as special."

What you do in your diff is exactly that, treating it special.

The manual page could maybe be more exact, but otherwise sth like
this could be enough (pardon the git diff, but you get the point).

/Alexander

diff --git a/usr.bin/find/function.c b/usr.bin/find/function.c
index 3d7f4963b1f..727f8dce43f 100644
--- a/usr.bin/find/function.c
+++ b/usr.bin/find/function.c
@@ -564,7 +564,7 @@ c_exec(char *unused, char ***argvp, int isok)
 */
for (ap = argv = *argvp, brace = 0;; ++ap) {
if (!*ap)
-   errx(1, "%s: no terminating \";\" or \"+\"",
+   errx(1, "%s: no terminating \";\" or \"{} +\"",
isok ? "-ok" : "-exec");
lastbrace = brace;
brace = 0;

Re: find -exec util {} arg + confusion

[Paul de Weerd]

Hi all,

It was pointed out to me off-list that I introduced a regression for
the case that has '+' as one of its arguments, e.g.:

[weerd@pom] $ find /var/empty -exec echo + {} +
find: -exec: "+" should follow {}

Updated diff fixes that case:

[weerd@pom] $ ./find /var/empty -exec echo cp {} /tmp/dest +
    find: -exec: "+" should follow {}
[weerd@pom] $ ./find /var/empty -exec echo + {} +
+ /var/empty

Any thoughts or concerns?  Other regressions?

Thanks,

Paul

Index: function.c
===
RCS file: /home/OpenBSD/cvs/src/usr.bin/find/function.c,v
retrieving revision 1.49
diff -u -p -r1.49 function.c
--- function.c  9 Apr 2020 15:07:49 -   1.49
+++ function.c  15 Nov 2020 18:12:04 -
@@ -572,9 +572,14 @@ c_exec(char *unused, char ***argvp, int 
brace = 1;
if (strcmp(*ap, ";") == 0)
break;
-   if (strcmp(*ap, "+") == 0 && lastbrace) {
-   new->flags |= F_PLUSSET;
-   break;
+   if (strcmp(*ap, "+") == 0) {
+   if (lastbrace) {
+   new->flags |= F_PLUSSET;
+   break;
+   } else if (!*(ap+1)) {
+   errx(1, "%s: \"+\" should follow {}",
+  isok ? "-ok" : "-exec");
+   }
}
    }
 


On Thu, Nov 12, 2020 at 08:51:22PM +0100, Paul de Weerd wrote:
| Hi all,
| 
| I misread find(1) and did:
| 
| [weerdpom] $ find path/to/cam -name \*.JPG -exec cp {} path/to/store +
| find: -exec no terminating ";" or "+"
| 
| That was somewhat surprising - there is a terminating "+".  The error
| really is that the "+" doesn't follow immediately after the "{}"
| (which the manpage of course told me).  Although it would be nice to
| be able to use tools like cp and mv to -exec with +, I suspect there
| to be dragons.  So I'm proposing to change the error to point this out
| to the unsuspecting user.
| 
| Cheers,
| 
| Paul 'WEiRD' de Weerd
| 
| Index: function.c
| ===
| RCS file: /home/OpenBSD/cvs/src/usr.bin/find/function.c,v
| retrieving revision 1.49
| diff -u -p -r1.49 function.c
| --- function.c9 Apr 2020 15:07:49 -   1.49
| +++ function.c12 Nov 2020 19:42:49 -
| @@ -572,9 +572,14 @@ c_exec(char *unused, char ***argvp, int 
|   brace = 1;
|   if (strcmp(*ap, ";") == 0)
|   break;
| - if (strcmp(*ap, "+") == 0 && lastbrace) {
| - new->flags |= F_PLUSSET;
| - break;
| + if (strcmp(*ap, "+") == 0) {
| + if (lastbrace) {
| + new->flags |= F_PLUSSET;
| + break;
| + } else {
| + errx(1, "%s: \"+\" should follow {}",
| +isok ? "-ok" : "-exec");
| + }
|   }
|   }
|  
| 
| -- 
| >[<++>-]<+++.>+++[<-->-]<.>+++[<+
| +++>-]<.>++[<>-]<+.--.[-]
|  http://www.weirdnet.nl/ 
| 

-- 
>[<++>-]<+++.>+++[<-->-]<.>+++[<+
+++>-]<.>++[<>-]<+.--.[-]
 http://www.weirdnet.nl/ 

find -exec util {} arg + confusion

[Paul de Weerd]

Hi all,

I misread find(1) and did:

[weerdpom] $ find path/to/cam -name \*.JPG -exec cp {} path/to/store +
find: -exec no terminating ";" or "+"

That was somewhat surprising - there is a terminating "+".  The error
really is that the "+" doesn't follow immediately after the "{}"
(which the manpage of course told me).  Although it would be nice to
be able to use tools like cp and mv to -exec with +, I suspect there
to be dragons.  So I'm proposing to change the error to point this out
to the unsuspecting user.

Cheers,

Paul 'WEiRD' de Weerd

Index: function.c
===
RCS file: /home/OpenBSD/cvs/src/usr.bin/find/function.c,v
retrieving revision 1.49
diff -u -p -r1.49 function.c
--- function.c  9 Apr 2020 15:07:49 -   1.49
+++ function.c  12 Nov 2020 19:42:49 -
@@ -572,9 +572,14 @@ c_exec(char *unused, char ***argvp, int 
brace = 1;
if (strcmp(*ap, ";") == 0)
break;
-   if (strcmp(*ap, "+") == 0 && lastbrace) {
-   new->flags |= F_PLUSSET;
-   break;
+   if (strcmp(*ap, "+") == 0) {
+   if (lastbrace) {
+   new->flags |= F_PLUSSET;
+   break;
+   } else {
+   errx(1, "%s: \"+\" should follow {}",
+  isok ? "-ok" : "-exec");
+   }
}
}
 

-- 
>[<++>-]<+++.>+++[<-->-]<.>+++[<+
+++>-]<.>++[<>-]<+.--.[-]
 http://www.weirdnet.nl/ 

Re: /etc/daily: use find -delete

[Denis Fondras]

On Thu, Oct 08, 2020 at 05:32:15AM -0600, Todd C. Miller wrote:
> We can use find's built-in -delete primary to remove old /tmp files
> and directories.  This is somewhat less error-prone than execing
> rm or rmdir.
> 

OK denis@

>  - todd
> 
> Index: etc/daily
> ===
> RCS file: /cvs/src/etc/daily,v
> retrieving revision 1.93
> diff -u -p -u -r1.93 daily
> --- etc/daily 9 Sep 2019 20:02:26 -   1.93
> +++ etc/daily 22 Aug 2020 01:21:16 -
> @@ -50,17 +50,17 @@ if [ -d /tmp -a ! -L /tmp ]; then
>   find -x . \
>   \( -path './ssh-*' -o -path ./.X11-unix -o -path ./.ICE-unix \
>   -o -path './tmux-*' \) \
> - -prune -o -type f -atime +7 -execdir rm -f -- {} \; 2>/dev/null
> + -prune -o -type f -atime +7 -delete 2>/dev/null
>   find -x . -type d -mtime +1 ! -path ./vi.recover ! -path ./.X11-unix \
>   ! -path ./.ICE-unix ! -name . \
> - -execdir rmdir -- {} \; >/dev/null 2>&1; }
> + -delete >/dev/null 2>&1; }
>  fi
>  
>  # Additional junk directory cleanup would go like this:
>  #if [ -d /scratch -a ! -L /scratch ]; then
>  #cd /scratch && {
> -#find . ! -name . -atime +1 -execdir rm -f -- {} \;
> -#find . ! -name . -type d -mtime +1 -execdir rmdir -- {} \; \
> +#find . ! -name . -atime +1 -delete
> +#find . ! -name . -type d -mtime +1 -delete \
>  #>/dev/null 2>&1; }
>  #fi
>  
> 

/etc/daily: use find -delete

[Todd C . Miller]

We can use find's built-in -delete primary to remove old /tmp files
and directories.  This is somewhat less error-prone than execing
rm or rmdir.

 - todd

Index: etc/daily
===
RCS file: /cvs/src/etc/daily,v
retrieving revision 1.93
diff -u -p -u -r1.93 daily
--- etc/daily   9 Sep 2019 20:02:26 -   1.93
+++ etc/daily   22 Aug 2020 01:21:16 -
@@ -50,17 +50,17 @@ if [ -d /tmp -a ! -L /tmp ]; then
find -x . \
\( -path './ssh-*' -o -path ./.X11-unix -o -path ./.ICE-unix \
-o -path './tmux-*' \) \
-   -prune -o -type f -atime +7 -execdir rm -f -- {} \; 2>/dev/null
+   -prune -o -type f -atime +7 -delete 2>/dev/null
    find -x . -type d -mtime +1 ! -path ./vi.recover ! -path ./.X11-unix \
! -path ./.ICE-unix ! -name . \
-   -execdir rmdir -- {} \; >/dev/null 2>&1; }
+   -delete >/dev/null 2>&1; }
 fi
 
 # Additional junk directory cleanup would go like this:
 #if [ -d /scratch -a ! -L /scratch ]; then
 #  cd /scratch && {
-#  find . ! -name . -atime +1 -execdir rm -f -- {} \;
-#  find . ! -name . -type d -mtime +1 -execdir rmdir -- {} \; \
+#  find . ! -name . -atime +1 -delete
+#  find . ! -name . -type d -mtime +1 -delete \
 #  >/dev/null 2>&1; }
 #fi
 

Re: man find -exec -- a little bit more hand-holding

[Jason McIntyre]

On Fri, Aug 14, 2020 at 09:24:35AM -0600, Theo de Raadt wrote:
> Christian Weisgerber  wrote:
> 
> > On 2020-08-14, Jason McIntyre  wrote:
> > 
> > > - i cannot work out what is with the \! examples. i know we try to make
> > >   entries work for both csh and sh style shells, but stuff like this
> > >   works without escaping:
> > >
> > >   $ find . ! -type f
> > 
> > Going through the CVS and SCCS history, I see that the examples
> > came with a rewrite of find(1) at Berkeley 30 years ago.
> > 
> > csh's behavior that "for convenience, a `!' is passed unchanged
> > when it is followed by a blank, tab, newline, `=' or `('" has been
> > documented as such at least since the start of the CSRG repository
> > in 1985.
> > 
> > bash, whose history substitution was modeled on csh, also shares
> > this behavior.
> > 
> > I think it was never necessary to escape the '!' and the man page
> > examples were written with an abundance of caution and a lack of
> > understanding of csh's exact replacement mechanism.
> 
> Yep, so I think that particular escape should be deleted.
> 

done.
jmc

Re: man find -exec -- a little bit more hand-holding

[Theo de Raadt]

Christian Weisgerber  wrote:

> On 2020-08-14, Jason McIntyre  wrote:
> 
> > - i cannot work out what is with the \! examples. i know we try to make
> >   entries work for both csh and sh style shells, but stuff like this
> >   works without escaping:
> >
> > $ find . ! -type f
> 
> Going through the CVS and SCCS history, I see that the examples
> came with a rewrite of find(1) at Berkeley 30 years ago.
> 
> csh's behavior that "for convenience, a `!' is passed unchanged
> when it is followed by a blank, tab, newline, `=' or `('" has been
> documented as such at least since the start of the CSRG repository
> in 1985.
> 
> bash, whose history substitution was modeled on csh, also shares
> this behavior.
> 
> I think it was never necessary to escape the '!' and the man page
> examples were written with an abundance of caution and a lack of
> understanding of csh's exact replacement mechanism.

Yep, so I think that particular escape should be deleted.

Re: man find -exec -- a little bit more hand-holding

[Christian Weisgerber]

On 2020-08-14, Jason McIntyre  wrote:

> - i cannot work out what is with the \! examples. i know we try to make
>   entries work for both csh and sh style shells, but stuff like this
>   works without escaping:
>
>   $ find . ! -type f

Going through the CVS and SCCS history, I see that the examples
came with a rewrite of find(1) at Berkeley 30 years ago.

csh's behavior that "for convenience, a `!' is passed unchanged
when it is followed by a blank, tab, newline, `=' or `('" has been
documented as such at least since the start of the CSRG repository
in 1985.

bash, whose history substitution was modeled on csh, also shares
this behavior.

I think it was never necessary to escape the '!' and the man page
examples were written with an abundance of caution and a lack of
understanding of csh's exact replacement mechanism.

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de

Re: man find -exec -- a little bit more hand-holding

[Jason McIntyre]

On Fri, Aug 14, 2020 at 04:30:13AM +0100, ropers wrote:
> The find man page might benefit from adding a little bit more
> user-friendly hand-holding here:
> 
> Index: find.1
> ===
> RCS file: /cvs/src/usr.bin/find/find.1,v
> retrieving revision 1.98
> diff -u -r1.98 find.1
> --- find.12 Sep 2019 21:18:41 -   1.98
> +++ find.114 Aug 2020 02:59:48 -
> @@ -231,6 +231,10 @@
>  .Qq {}
>  appears anywhere in the utility name or the
>  arguments it is replaced by the pathname of the current file.
> +The semicolon will likely have to be escaped, depending on the shell.
> +See
> +.Sx CAVEATS
> +below.
>  .Pp
>  If terminated by a plus sign,
>  the pathnames for which the
> 
> 
> EXAMPLE:
> 
> $ find ~ -iname ".*" -exec echo {} \;
> works, but
> $ find ~ -iname ".*" -exec echo {} ;
> does not.
> 
> (Yes, this is a contrived example.  I wouldn't want to make people
> copy things all over the place.)
> 

hi.

first off, i do sympathise ;) shell escaping always hurts my head too.

regarding your diff: i don;t think this is the way to go. really, we
would have to add such a text every time we encounter a character that
may need escaping. so the whole point of the text in CAVEATS in to avoid
such a large text addition, and just remind people to watch out
(caveat!) the text in CAVEATS pretty much makes your diff redundant.

the EXAMPLES section additionally shows escaping.

i think there's enough there for people.

havng said that:

- i had hoped we'd have a specific \; example. that format is so
  commonly seen that it might make sense. so i'd not be against either
  adding such an example (currently EXAMPLES is fairly small) or
  altering another.

- i cannot work out what is with the \! examples. i know we try to make
  entries work for both csh and sh style shells, but stuff like this
  works without escaping:

$ find . ! -type f

jmc

man find -exec -- a little bit more hand-holding

[ropers]

The find man page might benefit from adding a little bit more
user-friendly hand-holding here:

Index: find.1
===
RCS file: /cvs/src/usr.bin/find/find.1,v
retrieving revision 1.98
diff -u -r1.98 find.1
--- find.1  2 Sep 2019 21:18:41 -   1.98
+++ find.1  14 Aug 2020 02:59:48 -
@@ -231,6 +231,10 @@
 .Qq {}
 appears anywhere in the utility name or the
 arguments it is replaced by the pathname of the current file.
+The semicolon will likely have to be escaped, depending on the shell.
+See
+.Sx CAVEATS
+below.
 .Pp
 If terminated by a plus sign,
 the pathnames for which the


EXAMPLE:

$ find ~ -iname ".*" -exec echo {} \;
works, but
$ find ~ -iname ".*" -exec echo {} ;
does not.

(Yes, this is a contrived example.  I wouldn't want to make people
copy things all over the place.)

Re: softraid/bioctl cant find device /dev/bio

[sven falempin]

On Mon, Aug 3, 2020 at 11:38 AM Brian Brombacher 
wrote:

>
>
> > On Aug 3, 2020, at 9:54 AM, sven falempin 
> wrote:
> >
> > Hello
> >
> > I saw a similar issue in the mailing list around decembre 2019,
> > following an electrical problem softraid doesn't bring devices ups
> >
> >
> > # ls /dev/sd??
> > /dev/sd0a /dev/sd0g /dev/sd0m /dev/sd1c /dev/sd1i /dev/sd1o /dev/sd2e
> > /dev/sd2k
> > /dev/sd0b /dev/sd0h /dev/sd0n /dev/sd1d /dev/sd1j /dev/sd1p /dev/sd2f
> > /dev/sd2l
> > /dev/sd0c /dev/sd0i /dev/sd0o /dev/sd1e /dev/sd1k /dev/sd2a /dev/sd2g
> > /dev/sd2m
> > /dev/sd0d /dev/sd0j /dev/sd0p /dev/sd1f /dev/sd1l /dev/sd2b /dev/sd2h
> > /dev/sd2n
> > /dev/sd0e /dev/sd0k /dev/sd1a /dev/sd1g /dev/sd1m /dev/sd2c /dev/sd2i
> > /dev/sd2o
> > /dev/sd0f /dev/sd0l /dev/sd1b /dev/sd1h /dev/sd1n /dev/sd2d /dev/sd2j
> > /dev/sd2p
> > # dmesg | grep 6.7
> > OpenBSD 6.7 (RAMDISK_CD) #177: Thu May  7 11:19:02 MDT 2020
> > # dmesg | grep sd
> >dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/RAMDISK_CD
> > wsdisplay1 at vga1 mux 1: console (80x25, vt100 emulation)
> > sd0 at scsibus1 targ 0 lun 0: 
> > t10.ATA_QEMU_HARDDISK_Q
> > M5_
> > sd0: 1907729MB, 512 bytes/sector, 3907029168 sectors, thin
> > sd1 at scsibus1 targ 1 lun 0: 
> > t10.ATA_QEMU_HARDDISK_Q
> > M7_
> > sd1: 1907729MB, 512 bytes/sector, 3907029168 sectors, thin
> > wskbd0 at pckbd0: console keyboard, using wsdisplay1
> > softraid0: trying to bring up sd2 degraded
> > softraid0: sd2 was not shutdown properly
> > softraid0: sd2 is offline, will not be brought online
> > # bioctl -d sd2
> > bioctl: Can't locate sd2 device via /dev/bio
> > #
> >
> > I suspect a missing devices in /dev ( but it seems i have the required
> one )
> > and MAKEDEV all of course did a `uid 0 on /: out of inodes`
> >
> > I have backups but i ' d like to fix the issue !
>
> Hi Sven,
>
> The device sd2 wasn’t attached by softraid, your /dev/bio is fine.  This
> can happen if softraid fails to find all component disks or the metadata on
> one or more components does not match expectations (newer metadata seen on
> other disks).  Make sure all of the component disks are working.  If that
> is not the issue, you may need to re-run the command that you used to
> create the array and include -C force.  Be very careful doing this, I
> suggest running the command once without -C force to ensure it found all
> the components and fails to bring the array up due to the same error
> message you got (attempt to bring up degraded).
>
> If you’re not careful, you can blow out the whole array.
>
> -Brian
>
>
> The disk looks fine, the disklabel is ok, the array is just sd0 and sda1
both got the disklabel RAID part,
shall i do further checks ?

# bioctl -c 1 -l /dev/sd0a,/dev/sd1a softraid0
softraid0: trying to bring up sd2 degraded
softraid0: sd2 was not shutdown properly
softraid0: sd2 is offline, will not be brought online
softraid0: trying to bring up sd2 degraded
softraid0: sd2 was not shutdown properly
softraid0: sd2 is offline, will not be brought online

I wouldnt like to blow the whole array ! sd0a should be in perfect
condition but unsure about sd1a, i probably need to bioctl -R sd1

-- 
--
-
Knowing is not enough; we must apply. Willing is not enough; we must do

Re: softraid/bioctl cant find device /dev/bio

[Brian Brombacher]

> On Aug 3, 2020, at 9:54 AM, sven falempin  wrote:
> 
> Hello
> 
> I saw a similar issue in the mailing list around decembre 2019,
> following an electrical problem softraid doesn't bring devices ups
> 
> 
> # ls /dev/sd??
> /dev/sd0a /dev/sd0g /dev/sd0m /dev/sd1c /dev/sd1i /dev/sd1o /dev/sd2e
> /dev/sd2k
> /dev/sd0b /dev/sd0h /dev/sd0n /dev/sd1d /dev/sd1j /dev/sd1p /dev/sd2f
> /dev/sd2l
> /dev/sd0c /dev/sd0i /dev/sd0o /dev/sd1e /dev/sd1k /dev/sd2a /dev/sd2g
> /dev/sd2m
> /dev/sd0d /dev/sd0j /dev/sd0p /dev/sd1f /dev/sd1l /dev/sd2b /dev/sd2h
> /dev/sd2n
> /dev/sd0e /dev/sd0k /dev/sd1a /dev/sd1g /dev/sd1m /dev/sd2c /dev/sd2i
> /dev/sd2o
> /dev/sd0f /dev/sd0l /dev/sd1b /dev/sd1h /dev/sd1n /dev/sd2d /dev/sd2j
> /dev/sd2p
> # dmesg | grep 6.7
> OpenBSD 6.7 (RAMDISK_CD) #177: Thu May  7 11:19:02 MDT 2020
> # dmesg | grep sd
>dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/RAMDISK_CD
> wsdisplay1 at vga1 mux 1: console (80x25, vt100 emulation)
> sd0 at scsibus1 targ 0 lun 0: 
> t10.ATA_QEMU_HARDDISK_Q
> M5_
> sd0: 1907729MB, 512 bytes/sector, 3907029168 sectors, thin
> sd1 at scsibus1 targ 1 lun 0: 
> t10.ATA_QEMU_HARDDISK_Q
> M7_
> sd1: 1907729MB, 512 bytes/sector, 3907029168 sectors, thin
> wskbd0 at pckbd0: console keyboard, using wsdisplay1
> softraid0: trying to bring up sd2 degraded
> softraid0: sd2 was not shutdown properly
> softraid0: sd2 is offline, will not be brought online
> # bioctl -d sd2
> bioctl: Can't locate sd2 device via /dev/bio
> #
> 
> I suspect a missing devices in /dev ( but it seems i have the required one )
> and MAKEDEV all of course did a `uid 0 on /: out of inodes`
> 
> I have backups but i ' d like to fix the issue !

Hi Sven,

The device sd2 wasn’t attached by softraid, your /dev/bio is fine.  This can 
happen if softraid fails to find all component disks or the metadata on one or 
more components does not match expectations (newer metadata seen on other 
disks).  Make sure all of the component disks are working.  If that is not the 
issue, you may need to re-run the command that you used to create the array and 
include -C force.  Be very careful doing this, I suggest running the command 
once without -C force to ensure it found all the components and fails to bring 
the array up due to the same error message you got (attempt to bring up 
degraded).

If you’re not careful, you can blow out the whole array.

-Brian


> 
> -- 
> --
> -
> Knowing is not enough; we must apply. Willing is not enough; we must do

softraid/bioctl cant find device /dev/bio

[sven falempin]

Hello

I saw a similar issue in the mailing list around decembre 2019,
following an electrical problem softraid doesn't bring devices ups


# ls /dev/sd??
/dev/sd0a /dev/sd0g /dev/sd0m /dev/sd1c /dev/sd1i /dev/sd1o /dev/sd2e
/dev/sd2k
/dev/sd0b /dev/sd0h /dev/sd0n /dev/sd1d /dev/sd1j /dev/sd1p /dev/sd2f
/dev/sd2l
/dev/sd0c /dev/sd0i /dev/sd0o /dev/sd1e /dev/sd1k /dev/sd2a /dev/sd2g
/dev/sd2m
/dev/sd0d /dev/sd0j /dev/sd0p /dev/sd1f /dev/sd1l /dev/sd2b /dev/sd2h
/dev/sd2n
/dev/sd0e /dev/sd0k /dev/sd1a /dev/sd1g /dev/sd1m /dev/sd2c /dev/sd2i
/dev/sd2o
/dev/sd0f /dev/sd0l /dev/sd1b /dev/sd1h /dev/sd1n /dev/sd2d /dev/sd2j
/dev/sd2p
# dmesg | grep 6.7
OpenBSD 6.7 (RAMDISK_CD) #177: Thu May  7 11:19:02 MDT 2020
# dmesg | grep sd
dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/RAMDISK_CD
wsdisplay1 at vga1 mux 1: console (80x25, vt100 emulation)
sd0 at scsibus1 targ 0 lun 0: 
t10.ATA_QEMU_HARDDISK_Q
M5_
sd0: 1907729MB, 512 bytes/sector, 3907029168 sectors, thin
sd1 at scsibus1 targ 1 lun 0: 
t10.ATA_QEMU_HARDDISK_Q
M7_
sd1: 1907729MB, 512 bytes/sector, 3907029168 sectors, thin
wskbd0 at pckbd0: console keyboard, using wsdisplay1
softraid0: trying to bring up sd2 degraded
softraid0: sd2 was not shutdown properly
softraid0: sd2 is offline, will not be brought online
# bioctl -d sd2
bioctl: Can't locate sd2 device via /dev/bio
#

I suspect a missing devices in /dev ( but it seems i have the required one )
and MAKEDEV all of course did a `uid 0 on /: out of inodes`

I have backups but i ' d like to fix the issue !

-- 
--
-
Knowing is not enough; we must apply. Willing is not enough; we must do

Re: find(1) -exec + and ARG_MAX

[Theo de Raadt]

> Thanks for pointing this out.  As discussed with deraadt@ it's probably
> not a good idea to mimic too closely the algorithm in the kernel.

If the accounting in find is byte-for-byte accurate, it will be fragile.
It may be better if find consistantly undershoots when estimating.

Re: find(1) -exec + and ARG_MAX

[Jeremie Courreges-Anglas]

On Thu, Apr 09 2020, Marc Espie  wrote:
> 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.

If you're thinking about this comment in xargs.c:

/*
 * POSIX.2 limits the exec line length to ARG_MAX - 2K.  Running that
 * caused some E2BIG errors, so it was changed to ARG_MAX - 4K.  Given
 * that the smallest argument is 2 bytes in length, this means that
 * the number of arguments is limited to:
 *
 *   (ARG_MAX - 4K - LENGTH(utility + arguments)) / 2.
 *
 * We arbitrarily limit the number of arguments to 5000.  This is
 * allowed by POSIX.2 as long as the resulting minimum exec line is
 * at least LINE_MAX.  Realloc'ing as necessary is possible, but
 * probably not worthwhile.
 */

I did not drop it since it was not there in the first place. (:

The comment is here since rev 1.1 and doesn't give a rationale for the
4K reserved space, except "it worked when I tested".  The rest of the
comment is about the maximum number of arguments.  I think the comment
should be improved before spreading 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.

Thanks for pointing this out.  As discussed with deraadt@ it's probably
not a good idea to mimic too closely the algorithm in the kernel.
However if MAXINTERP+MAXPATHLEN quirk is the explanation behind the 4K
space reserve, we could turn that knowledge into better code.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: find(1) -exec + and ARG_MAX

[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.

Re: find(1) -exec + and ARG_MAX

[Todd C . Miller]

On Thu, 09 Apr 2020 14:44:14 +0200, Jeremie Courreges-Anglas wrote:

> 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.

Looks good.  OK millert@

 - todd

Re: find(1) -exec + and ARG_MAX

[Jeremie Courreges-Anglas]

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.

ok?


Index: function.c
===
RCS file: /d/cvs/src/usr.bin/find/function.c,v
retrieving revision 1.48
diff -u -p -p -u -r1.48 function.c
--- function.c  9 Apr 2020 10:27:32 -   1.48
+++ function.c  9 Apr 2020 12:36:52 -
@@ -588,6 +588,8 @@ c_exec(char *unused, char ***argvp, int 
 
if (new->flags & F_PLUSSET) {
long arg_max;
+   extern char **environ;
+   char **ep;
u_int c, bufsize;
 
cnt = ap - *argvp - 1;  /* units are words */
@@ -605,7 +607,11 @@ c_exec(char *unused, char ***argvp, int 
 */
arg_max = sysconf(_SC_ARG_MAX);
if (arg_max == -1)
-   err(1, "sysconf(_SC_ARG_MAX) failed");
+   err(1, "-exec: sysconf(_SC_ARG_MAX) failed");
+   for (ep = environ; *ep != NULL; ep++) {
+   /* 1 byte for each '\0' */
+   arg_max -= strlen(*ep) + 1 + sizeof(*ep);
+   }
 
/*
 * Count up the space of the user's arguments, and
@@ -617,6 +623,8 @@ c_exec(char *unused, char ***argvp, int 
c += strlen(*argv) + 1;
new->e_argv[cnt] = *argv;
}
+   if (arg_max < 4 * 1024 + c)
+   errx(1, "-exec: no space left to run child command");
bufsize = arg_max - 4 * 1024 - c;
 
/*

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: find(1) -exec + and ARG_MAX

[Todd C . Miller]

On Thu, 09 Apr 2020 03:47:45 +0200, Jeremie Courreges-Anglas wrote:

> 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 millert@.

 - todd

find(1) -exec + and ARG_MAX

[Jeremie Courreges-Anglas]

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?

PS: a followup diff will take into account the space taken by the
environment


Index: function.c
===
RCS file: /d/cvs/src/usr.bin/find/function.c,v
retrieving revision 1.47
diff -u -p -p -u -r1.47 function.c
--- function.c  28 Jun 2019 13:35:01 -  1.47
+++ function.c  9 Apr 2020 01:36:14 -
@@ -538,7 +538,7 @@ run_f_exec(PLAN *plan)
  *
  * If -exec ... {} +, use only the first array, but make it large
  * enough to hold 5000 args (cf. src/usr.bin/xargs/xargs.c for a
- * discussion), and then allocate ARG_MAX - 4K of space for args.
+ * discussion), and then allocate space for args.
  */
 PLAN *
 c_exec(char *unused, char ***argvp, int isok)
@@ -587,6 +587,7 @@ c_exec(char *unused, char ***argvp, int 
errx(1, "-ok: terminating \"+\" not permitted.");
 
if (new->flags & F_PLUSSET) {
+   long arg_max;
u_int c, bufsize;
 
cnt = ap - *argvp - 1;  /* units are words */
@@ -599,6 +600,14 @@ c_exec(char *unused, char ***argvp, int 
new->ep_narg = 0;
 
/*
+* Compute the maximum space we can use for arguments
+* passed to execve(2).
+*/
+   arg_max = sysconf(_SC_ARG_MAX);
+   if (arg_max == -1)
+   err(1, "sysconf(_SC_ARG_MAX) failed");
+
+   /*
 * Count up the space of the user's arguments, and
 * subtract that from what we allocate.
 */
@@ -608,8 +617,7 @@ c_exec(char *unused, char ***argvp, int 
c += strlen(*argv) + 1;
new->e_argv[cnt] = *argv;
}
-   bufsize = ARG_MAX - 4 * 1024 - c;
-
+   bufsize = arg_max - 4 * 1024 - c;
 
/*
 * Allocate, and then initialize current, base, and


-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: find.1: Markup primaries with Fl not Cm for easier tags

[Ingo Schwarze]

Hi Klemens,

Klemens Nanni wrote on Sat, Mar 21, 2020 at 12:49:20AM +0100:
> On Fri, Mar 20, 2020 at 04:53:19PM +0100, Ingo Schwarze wrote:

>> I don't feel very strongly about that, but i think .Cm does make
>> slightly more sense for primaries than .Fl (and .Ic is probably
>> acceptable, too, even though .Cm might be better because these are
>> command line arguments, not stand-alone commands).

> It does, however because the resulting effect is usually the same,
> I see no problem in using `Fl' for primaries.

Yes, it wouldn't be a big deal.  It might make a difference in
unusual situations though, for example if some website would
customize .Fl and .Cm differently in mandoc.css.

>> I think that is reasonable, yes.  I think it makes sense to consider
>> tags as words, i.e. strings that usually consist of letters and may
>> sometimes contain digits, but never contain whitespace and usually
>> do not start with punctuation characters.  I'm not sure this rule of
>> thumb can be made very strict, but i think it is possible to polish
>> this by handling a small number of common cases.  For example,
>> automatic tagging in man(7) already discards leading whitespace, 
>> some escape sequences, and leading dashes from tags.  Automatic
>> tagging in mdoc(7) already discards leading zero-width spaces
>> and leading backslashes.  I think it would make sense to also
>> skip leading dashes here, see the patch below.
>> 
>> Do you agree with that?

> Yes, I very much do.  Given the fact man(7) already works that way,
> I'm happy to see mdoc(7) follow;  it seems like the right approach, my
> attempt seems more of a workaround for special cases like find(1).
> 
> OK kn

Committed, thanks for pointing out the issue and for the feedback
on the patch.

Yours,
  Ingo

Re: find.1: Markup primaries with Fl not Cm for easier tags

[Klemens Nanni]

On Fri, Mar 20, 2020 at 04:53:19PM +0100, Ingo Schwarze wrote:
> Not really.  In the POSIX sense, options are indeed options while
> primaries are arguments.  That has implications, for example that
> all options must precede all primaries.
Fair point.

> I don't feel very strongly about that, but i think .Cm does make
> slightly more sense for primaries than .Fl (and .Ic is probably
> acceptable, too, even though .Cm might be better because these are
> command line arguments, not stand-alone commands).
It does, however because the resulting effect is usually the same, I see
no problem in using `Fl' for primaries.

> I think that is reasonable, yes.  I think it makes sense to consider
> tags as words, i.e. strings that usually consist of letters and may
> sometimes contain digits, but never contain whitespace and usually
> do not start with punctuation characters.  I'm not sure this rule of
> thumb can be made very strict, but i think it is possible to polish
> this by handling a small number of common cases.  For example,
> automatic tagging in man(7) already discards leading whitespace, 
> some escape sequences, and leading dashes from tags.  Automatic
> tagging in mdoc(7) already discards leading zero-width spaces
> and leading backslashes.  I think it would make sense to also
> skip leading dashes here, see the patch below.
> 
> Do you agree with that?
Yes, I very much do.  Given the fact man(7) already works that way,
I'm happy to see mdoc(7) follow;  it seems like the right approach, my
attempt seems more of a workaround for special cases like find(1).

OK kn

Re: find.1: Markup primaries with Fl not Cm for easier tags

[Ingo Schwarze]

Hi Klemens,

Klemens Nanni wrote on Fri, Mar 20, 2020 at 12:12:39AM +0100:

> In both command line usage and manual output format, find's options
> and primaries behave the same,

Not really.  In the POSIX sense, options are indeed options while
primaries are arguments.  That has implications, for example that
all options must precede all primaries.

> but their mdoc(7) markup is different

I don't feel very strongly about that, but i think .Cm does make
slightly more sense for primaries than .Fl (and .Ic is probably
acceptable, too, even though .Cm might be better because these are
command line arguments, not stand-alone commands).

> and therefore causes different tag names:
> 
> -x (option) can be looked up with ":tx" in the manual pager,
> whereas -amin (primary) requires ":t-amin" including the dash.
> 
> I'd like primaries to behave the same like options when it comes to
> tags in manuals, is that a reasonable expectation?

I think that is reasonable, yes.  I think it makes sense to consider
tags as words, i.e. strings that usually consist of letters and may
sometimes contain digits, but never contain whitespace and usually
do not start with punctuation characters.  I'm not sure this rule of
thumb can be made very strict, but i think it is possible to polish
this by handling a small number of common cases.  For example,
automatic tagging in man(7) already discards leading whitespace, 
some escape sequences, and leading dashes from tags.  Automatic
tagging in mdoc(7) already discards leading zero-width spaces
and leading backslashes.  I think it would make sense to also
skip leading dashes here, see the patch below.

Do you agree with that?

Yours,
  Ingo


Index: tag.c
===
RCS file: /cvs/src/usr.bin/mandoc/tag.c,v
retrieving revision 1.29
diff -u -p -r1.29 tag.c
--- tag.c   13 Mar 2020 16:14:14 -  1.29
+++ tag.c   20 Mar 2020 15:50:33 -
@@ -87,8 +87,24 @@ tag_put(const char *s, int prio, struct 
if (n->child == NULL || n->child->type != ROFFT_TEXT)
return;
s = n->child->string;
-   if (s[0] == '\\' && (s[1] == '&' || s[1] == 'e'))
-   s += 2;
+   switch (s[0]) {
+   case '-':
+   s++;
+   break;
+   case '\\':
+   switch (s[1]) {
+   case '&':
+   case '-':
+   case 'e':
+   s += 2;
+   break;
+   default:
+   break;
+   }
+   break;
+   default:
+   break;
+   }
}
 
/*

Re: find.1: Markup primaries with Fl not Cm for easier tags

[Jason McIntyre]

On Fri, Mar 20, 2020 at 12:12:39AM +0100, Klemens Nanni wrote:

morning.

> In both command line usage and manual output format, find's options
> and primaries behave the same, but their mdoc(7) markup is different and
> therefore causes different tag names:
>

i'm not sure what you mean by behaving the same, but essentially you
could think of options and primaries as being different (as the page
does now). so i'm not surprised. i don;t really think of -group, for
example, as being an option.

> -x (option) can be looked up with ":tx" in the manual pager,
> whereas -amin (primary) requires ":t-amin" including the dash.
> 
> I'd like primaries to behave the same like options when it comes to tags
> in manuals, is that a reasonable expectation?
> 

i'm not going to answer that, but...

> If so, diff below switches all primaries from `Cm -amin' to `Fl amin'
> markup such that their resulting tag name is "amin" not "-amin";  man's
> output stays identical.
> 

you mean Ic, not Cm, right? if we do decide to do this, the diff would
have to be more comprehensive. there are are a lot of places where
primaries are discussed, which would need to be changed too (i.e. not
just list items).

note also that /-x and /-amin work pretty well, without source changes!

jmc

> Feedback? OK?
> 
> 
> Index: find.1
> ===
> RCS file: /cvs/src/usr.bin/find/find.1,v
> retrieving revision 1.98
> diff -u -p -U0 -r1.98 find.1
> --- find.12 Sep 2019 21:18:41 -   1.98
> +++ find.119 Mar 2020 22:59:33 -
> @@ -149 +149 @@ the last option given overrides the othe
> -.It Ic -amin Ar n
> +.It Fl amin Ar n
> @@ -156 +156 @@ minutes.
> -.It Ic -anewer Ar file
> +.It Fl anewer Ar file
> @@ -160 +160 @@ True if the current file has a more rece
> -.It Ic -atime Ar n
> +.It Fl atime Ar n
> @@ -167 +167 @@ was started, rounded up to the next full
> -.It Ic -cmin Ar n
> +.It Fl cmin Ar n
> @@ -175 +175 @@ minutes.
> -.It Ic -cnewer Ar file
> +.It Fl cnewer Ar file
> @@ -179 +179 @@ True if the current file has a more rece
> -.It Ic -ctime Ar n
> +.It Fl ctime Ar n
> @@ -187 +187 @@ was started, rounded up to the next full
> -.It Ic -delete
> +.It Fl delete
> @@ -205 +205 @@ Following symlinks is incompatible with 
> -.It Ic -depth
> +.It Fl depth
> @@ -211 +211 @@ option.
> -.It Ic -empty
> +.It Fl empty
> @@ -214,2 +214,2 @@ True if the current file or directory is
> -.It Ic -exec Ar utility Oo Ar argument ... Oc \&;
> -.It Ic -exec Ar utility Oo Ar argument ... Oc {} +
> +.It Fl exec Ar utility Oo Ar argument ... Oc \&;
> +.It Fl exec Ar utility Oo Ar argument ... Oc {} +
> @@ -256 +256 @@ does not exceed
> -.It Ic -execdir Ar utility Oo Ar argument ... Oc \&;
> +.It Fl execdir Ar utility Oo Ar argument ... Oc \&;
> @@ -284 +284 @@ flags specified exactly match those of t
> -.It Ic -follow
> +.It Fl follow
> @@ -290 +290 @@ option.
> -.It Ic -fstype Ar type
> +.It Fl fstype Ar type
> @@ -303 +303 @@ mounted read-only.
> -.It Ic -group Ar gname
> +.It Fl group Ar gname
> @@ -312 +312 @@ is treated as a group ID.
> -.It Ic -iname Ar pattern
> +.It Fl iname Ar pattern
> @@ -317 +317 @@ primary except that the matching is done
> -.It Ic -inum Ar n
> +.It Fl inum Ar n
> @@ -321 +321 @@ True if the file has inode number
> -.It Ic -links Ar n
> +.It Fl links Ar n
> @@ -326 +326 @@ links.
> -.It Ic -ls
> +.It Fl ls
> @@ -339 +339 @@ The format is identical to that produced
> -.It Ic -maxdepth Ar n
> +.It Fl maxdepth Ar n
> @@ -343 +343 @@ True if the current search depth is less
> -.It Ic -mindepth Ar n
> +.It Fl mindepth Ar n
> @@ -347 +347 @@ True if the current search depth is at l
> -.It Ic -mmin Ar n
> +.It Fl mmin Ar n
> @@ -354 +354 @@ minutes.
> -.It Ic -mtime Ar n
> +.It Fl mtime Ar n
> @@ -361 +361 @@ was started, rounded up to the next full
> -.It Ic -name Ar pattern
> +.It Fl name Ar pattern
> @@ -367 +367 @@ which may use any of the special charact
> -.It Ic -newer Ar file
> +.It Fl newer Ar file
> @@ -371 +371 @@ True if the current file has a more rece
> -.It Ic -nogroup
> +.It Fl nogroup
> @@ -374 +374 @@ True if the file belongs to an unknown g
> -.It Ic -nouser
> +.It Fl nouser
> @@ -377 +377 @@ True if the file belongs to an unknown u
> -.It Ic -ok Ar utility Oo Ar argument ... Oc \&;
> +.It Fl ok Ar utility Oo Ar argument ... Oc \&;
> @@ -393 +393 @@ expression is false.
> -.It Ic -path Ar pattern
> +.It Fl path Ar pattern
> @@ -427 +427 @@ Note, the first character of a symbolic 
> -.It Ic -print
> +.It Fl print
> @@ -434 +434 @@ character.
> -.It Ic -print0
> +.It Fl print0
> @@ -442 +442 @@ option to
> -.It Ic -prune
> +.It Fl prune
> @@ -453 +453 @@ option was specified.
> -.It Ic -size Ar n Ns Op Cm c
> +.It Fl size Ar n Ns Op Cm c
> @@ -465 +465 @@ bytes.
> -.It Ic -type Ar t
> +.It Fl type Ar t
> @@ -486 +486 @@ socket
> -.It Ic -user Ar uname
> +.It Fl user Ar uname
> @@ -495 +495 @@ is treated as a user ID.
> -.It Ic -xdev
> +.It Fl xdev
> 

find.1: Markup primaries with Fl not Cm for easier tags

[Klemens Nanni]

In both command line usage and manual output format, find's options
and primaries behave the same, but their mdoc(7) markup is different and
therefore causes different tag names:

-x (option) can be looked up with ":tx" in the manual pager,
whereas -amin (primary) requires ":t-amin" including the dash.

I'd like primaries to behave the same like options when it comes to tags
in manuals, is that a reasonable expectation?

If so, diff below switches all primaries from `Cm -amin' to `Fl amin'
markup such that their resulting tag name is "amin" not "-amin";  man's
output stays identical.

Feedback? OK?


Index: find.1
=======
RCS file: /cvs/src/usr.bin/find/find.1,v
retrieving revision 1.98
diff -u -p -U0 -r1.98 find.1
--- find.1  2 Sep 2019 21:18:41 -   1.98
+++ find.1  19 Mar 2020 22:59:33 -
@@ -149 +149 @@ the last option given overrides the othe
-.It Ic -amin Ar n
+.It Fl amin Ar n
@@ -156 +156 @@ minutes.
-.It Ic -anewer Ar file
+.It Fl anewer Ar file
@@ -160 +160 @@ True if the current file has a more rece
-.It Ic -atime Ar n
+.It Fl atime Ar n
@@ -167 +167 @@ was started, rounded up to the next full
-.It Ic -cmin Ar n
+.It Fl cmin Ar n
@@ -175 +175 @@ minutes.
-.It Ic -cnewer Ar file
+.It Fl cnewer Ar file
@@ -179 +179 @@ True if the current file has a more rece
-.It Ic -ctime Ar n
+.It Fl ctime Ar n
@@ -187 +187 @@ was started, rounded up to the next full
-.It Ic -delete
+.It Fl delete
@@ -205 +205 @@ Following symlinks is incompatible with 
-.It Ic -depth
+.It Fl depth
@@ -211 +211 @@ option.
-.It Ic -empty
+.It Fl empty
@@ -214,2 +214,2 @@ True if the current file or directory is
-.It Ic -exec Ar utility Oo Ar argument ... Oc \&;
-.It Ic -exec Ar utility Oo Ar argument ... Oc {} +
+.It Fl exec Ar utility Oo Ar argument ... Oc \&;
+.It Fl exec Ar utility Oo Ar argument ... Oc {} +
@@ -256 +256 @@ does not exceed
-.It Ic -execdir Ar utility Oo Ar argument ... Oc \&;
+.It Fl execdir Ar utility Oo Ar argument ... Oc \&;
@@ -284 +284 @@ flags specified exactly match those of t
-.It Ic -follow
+.It Fl follow
@@ -290 +290 @@ option.
-.It Ic -fstype Ar type
+.It Fl fstype Ar type
@@ -303 +303 @@ mounted read-only.
-.It Ic -group Ar gname
+.It Fl group Ar gname
@@ -312 +312 @@ is treated as a group ID.
-.It Ic -iname Ar pattern
+.It Fl iname Ar pattern
@@ -317 +317 @@ primary except that the matching is done
-.It Ic -inum Ar n
+.It Fl inum Ar n
@@ -321 +321 @@ True if the file has inode number
-.It Ic -links Ar n
+.It Fl links Ar n
@@ -326 +326 @@ links.
-.It Ic -ls
+.It Fl ls
@@ -339 +339 @@ The format is identical to that produced
-.It Ic -maxdepth Ar n
+.It Fl maxdepth Ar n
@@ -343 +343 @@ True if the current search depth is less
-.It Ic -mindepth Ar n
+.It Fl mindepth Ar n
@@ -347 +347 @@ True if the current search depth is at l
-.It Ic -mmin Ar n
+.It Fl mmin Ar n
@@ -354 +354 @@ minutes.
-.It Ic -mtime Ar n
+.It Fl mtime Ar n
@@ -361 +361 @@ was started, rounded up to the next full
-.It Ic -name Ar pattern
+.It Fl name Ar pattern
@@ -367 +367 @@ which may use any of the special charact
-.It Ic -newer Ar file
+.It Fl newer Ar file
@@ -371 +371 @@ True if the current file has a more rece
-.It Ic -nogroup
+.It Fl nogroup
@@ -374 +374 @@ True if the file belongs to an unknown g
-.It Ic -nouser
+.It Fl nouser
@@ -377 +377 @@ True if the file belongs to an unknown u
-.It Ic -ok Ar utility Oo Ar argument ... Oc \&;
+.It Fl ok Ar utility Oo Ar argument ... Oc \&;
@@ -393 +393 @@ expression is false.
-.It Ic -path Ar pattern
+.It Fl path Ar pattern
@@ -427 +427 @@ Note, the first character of a symbolic 
-.It Ic -print
+.It Fl print
@@ -434 +434 @@ character.
-.It Ic -print0
+.It Fl print0
@@ -442 +442 @@ option to
-.It Ic -prune
+.It Fl prune
@@ -453 +453 @@ option was specified.
-.It Ic -size Ar n Ns Op Cm c
+.It Fl size Ar n Ns Op Cm c
@@ -465 +465 @@ bytes.
-.It Ic -type Ar t
+.It Fl type Ar t
@@ -486 +486 @@ socket
-.It Ic -user Ar uname
+.It Fl user Ar uname
@@ -495 +495 @@ is treated as a user ID.
-.It Ic -xdev
+.It Fl xdev

Re: sparc64: find root device on hardware RAID

[Mark Kettenis]

> Date: Mon, 13 Jan 2020 10:59:30 +0100
> From: Klemens Nanni 
> 
> On Fri, Jan 03, 2020 at 08:30:42PM +0100, Mark Kettenis wrote:
> > Can we leave out the #ifdef __sparc64__?  Unless somebody can come up
> > with a really good reason for it...
> The code should be safe on all platforms but I put it in to ensure I'm
> not breaking stuff I cannot test, e.g. anything else than sparc64/OBP.
> 
> deraadt expressed the same concerns.
> 
> With the two of you arguing for removal, I'm confident enough to remove
> it and make mpii(4) MI again;  unless I hear arguments against it, I'll
> commit this soon.

Great, ok kettenis@

> Index: mpii.c
> ===
> RCS file: /cvs/src/sys/dev/pci/mpii.c,v
> retrieving revision 1.125
> diff -u -p -r1.125 mpii.c
> --- mpii.c3 Jan 2020 08:39:31 -   1.125
> +++ mpii.c13 Jan 2020 09:59:01 -
> @@ -930,15 +930,13 @@ mpii_scsi_probe(struct scsi_link *link)
>   return (EINVAL);
>  
>   link->port_wwn = letoh64(vpg.wwid);
> -#ifdef __sparc64__
>   /*
>* WWIDs generated by LSI firmware are not IEEE NAA compliant
> -  * so historical practise in OBP is to set the top nibble to 3
> -  * to indicate that this is a RAID volume.
> +  * and historical practise in OBP on sparc64 is to set the top
> +  * nibble to 3 to indicate that this is a RAID volume.
>*/
>   link->port_wwn &= 0x0fff;
>   link->port_wwn |= 0x3000;
> -#endif
>  
>   return (0);
>   }
> 

Re: sparc64: find root device on hardware RAID

[Klemens Nanni]

On Fri, Jan 03, 2020 at 08:30:42PM +0100, Mark Kettenis wrote:
> Can we leave out the #ifdef __sparc64__?  Unless somebody can come up
> with a really good reason for it...
The code should be safe on all platforms but I put it in to ensure I'm
not breaking stuff I cannot test, e.g. anything else than sparc64/OBP.

deraadt expressed the same concerns.

With the two of you arguing for removal, I'm confident enough to remove
it and make mpii(4) MI again;  unless I hear arguments against it, I'll
commit this soon.


Index: mpii.c
===
RCS file: /cvs/src/sys/dev/pci/mpii.c,v
retrieving revision 1.125
diff -u -p -r1.125 mpii.c
--- mpii.c  3 Jan 2020 08:39:31 -   1.125
+++ mpii.c  13 Jan 2020 09:59:01 -
@@ -930,15 +930,13 @@ mpii_scsi_probe(struct scsi_link *link)
return (EINVAL);
 
link->port_wwn = letoh64(vpg.wwid);
-#ifdef __sparc64__
/*
 * WWIDs generated by LSI firmware are not IEEE NAA compliant
-* so historical practise in OBP is to set the top nibble to 3
-* to indicate that this is a RAID volume.
+* and historical practise in OBP on sparc64 is to set the top
+* nibble to 3 to indicate that this is a RAID volume.
 */
link->port_wwn &= 0x0fff;
link->port_wwn |= 0x3000;
-#endif
 
return (0);
}

Re: sparc64: find root device on hardware RAID

[Mark Kettenis]

> Date: Tue, 31 Dec 2019 09:12:56 +1000
> From: Jonathan Matthew 
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> 
> On Mon, Dec 30, 2019 at 03:36:54PM +0100, Klemens Nanni wrote:
> > On Mon, Dec 30, 2019 at 06:59:35PM +1000, Jonathan Matthew wrote:
> > > Here's the Solaris explanation:
> > > 
> > > https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/sys/scsi/adapters/mpt_sas/mptsas_var.h#L195
> > Thanks for digging.
> > 
> > > I think we should copy what they're doing here, that is, replace the high 
> > > bits
> > > with 3 rather than adding 3 to it.  I'm really not sure where we should do
> > > this though.  Maybe in mpii, but only on sparc64?
> > As that is now a general quirk in all RAID volumes and no longer
> > specific to bootpath handling, mpii seems only appropiate and autoconf
> > must not know about this.
> > 
> > Since Illumos does exactly that with mptsas_get_raid_wwid() in
> > mptsas_raidconf_page_0_cb(), which after a brief look seems like the
> > code path equivalent to our recent WWID addition in mpii_scsi_probe(),
> > I'm inclined to just do it there.
> > 
> > Diff below doas that.
> > 
> > > As far as I can tell, the raid controller generates 128 bit WWIDs for raid
> > > volumes, but only has a 64 bit field to report the ID to the host, so it 
> > > only
> > > puts the vendor-specified part here (you can see the last half of the ID 
> > > string
> > > printed when sd0 attaches matches sl->port_wwn in reverse).  I think the
> > > purpose of setting the high 4 bits to 3 is that 3 is not a defined NAA 
> > > value,
> > > so you're not going to find a proper WWID coming from other device that 
> > > will
> > > match that.
> > I did not manage to recognise this detail of the reversed ID, indeed:
> > 
> > sd0 at scsibus1 targ 0 lun 0:  
> > naa.600508e06cd1dcd59022a30a
> > 
> > Feedback? OK?
> 
> I think this is probably the most sensible thing we can do here.
> ok jmatthew@ but I'd wait and see if anyone has a better idea.

Can we leave out the #ifdef __sparc64__?  Unless somebody can come up
with a really good reason for it...

> > Index: dev/pci/mpii.c
> > ===
> > RCS file: /cvs/src/sys/dev/pci/mpii.c,v
> > retrieving revision 1.123
> > diff -u -p -r1.123 mpii.c
> > --- dev/pci/mpii.c  29 Dec 2019 21:30:21 -  1.123
> > +++ dev/pci/mpii.c  30 Dec 2019 14:26:57 -
> > @@ -930,6 +930,15 @@ mpii_scsi_probe(struct scsi_link *link)
> > return (EINVAL);
> >  
> > link->port_wwn = letoh64(vpg.wwid);
> > +#ifdef __sparc64__
> > +   /*
> > +* WWIDs generated by LSI firmware are not IEEE NAA compliant
> > +* so historical practise in OBP is to set the top nibble to 3
> > +* to indicate that this is a RAID volume.
> > +*/
> > +   link->port_wwn &= 0x0fff;
> > +   link->port_wwn |= 0x3000;
> > +#endif
> >  
> > return (0);
> > }
> > Index: arch/sparc64/sparc64/autoconf.c
> > ===
> > RCS file: /cvs/src/sys/arch/sparc64/sparc64/autoconf.c,v
> > retrieving revision 1.133
> > diff -u -p -r1.133 autoconf.c
> > --- arch/sparc64/sparc64/autoconf.c 15 Oct 2019 05:21:16 -  1.133
> > +++ arch/sparc64/sparc64/autoconf.c 30 Dec 2019 14:27:17 -
> > @@ -1455,7 +1455,7 @@ device_register(struct device *dev, void
> > u_int lun = bp->val[1];
> >  
> > if (bp->val[0] & 0x && bp->val[0] != -1) {
> > -   /* Fibre channel? */
> > +   /* Hardware RAID or Fibre channel? */
> > if (bp->val[0] == sl->port_wwn && lun == sl->lun) {
> > nail_bootdev(dev, bp);
> > }
> 
> 

Re: sparc64: find root device on hardware RAID

[Mark Kettenis]

> Date: Mon, 30 Dec 2019 18:59:35 +1000
> From: Jonathan Matthew 
> 
> On Sun, Dec 29, 2019 at 05:58:02AM +0100, Klemens Nanni wrote:
> > On Sun, Dec 29, 2019 at 01:59:38PM +1000, Jonathan Matthew wrote:
> > > I think we have the wrong size for the volume name, hence the difference
> > > between the size reported by the controller and the size of vpg.
> > Indeed, good catch!
> > 
> > OBP's `create-raid*-volume' commands also prompt for names no longer
> > than that:
> > 
> > {0} ok 9 b c d create-raid0-volume
> > ...
> > Enter a volume name:  [0 to 15 characters] foo
> > {0} ok
> > 
> > > try this out?
> > Just works, the WWID is no longer clobbered and autoconf eventually sees
> > it in the port WWN:
> > 
> > mpii0 at pci15 dev 0 function 0 "Symbios Logic SAS2008" rev 0x03: msi
> > mpii0: Solana On-Board, firmware 9.0.0.0 IR, MPI 2.0
> > scsibus1 at mpii0: 834 targets
> > mpii_scsi_probe: target 0 lun 0 port_wwn 0 node_wwn 0 has 
> > MPII_DF_VOLUME set in flags 10
> > struct mpii_cfg_raid_vol_pg1 vpg:
> > volume_id: 81, tvolume_bus: 3, volume_ioc: 0
> > wwid: aa32290d5dcd16c
> > sd0 at scsibus1 targ 0 lun 0:  
> > naa.600508e06cd1dcd59022a30a
> > device_register: RAID:
> > bp->val[]: 3aa32290d5dcd16c, 0, 0
> > target: d5dcd16c, sl->target: 0
> > lun: 0, sl->lun: 0
> > sl->port_wwn: aa32290d5dcd16c, sl->node_wwn: 0
> > 
> > sd0: 713824MB, 512 bytes/sector, 1461911552 sectors
> > 
> > Thanks a lot,
> > OK kn
> > 
> > Now I need to work around the first digit's mismatch;  for reasons still
> > unknown to me, official documentation states that the RAID volume WWID's
> > first digit --if it is zero-- must be replaced with three,  so the
> > bootpath contains 3aa32290d5dcd16c whereas the port WWN has the correct
> > aa32290d5dcd16c.
> 
> Here's the Solaris explanation:
> 
> https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/sys/scsi/adapters/mpt_sas/mptsas_var.h#L195
> 
> I think we should copy what they're doing here, that is, replace the high bits
> with 3 rather than adding 3 to it.  I'm really not sure where we should do
> this though.  Maybe in mpii, but only on sparc64?
> 
> As far as I can tell, the raid controller generates 128 bit WWIDs for raid
> volumes, but only has a 64 bit field to report the ID to the host, so it only
> puts the vendor-specified part here (you can see the last half of the ID 
> string
> printed when sd0 attaches matches sl->port_wwn in reverse).  I think the
> purpose of setting the high 4 bits to 3 is that 3 is not a defined NAA value,
> so you're not going to find a proper WWID coming from other device that will
> match that.

Can you think of a reason why we would care about the exact WWIDs for
these RAID volumes on other architectures?  If not, I'd prefer to
"fix" this in mpii(4) since this sounds like a deficiency in the LSI
firmware.

Re: sparc64: find root device on hardware RAID

[Klemens Nanni]

On Tue, Dec 31, 2019 at 09:12:56AM +1000, Jonathan Matthew wrote:
> I think this is probably the most sensible thing we can do here.
> ok jmatthew@ but I'd wait and see if anyone has a better idea.
I'll commit later this evening so that snapshots will just work on my
machine, this makes upgrading easier for me.

We can still improve this in-tree if someone comes up with a better idea.

Re: sparc64: find root device on hardware RAID

[Jonathan Matthew]

On Mon, Dec 30, 2019 at 03:36:54PM +0100, Klemens Nanni wrote:
> On Mon, Dec 30, 2019 at 06:59:35PM +1000, Jonathan Matthew wrote:
> > Here's the Solaris explanation:
> > 
> > https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/sys/scsi/adapters/mpt_sas/mptsas_var.h#L195
> Thanks for digging.
> 
> > I think we should copy what they're doing here, that is, replace the high 
> > bits
> > with 3 rather than adding 3 to it.  I'm really not sure where we should do
> > this though.  Maybe in mpii, but only on sparc64?
> As that is now a general quirk in all RAID volumes and no longer
> specific to bootpath handling, mpii seems only appropiate and autoconf
> must not know about this.
> 
> Since Illumos does exactly that with mptsas_get_raid_wwid() in
> mptsas_raidconf_page_0_cb(), which after a brief look seems like the
> code path equivalent to our recent WWID addition in mpii_scsi_probe(),
> I'm inclined to just do it there.
> 
> Diff below doas that.
> 
> > As far as I can tell, the raid controller generates 128 bit WWIDs for raid
> > volumes, but only has a 64 bit field to report the ID to the host, so it 
> > only
> > puts the vendor-specified part here (you can see the last half of the ID 
> > string
> > printed when sd0 attaches matches sl->port_wwn in reverse).  I think the
> > purpose of setting the high 4 bits to 3 is that 3 is not a defined NAA 
> > value,
> > so you're not going to find a proper WWID coming from other device that will
> > match that.
> I did not manage to recognise this detail of the reversed ID, indeed:
> 
>   sd0 at scsibus1 targ 0 lun 0:  
> naa.600508e06cd1dcd59022a30a
> 
> Feedback? OK?

I think this is probably the most sensible thing we can do here.
ok jmatthew@ but I'd wait and see if anyone has a better idea.

> 
> 
> Index: dev/pci/mpii.c
> ===
> RCS file: /cvs/src/sys/dev/pci/mpii.c,v
> retrieving revision 1.123
> diff -u -p -r1.123 mpii.c
> --- dev/pci/mpii.c29 Dec 2019 21:30:21 -  1.123
> +++ dev/pci/mpii.c30 Dec 2019 14:26:57 -
> @@ -930,6 +930,15 @@ mpii_scsi_probe(struct scsi_link *link)
>   return (EINVAL);
>  
>   link->port_wwn = letoh64(vpg.wwid);
> +#ifdef __sparc64__
> + /*
> +  * WWIDs generated by LSI firmware are not IEEE NAA compliant
> +  * so historical practise in OBP is to set the top nibble to 3
> +  * to indicate that this is a RAID volume.
> +  */
> + link->port_wwn &= 0x0fff;
> + link->port_wwn |= 0x3000;
> +#endif
>  
>   return (0);
>   }
> Index: arch/sparc64/sparc64/autoconf.c
> ===
> RCS file: /cvs/src/sys/arch/sparc64/sparc64/autoconf.c,v
> retrieving revision 1.133
> diff -u -p -r1.133 autoconf.c
> --- arch/sparc64/sparc64/autoconf.c   15 Oct 2019 05:21:16 -  1.133
> +++ arch/sparc64/sparc64/autoconf.c   30 Dec 2019 14:27:17 -
> @@ -1455,7 +1455,7 @@ device_register(struct device *dev, void
>   u_int lun = bp->val[1];
>  
>   if (bp->val[0] & 0x && bp->val[0] != -1) {
> - /* Fibre channel? */
> + /* Hardware RAID or Fibre channel? */
>   if (bp->val[0] == sl->port_wwn && lun == sl->lun) {
>   nail_bootdev(dev, bp);
>   }

Re: sparc64: find root device on hardware RAID

[Klemens Nanni]

On Mon, Dec 30, 2019 at 06:59:35PM +1000, Jonathan Matthew wrote:
> Here's the Solaris explanation:
> 
> https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/sys/scsi/adapters/mpt_sas/mptsas_var.h#L195
Thanks for digging.

> I think we should copy what they're doing here, that is, replace the high bits
> with 3 rather than adding 3 to it.  I'm really not sure where we should do
> this though.  Maybe in mpii, but only on sparc64?
As that is now a general quirk in all RAID volumes and no longer
specific to bootpath handling, mpii seems only appropiate and autoconf
must not know about this.

Since Illumos does exactly that with mptsas_get_raid_wwid() in
mptsas_raidconf_page_0_cb(), which after a brief look seems like the
code path equivalent to our recent WWID addition in mpii_scsi_probe(),
I'm inclined to just do it there.

Diff below doas that.

> As far as I can tell, the raid controller generates 128 bit WWIDs for raid
> volumes, but only has a 64 bit field to report the ID to the host, so it only
> puts the vendor-specified part here (you can see the last half of the ID 
> string
> printed when sd0 attaches matches sl->port_wwn in reverse).  I think the
> purpose of setting the high 4 bits to 3 is that 3 is not a defined NAA value,
> so you're not going to find a proper WWID coming from other device that will
> match that.
I did not manage to recognise this detail of the reversed ID, indeed:

sd0 at scsibus1 targ 0 lun 0:  
naa.600508e06cd1dcd59022a30a

Feedback? OK?


Index: dev/pci/mpii.c
===
RCS file: /cvs/src/sys/dev/pci/mpii.c,v
retrieving revision 1.123
diff -u -p -r1.123 mpii.c
--- dev/pci/mpii.c  29 Dec 2019 21:30:21 -  1.123
+++ dev/pci/mpii.c  30 Dec 2019 14:26:57 -
@@ -930,6 +930,15 @@ mpii_scsi_probe(struct scsi_link *link)
return (EINVAL);
 
link->port_wwn = letoh64(vpg.wwid);
+#ifdef __sparc64__
+   /*
+* WWIDs generated by LSI firmware are not IEEE NAA compliant
+* so historical practise in OBP is to set the top nibble to 3
+* to indicate that this is a RAID volume.
+*/
+   link->port_wwn &= 0x0fff;
+   link->port_wwn |= 0x3000;
+#endif
 
return (0);
}
Index: arch/sparc64/sparc64/autoconf.c
===
RCS file: /cvs/src/sys/arch/sparc64/sparc64/autoconf.c,v
retrieving revision 1.133
diff -u -p -r1.133 autoconf.c
--- arch/sparc64/sparc64/autoconf.c 15 Oct 2019 05:21:16 -  1.133
+++ arch/sparc64/sparc64/autoconf.c 30 Dec 2019 14:27:17 -
@@ -1455,7 +1455,7 @@ device_register(struct device *dev, void
u_int lun = bp->val[1];
 
if (bp->val[0] & 0x && bp->val[0] != -1) {
-   /* Fibre channel? */
+   /* Hardware RAID or Fibre channel? */
if (bp->val[0] == sl->port_wwn && lun == sl->lun) {
nail_bootdev(dev, bp);
}

Re: sparc64: find root device on hardware RAID

[Jonathan Matthew]

On Sun, Dec 29, 2019 at 05:58:02AM +0100, Klemens Nanni wrote:
> On Sun, Dec 29, 2019 at 01:59:38PM +1000, Jonathan Matthew wrote:
> > I think we have the wrong size for the volume name, hence the difference
> > between the size reported by the controller and the size of vpg.
> Indeed, good catch!
> 
> OBP's `create-raid*-volume' commands also prompt for names no longer
> than that:
> 
>   {0} ok 9 b c d create-raid0-volume
>   ...
>   Enter a volume name:  [0 to 15 characters] foo
>   {0} ok
> 
> > try this out?
> Just works, the WWID is no longer clobbered and autoconf eventually sees
> it in the port WWN:
> 
>   mpii0 at pci15 dev 0 function 0 "Symbios Logic SAS2008" rev 0x03: msi
>   mpii0: Solana On-Board, firmware 9.0.0.0 IR, MPI 2.0
>   scsibus1 at mpii0: 834 targets
>   mpii_scsi_probe: target 0 lun 0 port_wwn 0 node_wwn 0 has 
> MPII_DF_VOLUME set in flags 10
>   struct mpii_cfg_raid_vol_pg1 vpg:
>   volume_id: 81, tvolume_bus: 3, volume_ioc: 0
>   wwid: aa32290d5dcd16c
>   sd0 at scsibus1 targ 0 lun 0:  
> naa.600508e06cd1dcd59022a30a
>   device_register: RAID:
>   bp->val[]: 3aa32290d5dcd16c, 0, 0
>   target: d5dcd16c, sl->target: 0
>   lun: 0, sl->lun: 0
>   sl->port_wwn: aa32290d5dcd16c, sl->node_wwn: 0
> 
>   sd0: 713824MB, 512 bytes/sector, 1461911552 sectors
> 
> Thanks a lot,
> OK kn
> 
> Now I need to work around the first digit's mismatch;  for reasons still
> unknown to me, official documentation states that the RAID volume WWID's
> first digit --if it is zero-- must be replaced with three,  so the
> bootpath contains 3aa32290d5dcd16c whereas the port WWN has the correct
> aa32290d5dcd16c.

Here's the Solaris explanation:

https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/sys/scsi/adapters/mpt_sas/mptsas_var.h#L195

I think we should copy what they're doing here, that is, replace the high bits
with 3 rather than adding 3 to it.  I'm really not sure where we should do
this though.  Maybe in mpii, but only on sparc64?

As far as I can tell, the raid controller generates 128 bit WWIDs for raid
volumes, but only has a 64 bit field to report the ID to the host, so it only
puts the vendor-specified part here (you can see the last half of the ID string
printed when sd0 attaches matches sl->port_wwn in reverse).  I think the
purpose of setting the high 4 bits to 3 is that 3 is not a defined NAA value,
so you're not going to find a proper WWID coming from other device that will
match that.

Re: sparc64: find root device on hardware RAID

[Klemens Nanni]

On Mon, Dec 30, 2019 at 06:54:02AM +1000, Jonathan Matthew wrote:
> I'd prefer this:
> 
> pagelen = min(hdr.page_length * 4, sizeof(vpg));
> 
> just to avoid trashing the stack if the page grows in newer firmware versions.
Sure, I'll commit with min() and a small comment for that, thanks.

Re: sparc64: find root device on hardware RAID

[Jonathan Matthew]

On Sun, Dec 29, 2019 at 07:40:01AM +0100, Klemens Nanni wrote:
> On Sun, Dec 29, 2019 at 03:30:15AM +0100, Klemens Nanni wrote:
> > > link->target isn't the right place to put this, for one thing it's only 
> > > 16 bits
> > > and the wwn is 64 bits, and it's used throughout the driver to look up 
> > > devices
> > > in an array, so changing it will break things.  I think link->port_wwn is 
> > > the
> > > right place to store it.
> > Ah, link->target was there because I played with that in earlier diffs,
> > port_wwn is indeed correct here.
> > 
> > > The fields in the page returned by the driver are also little endian, so 
> > > you'd
> > > need letoh64(vpg.wwid) here.
> > Thanks, I see that is done elsewhere in the driver, too.
> >  
> > > You should still return 0 here, as continuing on will send the SAS device
> > > page 0 request to the raid volume, which will probably upset the 
> > > controller
> > > enough to stop talking to you.
> > Oh well... that explains the hangs - I accidentially dropped the return,
> > it should obviously stay and my diff should only add another page fetch.
> All three points are addressed in the updated diff below, diff three to
> find the root device.
> 
> mpii(4) currently leaves the port WWN empty for RAID volumes (physical
> devices are populated).  autoboot(9) uses this to match the root device
> as per the second diff, so we must fill in after fetching the relevant
> page as suggested by mikeb and jmatthew.
> 
> Feedback? OK?

ok jmatthew@ with one tweak below.

> 
> Index: mpii.c
> ===
> RCS file: /cvs/src/sys/dev/pci/mpii.c,v
> retrieving revision 1.122
> diff -u -p -r1.122 mpii.c
> --- mpii.c28 Dec 2019 04:38:22 -  1.122
> +++ mpii.c29 Dec 2019 05:02:38 -
> @@ -910,8 +910,28 @@ mpii_scsi_probe(struct scsi_link *link)
>   if (ISSET(flags, MPII_DF_HIDDEN) || ISSET(flags, MPII_DF_UNUSED))
>   return (1);
>  
> - if (ISSET(flags, MPII_DF_VOLUME))
> + if (ISSET(flags, MPII_DF_VOLUME)) {
> + struct mpii_cfg_hdr hdr;
> + struct mpii_cfg_raid_vol_pg1 vpg;
> + size_t pagelen;
> +
> + address = MPII_CFG_RAID_VOL_ADDR_HANDLE | dev->dev_handle;
> +
> + if (mpii_req_cfg_header(sc, MPII_CONFIG_REQ_PAGE_TYPE_RAID_VOL,
> + 1, address, MPII_PG_POLL, ) != 0)
> + return (EINVAL);
> +
> + memset(, 0, sizeof(vpg));
> + pagelen = hdr.page_length * 4;

I'd prefer this:

pagelen = min(hdr.page_length * 4, sizeof(vpg));

just to avoid trashing the stack if the page grows in newer firmware versions.

> +
> + if (mpii_req_cfg_page(sc, address, MPII_PG_POLL, , 1,
> + , pagelen) != 0)
> + return (EINVAL);
> +
> + link->port_wwn = letoh64(vpg.wwid);
> +
>   return (0);
> + }
>  
>   memset(, 0, sizeof(ehdr));
>   ehdr.page_type = MPII_CONFIG_REQ_PAGE_TYPE_EXTENDED;

Re: sparc64: find root device on hardware RAID

[Klemens Nanni]

On Sun, Dec 29, 2019 at 03:30:15AM +0100, Klemens Nanni wrote:
> > link->target isn't the right place to put this, for one thing it's only 16 
> > bits
> > and the wwn is 64 bits, and it's used throughout the driver to look up 
> > devices
> > in an array, so changing it will break things.  I think link->port_wwn is 
> > the
> > right place to store it.
> Ah, link->target was there because I played with that in earlier diffs,
> port_wwn is indeed correct here.
> 
> > The fields in the page returned by the driver are also little endian, so 
> > you'd
> > need letoh64(vpg.wwid) here.
> Thanks, I see that is done elsewhere in the driver, too.
>  
> > You should still return 0 here, as continuing on will send the SAS device
> > page 0 request to the raid volume, which will probably upset the controller
> > enough to stop talking to you.
> Oh well... that explains the hangs - I accidentially dropped the return,
> it should obviously stay and my diff should only add another page fetch.
All three points are addressed in the updated diff below, diff three to
find the root device.

mpii(4) currently leaves the port WWN empty for RAID volumes (physical
devices are populated).  autoboot(9) uses this to match the root device
as per the second diff, so we must fill in after fetching the relevant
page as suggested by mikeb and jmatthew.

Feedback? OK?

Index: mpii.c
===
RCS file: /cvs/src/sys/dev/pci/mpii.c,v
retrieving revision 1.122
diff -u -p -r1.122 mpii.c
--- mpii.c  28 Dec 2019 04:38:22 -  1.122
+++ mpii.c  29 Dec 2019 05:02:38 -
@@ -910,8 +910,28 @@ mpii_scsi_probe(struct scsi_link *link)
if (ISSET(flags, MPII_DF_HIDDEN) || ISSET(flags, MPII_DF_UNUSED))
return (1);
 
-   if (ISSET(flags, MPII_DF_VOLUME))
+   if (ISSET(flags, MPII_DF_VOLUME)) {
+   struct mpii_cfg_hdr hdr;
+   struct mpii_cfg_raid_vol_pg1 vpg;
+   size_t pagelen;
+
+   address = MPII_CFG_RAID_VOL_ADDR_HANDLE | dev->dev_handle;
+
+   if (mpii_req_cfg_header(sc, MPII_CONFIG_REQ_PAGE_TYPE_RAID_VOL,
+   1, address, MPII_PG_POLL, ) != 0)
+   return (EINVAL);
+
+   memset(, 0, sizeof(vpg));
+   pagelen = hdr.page_length * 4;
+
+   if (mpii_req_cfg_page(sc, address, MPII_PG_POLL, , 1,
+   , pagelen) != 0)
+   return (EINVAL);
+
+   link->port_wwn = letoh64(vpg.wwid);
+
return (0);
+   }
 
memset(, 0, sizeof(ehdr));
ehdr.page_type = MPII_CONFIG_REQ_PAGE_TYPE_EXTENDED;

Re: sparc64: find root device on hardware RAID

[Klemens Nanni]

On Sun, Dec 29, 2019 at 05:58:02AM +0100, Klemens Nanni wrote:
> Now I need to work around the first digit's mismatch;  for reasons still
> unknown to me, official documentation states that the RAID volume WWID's
> first digit --if it is zero-- must be replaced with three,  so the
> bootpath contains 3aa32290d5dcd16c whereas the port WWN has the correct
> aa32290d5dcd16c.
> 
> Fix that and the kernel will match the device and find its root device
> automatically.  I get around to a diff for that now.
https://jp.fujitsu.com/platform/server/sparc/manual/en/c120-e679/14.2.12.html
is the only freely available documentation I could find online which
mentions how to craft bootpaths for RAID volumes.

It says
Replace the number "0" at the beginning of WWID of the RAID volume with 
"3".

which implies to me that every WWID of such RAID volumes starts with
zero, hence it must always be replaced three;  otherwise documentation
is incomplete.

I cannot find documentation or any kind of reasoning as to *why* this
must be done, anyone who knows please enlighten me.


So to match these, simply add an additional check in the existing code
path for Fibre channel with already does what we want for hardware RAID.

This is the second of three diffs for autoconf(9) to find the root
device automatically, jmatthew previously sent the first one.  Third
one follows in a separate mail.

Feedback? OK?


Index: sparc64/autoconf.c
===
RCS file: /cvs/src/sys/arch/sparc64/sparc64/autoconf.c,v
retrieving revision 1.133
diff -u -p -r1.133 autoconf.c
--- sparc64/autoconf.c  15 Oct 2019 05:21:16 -  1.133
+++ sparc64/autoconf.c  29 Dec 2019 06:05:44 -
@@ -1455,8 +1455,9 @@ device_register(struct device *dev, void
u_int lun = bp->val[1];
 
if (bp->val[0] & 0x && bp->val[0] != -1) {
-   /* Fibre channel? */
-   if (bp->val[0] == sl->port_wwn && lun == sl->lun) {
+   /* Hardware RAID or Fibre channel? */
+   if ((bp->val[0] == sl->port_wwn + 0x3000 ||
+bp->val[0] == sl->port_wwn) && lun == sl->lun) {
nail_bootdev(dev, bp);
}
 

Re: sparc64: find root device on hardware RAID

[Klemens Nanni]

On Sun, Dec 29, 2019 at 01:59:38PM +1000, Jonathan Matthew wrote:
> I think we have the wrong size for the volume name, hence the difference
> between the size reported by the controller and the size of vpg.
Indeed, good catch!

OBP's `create-raid*-volume' commands also prompt for names no longer
than that:

{0} ok 9 b c d create-raid0-volume
...
Enter a volume name:  [0 to 15 characters] foo
{0} ok

> try this out?
Just works, the WWID is no longer clobbered and autoconf eventually sees
it in the port WWN:

mpii0 at pci15 dev 0 function 0 "Symbios Logic SAS2008" rev 0x03: msi
mpii0: Solana On-Board, firmware 9.0.0.0 IR, MPI 2.0
scsibus1 at mpii0: 834 targets
mpii_scsi_probe: target 0 lun 0 port_wwn 0 node_wwn 0 has 
MPII_DF_VOLUME set in flags 10
struct mpii_cfg_raid_vol_pg1 vpg:
volume_id: 81, tvolume_bus: 3, volume_ioc: 0
wwid: aa32290d5dcd16c
sd0 at scsibus1 targ 0 lun 0:  
naa.600508e06cd1dcd59022a30a
device_register: RAID:
bp->val[]: 3aa32290d5dcd16c, 0, 0
target: d5dcd16c, sl->target: 0
lun: 0, sl->lun: 0
sl->port_wwn: aa32290d5dcd16c, sl->node_wwn: 0

sd0: 713824MB, 512 bytes/sector, 1461911552 sectors

Thanks a lot,
OK kn

Now I need to work around the first digit's mismatch;  for reasons still
unknown to me, official documentation states that the RAID volume WWID's
first digit --if it is zero-- must be replaced with three,  so the
bootpath contains 3aa32290d5dcd16c whereas the port WWN has the correct
aa32290d5dcd16c.

Fix that and the kernel will match the device and find its root device
automatically.  I get around to a diff for that now.

Re: sparc64: find root device on hardware RAID

[Jonathan Matthew]

On Sun, Dec 29, 2019 at 03:30:15AM +0100, Klemens Nanni wrote:
> On Sun, Dec 29, 2019 at 10:29:48AM +1000, Jonathan Matthew wrote:
> > > + memset(, 0, sizeof(vpg));
> > > + pagelen = hdr.page_length * 4;
> > 
> > We probably should check that this isn't larger than the size of vpg.
> pagelen is 64, sizeof(vpg) is 80.
> 
> > link->target isn't the right place to put this, for one thing it's only 16 
> > bits
> > and the wwn is 64 bits, and it's used throughout the driver to look up 
> > devices
> > in an array, so changing it will break things.  I think link->port_wwn is 
> > the
> > right place to store it.
> Ah, link->target was there because I played with that in earlier diffs,
> port_wwn is indeed correct here.
> 
> > The fields in the page returned by the driver are also little endian, so 
> > you'd
> > need letoh64(vpg.wwid) here.
> Thanks, I see that is done elsewhere in the driver, too.
>  
> > You should still return 0 here, as continuing on will send the SAS device
> > page 0 request to the raid volume, which will probably upset the controller
> > enough to stop talking to you.
> Oh well... that explains the hangs - I accidentially dropped the return,
> it should obviously stay and my diff should only add another page fetch.
> 
> With the return the kernel no longer loops in mpii_wait() and boots fine.
> 
> vpg.wwid however is zero;  the other members volume_id, volume_bus, and
> volume_ioc are set/non-zero, guid contains "LSI" and name contains
> "ssd1e" which I how I named this RAID volume in OBP.
> 
> So why are those bits filled in but not the WWID?

I think we have the wrong size for the volume name, hence the difference
between the size reported by the controller and the size of vpg.

try this out?


diff --git sys/dev/pci/mpiireg.h sys/dev/pci/mpiireg.h
index 638f31171d1..11dacf86953 100644
--- sys/dev/pci/mpiireg.h
+++ sys/dev/pci/mpiireg.h
@@ -1355,7 +1355,7 @@ struct mpii_cfg_raid_vol_pg1 {
 
u_int8_tguid[24];
 
-   u_int8_tname[32];
+   u_int8_tname[16];
 
u_int64_t   wwid;
 

Re: sparc64: find root device on hardware RAID

[Klemens Nanni]

On Sun, Dec 29, 2019 at 10:29:48AM +1000, Jonathan Matthew wrote:
> > +   memset(, 0, sizeof(vpg));
> > +   pagelen = hdr.page_length * 4;
> 
> We probably should check that this isn't larger than the size of vpg.
pagelen is 64, sizeof(vpg) is 80.

> link->target isn't the right place to put this, for one thing it's only 16 
> bits
> and the wwn is 64 bits, and it's used throughout the driver to look up devices
> in an array, so changing it will break things.  I think link->port_wwn is the
> right place to store it.
Ah, link->target was there because I played with that in earlier diffs,
port_wwn is indeed correct here.

> The fields in the page returned by the driver are also little endian, so you'd
> need letoh64(vpg.wwid) here.
Thanks, I see that is done elsewhere in the driver, too.
 
> You should still return 0 here, as continuing on will send the SAS device
> page 0 request to the raid volume, which will probably upset the controller
> enough to stop talking to you.
Oh well... that explains the hangs - I accidentially dropped the return,
it should obviously stay and my diff should only add another page fetch.

With the return the kernel no longer loops in mpii_wait() and boots fine.

vpg.wwid however is zero;  the other members volume_id, volume_bus, and
volume_ioc are set/non-zero, guid contains "LSI" and name contains
"ssd1e" which I how I named this RAID volume in OBP.

So why are those bits filled in but not the WWID?

Re: sparc64: find root device on hardware RAID

[Jonathan Matthew]

db_ktrap(101, 20074a0, 1, 0, 0, 2007728) at db_ktrap+0x104
> trap(20074a0, 101, 11e55e4, 820006, 0, 78) at trap+0x2c0
> Lslowtrap_reenter(1, 20077f8, 175adf8, 20077f8, 193f570, cb) at 
> Lslowtrap_reenter+0xf8
> panic(175adf8, 165e310, 40090fae000, 20077f8, 1ca1000, 100) at panic+0xb8
> data_access_fault(20078f0, 31, 165e310, 40090fae000, 40090fae000, 1) at 
> data_access_fault+0x2f0
> sun4v_datatrap(0, 2018000, fff8, 0, 40090ea7ec0, 16) at 
> sun4v_datatrap+0x210
> _kernel_lock(40090ea7ec0, a, 1, ac3b86cb36c, 0, 16) at _kernel_lock+0x34
> scsi_xs_exec(40090ea7ec0, 40090eee710, 1c3, 2007c68, 3f, 78) at 
> scsi_xs_exec+0x30
> scsi_xs_sync(40090ea7ec0, 1c3, 1b0, 0, 193f570, 1c00) at scsi_xs_sync+0x84
> scsi_test_unit_ready(c, 5, 40090ea7ec0, 193f570, 1c00, 40090ee7500) at 
> scsi_test_unit_ready+0x38
> scsi_probedev(40090ee5680, 0, 0, 0, 1c00, 40090ed28c0) at scsi_probedev+0x42c
> scsi_probe(40090ee5680, 0, , 0, 193f570, 73) at 
> scsi_probe+0x98
> 
> https://www.openbsd.org/ddb.html describes the minimum info required in bug
> reports.  Insufficient info makes it difficult to find and fix bugs.
> ddb{0}>
> 
> I need to get more familiar with this code.
> 

Re: sparc64: find root device on hardware RAID

[Klemens Nanni]

On Fri, Dec 27, 2019 at 07:50:34PM +0100, Klemens Nanni wrote:
> Diff below is what I booted last, but for reasons yet unkown to me the
> kernel just hangs afer debug printf()
>
>   ...
>   mpii0 at pci15 dev 0 function 0 "Symbios Logic SAS2008" rev 0x03: msi
>   mpii0: Solana On-Board, firmware 9.0.0.0 IR, MPI 2.0
>   scsibus1 at mpii0: 834 targets
>   mpii_scsi_probe: target 0 lun 0 port_wwn 0 node_wwn 0 has 
> MPII_DF_VOLUME set in flags 10
More specifically, the driver hangs here:

2839 void
2840 mpii_wait(struct mpii_softc *sc, struct mpii_ccb *ccb)
2841 {
2842struct mutexmtx = MUTEX_INITIALIZER(IPL_BIO);
2843void(*done)(struct mpii_ccb *);
2844void*cookie;
2845
2846done = ccb->ccb_done;
2847cookie = ccb->ccb_cookie;
2848
2849ccb->ccb_done = mpii_wait_done;
2850ccb->ccb_cookie = 
2851
2852/* XXX this will wait forever for the ccb to complete */
2853
2854mpii_start(sc, ccb);
2855
2856mtx_enter();
2857while (ccb->ccb_cookie != NULL)
2858msleep(ccb, , PRIBIO, "mpiiwait", 0);
2859mtx_leave();
2860
2861ccb->ccb_cookie = cookie;
2862done(ccb);
2863 }

mpii_start() returns then mpii_wait() "wait[s] forever".  I'm still
practically lost in this area.

Why does it hang there only if I fetch the `struct mpii_cfg_raid_vol_pg1'
page for its wwid member in mpii_scsi_probe() as per the previous diff?

The commit which introduced the XXX:

revision 1.31
date: 2010/07/07 10:29:17;  author: dlg;  state: Exp;  lines: +52 -34;
bring mpi_wait over to mpii for an mpsafe mechanism to sleep while 
waiting
for a command to complete. this also replaces all the while (!ready) \
tsleep() wrapped in splbio code with mpii_wait.

tested with bioctl runs and sensor updates on a raid volume

Re: sparc64: find root device on hardware RAID

[Klemens Nanni]

On Fri, Dec 27, 2019 at 09:46:56AM +0100, Mike Belopuhov wrote:
> Looks like WWID for the RAID volume can be read from the RAID Volume
> Page 1 (mpii_cfg_raid_vol_pg1).
jmatthew also suggested that, thanks.

I'm looking into now mpii(4) and already had a rather naive attempt at
setting the SCSI target to the volume's WWID, but with no avail.

Diff below is what I booted last, but for reasons yet unkown to me the
kernel just hangs afer debug printf()

...
mpii0 at pci15 dev 0 function 0 "Symbios Logic SAS2008" rev 0x03: msi   
   
mpii0: Solana On-Board, firmware 9.0.0.0 IR, MPI 2.0
   
scsibus1 at mpii0: 834 targets  
   
mpii_scsi_probe: target 0 lun 0 port_wwn 0 node_wwn 0 has 
MPII_DF_VOLUME set in flags 10

and the machine must be reset.  So something is wrong with this diff and
I need to get more familiar with this code, but one can also observe
lun->target = vpg.wwid = 0 which I did not expect;  perhaps the driver
currently does not obtain the volume's WWID at all or incorrectly?  Or
perhaps I am missing steps prior to fetching the page;  current mpii(4)
does not seem to use struct mpii_cfg_raid_vol_pg1 at all.

Index: mpii.c
===
RCS file: /cvs/src/sys/dev/pci/mpii.c,v
retrieving revision 1.121
diff -u -p -r1.121 mpii.c
--- mpii.c  12 Sep 2019 22:22:53 -  1.121
+++ mpii.c  27 Dec 2019 14:51:36 -
@@ -909,8 +909,33 @@ mpii_scsi_probe(struct scsi_link *link)
if (ISSET(flags, MPII_DF_HIDDEN) || ISSET(flags, MPII_DF_UNUSED))
return (1);
 
-   if (ISSET(flags, MPII_DF_VOLUME))
-   return (0);
+   if (ISSET(flags, MPII_DF_VOLUME)) {
+   struct mpii_cfg_hdr hdr;
+   struct mpii_cfg_raid_vol_pg1 vpg;
+   size_t pagelen;
+
+   address = MPII_CFG_RAID_VOL_ADDR_HANDLE | dev->dev_handle;
+
+   if (mpii_req_cfg_header(sc, MPII_CONFIG_REQ_PAGE_TYPE_RAID_VOL,
+   1, address, MPII_PG_POLL, ) != 0)
+   return (EINVAL);
+
+   memset(, 0, sizeof(vpg));
+   pagelen = hdr.page_length * 4;
+
+   if (mpii_req_cfg_page(sc, address, MPII_PG_POLL, , 1,
+   , pagelen) != 0)
+   return (EINVAL);
+
+   link->target = vpg.wwid;
+
+   printf("%s: target %x lun %x"
+   " port_wwn %llx node_wwn %llx"
+   " has MPII_DF_VOLUME set in flags %x\n",
+   __func__, link->target, link->lun,
+   link->port_wwn, link->node_wwn,
+   flags);
+   }
 
memset(, 0, sizeof(ehdr));
ehdr.page_type = MPII_CONFIG_REQ_PAGE_TYPE_EXTENDED;


The previous version of this diff put vpg on the heap, e.g.

+   struct mpii_cfg_raid_vol_pg1 *vpg;
+   vpg = malloc(pagelen, M_TEMP, M_WAITOK | M_CANFAIL | M_ZERO);
...

just like it is done for the mpii_cfg_raid_vol_pg0 struct in
mpii_ioctl_cache(), which is where I copied the code from to fetch pages.

But with this, the kernel paniced:

mpii_scsi_probe: target beef lun 0 port_wwn 0 node_wwn 0 has MPII_DF_VOLUME set 
in flags 10
panic: kernel data fault: pc=165e310 addr=40090fae000
panic: Unable to send mondo 1011fa4 to cpu 0: 6
Stopped at  db_enter+0x8:   nop
TIDPIDUID PRFLAGS PFLAGS  CPU  COMMAND
* 0  0  0 0x1  0x2000K swapper
sun4v_send_ipi(0, 1011fa4, 0, 6, 0, 16) at sun4v_send_ipi+0xac
db_enter_ddb(419aa7f8000, a, 1c3, 2007c68, 3f, 78) at db_enter_ddb+0x244
db_ktrap(101, 20074a0, 1, 0, 0, 2007728) at db_ktrap+0x104
trap(20074a0, 101, 11e55e4, 820006, 0, 78) at trap+0x2c0
Lslowtrap_reenter(1, 20077f8, 175adf8, 20077f8, 193f570, cb) at 
Lslowtrap_reenter+0xf8
panic(175adf8, 165e310, 40090fae000, 20077f8, 1ca1000, 100) at panic+0xb8
data_access_fault(20078f0, 31, 165e310, 40090fae000, 40090fae000, 1) at 
data_access_fault+0x2f0
sun4v_datatrap(0, 2018000, fff8, 0, 40090ea7ec0, 16) at 
sun4v_datatrap+0x210
_kernel_lock(40090ea7ec0, a, 1, ac3b86cb36c, 0, 16) at _kernel_lock+0x34
scsi_xs_exec(40090ea7ec0, 40090eee710, 1c3, 2007c68, 3f, 78) at 
scsi_xs_exec+0x30
scsi_xs_sync(40090ea7ec0, 1c3, 1b0, 0, 193f570, 1c00) at scsi_xs_sync+0x84
scsi_test_unit_ready(c, 5, 40090ea7ec0, 193f570, 1c00, 40090ee7500) at 
scsi_test_unit_ready+0x38
scsi_probedev(40090ee5680, 0, 0, 0, 1c00, 40090ed28c0) at scsi_probedev+0x42c
scsi_probe(40090ee5680, 0, , 0, 193f570, 73) at scsi_probe+0x98

https://www.openbsd.org/ddb.html describes the minimum info required in bug
reports.  Insufficient info makes it difficult to find and fix bugs.
ddb{0}>

I need to get more familiar with this code.

Re: sparc64: find root device on hardware RAID

[Mike Belopuhov]

Klemens Nanni writes:

> On Thu, Dec 26, 2019 at 07:49:06PM +0100, Mark Kettenis wrote:
>> Well, there's your problem.  The mpii(4) doesn't fill in the WWNs for
>> the logical volume so there is nothing that can be matched to the WWN
>> from the bootpath.
> Obvious now that you mention it.
>
>> > See below a diff for debug printf() I use to look at thoes values.
>> > Complete console log from OBP prompt to multiuser follows to to show the
>> > boot process and debug output for all devices.
>> > 
>> > What I find odd is how 0aa32290d5dcd16c is the WWID of the RAID volume,
>> > and yet all devices attaching to scsibus* including those not being part
>> > of the RAID show the very same bp->val[0] of 3aa32290d5dcd16c.
>> 
>> bp->val[0] comes from the boot path; there is only one.
> Ha, sure that.  I confused myself with printing it for every device
> passing that code path where it is used as target, hence debug printfs
> showing the same value for multiple devices.
>
>> As you can see, the WWNs are filled in for the other disks (sd1, cd0)
>> that attach to the controller.  So you probably need some additional
>> code in mpii(4) to fill in the WWNs for logical volumes.  I recommend
>> talking to dlg@ and jmatthew@ directly about that.
> That makes sense, I didn't look toward mpii(4) yet.
>
> Thank you for pointing things out and asking such questions, this is
> very very helpful guidance.  I'm looking further into the controller
> driver now.


Looks like WWID for the RAID volume can be read from the RAID Volume
Page 1 (mpii_cfg_raid_vol_pg1).

Cheers,
Mike

Re: sparc64: find root device on hardware RAID

[Klemens Nanni]

On Thu, Dec 26, 2019 at 07:49:06PM +0100, Mark Kettenis wrote:
> Well, there's your problem.  The mpii(4) doesn't fill in the WWNs for
> the logical volume so there is nothing that can be matched to the WWN
> from the bootpath.
Obvious now that you mention it.

> > See below a diff for debug printf() I use to look at thoes values.
> > Complete console log from OBP prompt to multiuser follows to to show the
> > boot process and debug output for all devices.
> > 
> > What I find odd is how 0aa32290d5dcd16c is the WWID of the RAID volume,
> > and yet all devices attaching to scsibus* including those not being part
> > of the RAID show the very same bp->val[0] of 3aa32290d5dcd16c.
> 
> bp->val[0] comes from the boot path; there is only one.
Ha, sure that.  I confused myself with printing it for every device
passing that code path where it is used as target, hence debug printfs
showing the same value for multiple devices.

> As you can see, the WWNs are filled in for the other disks (sd1, cd0)
> that attach to the controller.  So you probably need some additional
> code in mpii(4) to fill in the WWNs for logical volumes.  I recommend
> talking to dlg@ and jmatthew@ directly about that.
That makes sense, I didn't look toward mpii(4) yet.

Thank you for pointing things out and asking such questions, this is
very very helpful guidance.  I'm looking further into the controller
driver now.

Re: sparc64: find root device on hardware RAID

[Mark Kettenis]

> Date: Thu, 26 Dec 2019 19:02:26 +0100
> From: Klemens Nanni 
> 
> On Thu, Dec 26, 2019 at 06:25:10PM +0100, Mark Kettenis wrote:
> > Hmm, you really shouldn't end up there if you're booting by WWN.  I
> > guess the
> > 
> > bp->val[0] == sl->port_wwn
> > 
> > check is failing in your case.  What are the values of:
> > 
> > bp->val[0]
> > sl->port_wwn
> > sl->node_wwn
> > 
> > in your case?
> For the RAID volume sd0:
> 
>   bp->val[0] = 0x3aa32290d5dcd16c
>   sl->port_wwn = 0
>   sl->node_wwn = 0

Well, there's your problem.  The mpii(4) doesn't fill in the WWNs for
the logical volume so there is nothing that can be matched to the WWN
from the bootpath.

> See below a diff for debug printf() I use to look at thoes values.
> Complete console log from OBP prompt to multiuser follows to to show the
> boot process and debug output for all devices.
> 
> What I find odd is how 0aa32290d5dcd16c is the WWID of the RAID volume,
> and yet all devices attaching to scsibus* including those not being part
> of the RAID show the very same bp->val[0] of 3aa32290d5dcd16c.

bp->val[0] comes from the boot path; there is only one.

As you can see, the WWNs are filled in for the other disks (sd1, cd0)
that attach to the controller.  So you probably need some additional
code in mpii(4) to fill in the WWNs for logical volumes.  I recommend
talking to dlg@ and jmatthew@ directly about that.

Cheers,

Mark


P.S. dlg@ and jmatthew@ may also be interested in the following
 unrecognized device:

> vendor "Symbios Logic", unknown product 0x007e (class mass storage subclass 
> SAS, rev 0x03) at pci19 dev 0 function 0 not configured

Re: sparc64: find root device on hardware RAID

[Klemens Nanni]

On Thu, Dec 26, 2019 at 06:25:10PM +0100, Mark Kettenis wrote:
> Hmm, you really shouldn't end up there if you're booting by WWN.  I
> guess the
> 
> bp->val[0] == sl->port_wwn
> 
> check is failing in your case.  What are the values of:
> 
> bp->val[0]
> sl->port_wwn
> sl->node_wwn
> 
> in your case?
For the RAID volume sd0:

bp->val[0] = 0x3aa32290d5dcd16c
sl->port_wwn = 0
sl->node_wwn = 0

See below a diff for debug printf() I use to look at thoes values.
Complete console log from OBP prompt to multiuser follows to to show the
boot process and debug output for all devices.

What I find odd is how 0aa32290d5dcd16c is the WWID of the RAID volume,
and yet all devices attaching to scsibus* including those not being part
of the RAID show the very same bp->val[0] of 3aa32290d5dcd16c.


Index: sparc64/autoconf.c
===
RCS file: /cvs/src/sys/arch/sparc64/sparc64/autoconf.c,v
retrieving revision 1.133
diff -u -p -r1.133 autoconf.c
--- sparc64/autoconf.c  15 Oct 2019 05:21:16 -  1.133
+++ sparc64/autoconf.c  26 Dec 2019 17:47:20 -
@@ -1455,6 +1455,24 @@ device_register(struct device *dev, void
u_int lun = bp->val[1];
 
if (bp->val[0] & 0x && bp->val[0] != -1) {
+   printf("\n%s: RAID:\n"
+   "\tbp->val[]: %lx, %lx, %lx\n"
+   "\ttarget: %x, sl->target: %x, sl->adapter_target: 
%x\n"
+   "\tlun: %x, sl->lun: %x\n"
+   "\tpartition: '%c'\n"
+   "\tsl->port_wwn: %llx\n"
+   "\tsl->node_wwn: %llx\n"
+   "\tsl->id->d_type: %d, sl->id: %s\n",
+   __func__,
+   bp->val[0], bp->val[1], bp->val[2],
+   target, sl->target, sl->adapter_target,
+   lun, sl->lun,
+   (int)bp->val[2],
+   sl->port_wwn,
+   sl->node_wwn,
+   (sl->id == NULL ? -1 : sl->id->d_type), (sl->id == 
NULL ? 0 : (const char *)(sl->id + 1))
+   );
+
/* Fibre channel? */
if (bp->val[0] == sl->port_wwn && lun == sl->lun) {
nail_bootdev(dev, bp);


{0} ok boot
NOTICE: Entering OpenBoot.
NOTICE: Fetching Guest MD from HV.
NOTICE: Starting additional cpus.
NOTICE: Initializing LDC services.
NOTICE: Probing PCI devices.
NOTICE: Finished PCI probing.

SPARC T4-2, No Keyboard
Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights reserved.
OpenBoot 4.38.16, 64. GB memory available, Serial #100254168.
Ethernet address 0:21:28:f9:c1:d8, Host ID: 85f9c1d8.



Boot device: raid  File and args: /bsd.debug
OpenBSD IEEE 1275 Bootblock 1.4
..>> OpenBSD BOOT 1.15

ERROR: /iscsi-hba: No iscsi-network-bootpath property
Booting /pci@400/pci@2/pci@0/pci@e/scsi@0/disk@w3aa32290d5dcd16c,0:a/bsd.debug
9688952@0x100+2184@0x193d778+196104@0x1c0+3998200@0x1c2fe08
symbols @ 0xfe458400 165+625008+428736 start=0x100
[ using 1054936 bytes of bsd ELF symbol table ]
console is /virtual-devices@100/console@1
Copyright (c) 1982, 1986, 1989, 1991, 1993
The Regents of the University of California.  All rights reserved.
Copyright (c) 1995-2019 OpenBSD. All rights reserved.  https://www.OpenBSD.org

OpenBSD 6.6-current (GENERIC.MP) #31: Thu Dec 26 18:47:42 CET 2019
kn@xxx:/sys/arch/sparc64/compile/GENERIC.MP
real mem = 68719476736 (65536MB)
avail mem = 67497238528 (64370MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root: SPARC T4-2
cpu0 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu1 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu2 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu3 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu4 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu5 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu6 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu7 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu8 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu9 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu10 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu11 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu12 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu13 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu14 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu15 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu16 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.862 MHz
cpu17 at mainbus0: SPARC-T4 (rev 0.0) @ 2847.

Re: sparc64: find root device on hardware RAID

[Mark Kettenis]

> Date: Thu, 26 Dec 2019 17:55:24 +0100
> From: Klemens Nanni 
> 
> On Thu, Dec 26, 2019 at 09:50:16AM +0100, Mark Kettenis wrote:
> > What happens if the bootpath doesn't specify a partition?  Currently
> > we end up with bp->val[2] being zero in that case, which means we end
> > up booting from 'a' which is the default partition.  So I think you
> > need to check for that case when you call setroot().
> Right, if no partition is specified val[2] is zero - for both current
> code as well as with my diff.
> 
> So in setroot() I should have checked whether val[2] was set.
> 
> Note how the current code accounts for bp being NULL and sets bootdv
> accordingly, but then uses bp->val[2] unconditionally.  This looks like
> a potential NULL dereference, perhaps noone has ever hit it?
> 
>   bp = nbootpath == 0 ? NULL : [nbootpath-1];
>   bootdv = (bp == NULL) ? NULL : bp->dev;
> 
> #if NMPATH > 0
>   if (bootdv != NULL)
>   bootdv = mpath_bootdv(bootdv);
> #endif
> 
>   setroot(bootdv, bp->val[2], RB_USERREQ | RB_HALT);
> 
> > However, that means the partition != 0 check is probably meaningless...
> Indeed, when omitting the partition OBP passes the bootpath as is
> 
>   {0} ok boot /pci@400/pci@2/pci@0/pci@e/scsi@0/disk@w3aa32290d5dcd16c,0 
> /bsd.debug
>   ...
>   Boot device: /pci@400/pci@2/pci@0/pci@e/scsi@0/disk@w3aa32290d5dcd16c,0 
>  File and args: /bsd.debug
>   
> 
> but stand/ofwboot/ofdev.c:devopen() will default to "a"
> 
>   OpenBSD IEEE 1275 Bootblock 1.4 
> 
>   ..>> OpenBSD BOOT 1.15  
> 
> 
>   ERROR: /iscsi-hba: No iscsi-network-bootpath property   
> 
>   Booting 
> /pci@400/pci@2/pci@0/pci@e/scsi@0/disk@w3aa32290d5dcd16c,0:a/bsd.debug  
> 
> So the kernel will always see a partition with such given bootpath,
> hence the val[2] check is always true in that place.
> 
> > Also note that booting by WWWN isn't just for fibre-channel and
> > hardware RAID.  It works for single disks as well on some controllers.
> Ah, OK.  I errornuously implied this from reading the code.
> 
> > Why does matching the LUN fail?  If I read the code correctly, you'll
> > end up with bp->val[1] = 0.  What is the value of sl->lun?
> the LUN always matches, but the Target does not.  Both lun = bp->val[1]
> and sl->lun are zero.
> 
> However, the last check
> 
>   if (bp->val[0] & 0x && bp->val[0] != -1) { 
>   ...
> 
>   if (target == sl->target && lun == sl->lun) {   
>
>   nail_bootdev(dev, bp);  
>
>   return; 
>
>   }   
>
>   }
> 
> fails because of target mismatch.
> 
> On my sd0 being the RAID volume I see target = bp->val[2] being
> 0xd5dcd16c (lower 16 bit of WWID) while sl->target is zero.

Hmm, you really shouldn't end up there if you're booting by WWN.  I
guess the

bp->val[0] == sl->port_wwn

check is failing in your case.  What are the values of:

bp->val[0]
sl->port_wwn
sl->node_wwn

in your case?

Re: sparc64: find root device on hardware RAID

[Klemens Nanni]

On Thu, Dec 26, 2019 at 09:50:16AM +0100, Mark Kettenis wrote:
> What happens if the bootpath doesn't specify a partition?  Currently
> we end up with bp->val[2] being zero in that case, which means we end
> up booting from 'a' which is the default partition.  So I think you
> need to check for that case when you call setroot().
Right, if no partition is specified val[2] is zero - for both current
code as well as with my diff.

So in setroot() I should have checked whether val[2] was set.

Note how the current code accounts for bp being NULL and sets bootdv
accordingly, but then uses bp->val[2] unconditionally.  This looks like
a potential NULL dereference, perhaps noone has ever hit it?

bp = nbootpath == 0 ? NULL : [nbootpath-1];
bootdv = (bp == NULL) ? NULL : bp->dev;

#if NMPATH > 0
if (bootdv != NULL)
bootdv = mpath_bootdv(bootdv);
#endif

setroot(bootdv, bp->val[2], RB_USERREQ | RB_HALT);

> However, that means the partition != 0 check is probably meaningless...
Indeed, when omitting the partition OBP passes the bootpath as is

{0} ok boot /pci@400/pci@2/pci@0/pci@e/scsi@0/disk@w3aa32290d5dcd16c,0 
/bsd.debug
...
Boot device: /pci@400/pci@2/pci@0/pci@e/scsi@0/disk@w3aa32290d5dcd16c,0 
 File and args: /bsd.debug  


but stand/ofwboot/ofdev.c:devopen() will default to "a"

OpenBSD IEEE 1275 Bootblock 1.4 

..>> OpenBSD BOOT 1.15  


ERROR: /iscsi-hba: No iscsi-network-bootpath property   

Booting 
/pci@400/pci@2/pci@0/pci@e/scsi@0/disk@w3aa32290d5dcd16c,0:a/bsd.debug  

So the kernel will always see a partition with such given bootpath,
hence the val[2] check is always true in that place.

> Also note that booting by WWWN isn't just for fibre-channel and
> hardware RAID.  It works for single disks as well on some controllers.
Ah, OK.  I errornuously implied this from reading the code.

> Why does matching the LUN fail?  If I read the code correctly, you'll
> end up with bp->val[1] = 0.  What is the value of sl->lun?
the LUN always matches, but the Target does not.  Both lun = bp->val[1]
and sl->lun are zero.

However, the last check

if (bp->val[0] & 0x && bp->val[0] != -1) { 
...

if (target == sl->target && lun == sl->lun) {   
   
nail_bootdev(dev, bp);  
   
return; 
   
}   
   
}

fails because of target mismatch.

On my sd0 being the RAID volume I see target = bp->val[2] being
0xd5dcd16c (lower 16 bit of WWID) while sl->target is zero.

Re: sparc64: find root device on hardware RAID

[Mark Kettenis]

> Date: Thu, 26 Dec 2019 07:38:47 +0100
> From: Klemens Nanni 
> 
> Hardware RAID volume bootpaths are similar to Fibre channel ones in that
> they use the disk's WWID (lower 16 bit only) as SCSI target, e.g.
> 
>   /pci@400/pci@2/pci@0/pci@e/scsi@0/disk@w3aa32290d5dcd16c,0:a
> 
> Currently device_register() does recognise devices with their WWN as
> target but only continues to nail_bootdev() them if their lun and port
> match as well.
> 
> Diff below makes device_register() look at the bootpath's partition and
> --in lack of a better criterium-- the device name as well to test for
> hardware RAIDs.
> 
> But to be able to match the partition, partition index calculation must
> be deferred from bootpath_build() to diskconf() so it becomes possible
> to check whether partition "a" (index 0) has been specified in the
> bootpath - this is currently not possible because the partition gets
> turned into an index right away which makes it indistinguishable from
> val[2]'s default initialised value.
> 
> As a bonus, this now makes also makes the kernel print back the bootpath
> correctly with regard to "a" partitions:
> 
>   bootpath: 
> /pci@400,0/pci@2,0/pci@0,0/pci@e,0/scsi@0,0/disk@3aa32290d5dcd16c,0:a
>   root on sd0a (5eb46f4312eeecb7.a) swap on sd0b dump on sd0b
> 
> 
> Is there a better way to detect hardware RAIDs as such?
> 
> 
> Index: sparc64/autoconf.c
> ===
> RCS file: /cvs/src/sys/arch/sparc64/sparc64/autoconf.c,v
> retrieving revision 1.133
> diff -u -p -r1.133 autoconf.c
> --- sparc64/autoconf.c15 Oct 2019 05:21:16 -  1.133
> +++ sparc64/autoconf.c26 Dec 2019 05:56:20 -
> @@ -525,7 +525,7 @@ bootpath_build(void)
>* be an ethernet media specification, so be
>* sure to skip all letters.
>*/
> - bp->val[2] = *++cp - 'a';
> + bp->val[2] = *++cp;
>   while (*cp != '\0' && *cp != '/')
>   cp++;
>   }
> @@ -611,7 +611,7 @@ bootpath_print(struct bootpath *bp)
>   else
>   printf("/%s@%lx,%lx", bp->name, bp->val[0], bp->val[1]);
>   if (bp->val[2] != 0)
> - printf(":%c", (int)bp->val[2] + 'a');
> + printf(":%c", (int)bp->val[2]);
>   bp++;
>   }
>   printf("\n");
> @@ -813,7 +813,7 @@ diskconf(void)
>   bootdv = mpath_bootdv(bootdv);
>  #endif
>  
> - setroot(bootdv, bp->val[2], RB_USERREQ | RB_HALT);
> + setroot(bootdv, bp->val[2] - 'a', RB_USERREQ | RB_HALT);
>   dumpconf();
>  }
>  
> @@ -1453,7 +1453,9 @@ device_register(struct device *dev, void
>   (struct scsibus_softc *)dev->dv_parent;
>   u_int target = bp->val[0];
>   u_int lun = bp->val[1];
> + int partition = bp->val[2];
>  
> + /* Is target a full WWN/WWID? */
>   if (bp->val[0] & 0x && bp->val[0] != -1) {
>   /* Fibre channel? */
>   if (bp->val[0] == sl->port_wwn && lun == sl->lun) {
> @@ -1468,6 +1470,12 @@ device_register(struct device *dev, void
>   sl->id->d_type == DEVID_NAA &&
>   memcmp(sl->id + 1, >val[0], 8) == 0)
>   nail_bootdev(dev, bp);
> +
> + /* Hardware RAID? */
> + /* XXX: how to detect properly? */
> + if (strcmp(devname, "sd") == 0 && partition != 0) {
> + nail_bootdev(dev, bp);
> + }
>   return;

What happens if the bootpath doesn't specify a partition?  Currently
we end up with bp->val[2] being zero in that case, which means we end
up booting from 'a' which is the default partition.  So I think you
need to check for that case when you call setroot().

However, that means the partition != 0 check is probably meaningless...

Also note that booting by WWWN isn't just for fibre-channel and
hardware RAID.  It works for single disks as well on some controllers.

Why does matching the LUN fail?  If I read the code correctly, you'll
end up with bp->val[1] = 0.  What is the value of sl->lun?

sparc64: find root device on hardware RAID

[Klemens Nanni]

Hardware RAID volume bootpaths are similar to Fibre channel ones in that
they use the disk's WWID (lower 16 bit only) as SCSI target, e.g.

/pci@400/pci@2/pci@0/pci@e/scsi@0/disk@w3aa32290d5dcd16c,0:a

Currently device_register() does recognise devices with their WWN as
target but only continues to nail_bootdev() them if their lun and port
match as well.

Diff below makes device_register() look at the bootpath's partition and
--in lack of a better criterium-- the device name as well to test for
hardware RAIDs.

But to be able to match the partition, partition index calculation must
be deferred from bootpath_build() to diskconf() so it becomes possible
to check whether partition "a" (index 0) has been specified in the
bootpath - this is currently not possible because the partition gets
turned into an index right away which makes it indistinguishable from
val[2]'s default initialised value.

As a bonus, this now makes also makes the kernel print back the bootpath
correctly with regard to "a" partitions:

bootpath: 
/pci@400,0/pci@2,0/pci@0,0/pci@e,0/scsi@0,0/disk@3aa32290d5dcd16c,0:a
root on sd0a (5eb46f4312eeecb7.a) swap on sd0b dump on sd0b


Is there a better way to detect hardware RAIDs as such?


Index: sparc64/autoconf.c
===
RCS file: /cvs/src/sys/arch/sparc64/sparc64/autoconf.c,v
retrieving revision 1.133
diff -u -p -r1.133 autoconf.c
--- sparc64/autoconf.c  15 Oct 2019 05:21:16 -  1.133
+++ sparc64/autoconf.c  26 Dec 2019 05:56:20 -
@@ -525,7 +525,7 @@ bootpath_build(void)
 * be an ethernet media specification, so be
 * sure to skip all letters.
 */
-   bp->val[2] = *++cp - 'a';
+   bp->val[2] = *++cp;
while (*cp != '\0' && *cp != '/')
cp++;
}
@@ -611,7 +611,7 @@ bootpath_print(struct bootpath *bp)
else
printf("/%s@%lx,%lx", bp->name, bp->val[0], bp->val[1]);
if (bp->val[2] != 0)
-   printf(":%c", (int)bp->val[2] + 'a');
+   printf(":%c", (int)bp->val[2]);
bp++;
}
printf("\n");
@@ -813,7 +813,7 @@ diskconf(void)
bootdv = mpath_bootdv(bootdv);
 #endif
 
-   setroot(bootdv, bp->val[2], RB_USERREQ | RB_HALT);
+   setroot(bootdv, bp->val[2] - 'a', RB_USERREQ | RB_HALT);
dumpconf();
 }
 
@@ -1453,7 +1453,9 @@ device_register(struct device *dev, void
(struct scsibus_softc *)dev->dv_parent;
u_int target = bp->val[0];
u_int lun = bp->val[1];
+   int partition = bp->val[2];
 
+   /* Is target a full WWN/WWID? */
if (bp->val[0] & 0x && bp->val[0] != -1) {
/* Fibre channel? */
if (bp->val[0] == sl->port_wwn && lun == sl->lun) {
@@ -1468,6 +1470,12 @@ device_register(struct device *dev, void
sl->id->d_type == DEVID_NAA &&
memcmp(sl->id + 1, >val[0], 8) == 0)
nail_bootdev(dev, bp);
+
+   /* Hardware RAID? */
+   /* XXX: how to detect properly? */
+   if (strcmp(devname, "sd") == 0 && partition != 0) {
+   nail_bootdev(dev, bp);
+   }
return;
}
 

Re: find.1: Use -delete in EXAMPLES

[Todd C . Miller]

On Thu, 22 Aug 2019 19:20:53 +0200, Klemens Nanni wrote:

> I concur.  We can at least showcase the "batching" version that passes
> multiple arguments to rm(1) while also mentioning `-delete'.
>
> Both find(1)'s `-print0' and xarg(1)'s `-0' reference each other, I'd
> say that is enough together with find(1)'s CAVEATS such that
> `find -print0 | xargs -0' does not have to me shown in EXAMPLES again.

OK millert@

 - todd

Re: find.1: Use -delete in EXAMPLES

[Klemens Nanni]

On Thu, Aug 22, 2019 at 11:00:57AM -0600, Todd C. Miller wrote:
> The point of that example is to show how to safely use xargs.  Since
> find now has its own built-in xargs support perhaps we should adapt
> the example to use that instead.
As claudio and tb already pointed out, that is indeed a valid point.

> We can also list the -delete method as an alternative.  E.g.
I concur.  We can at least showcase the "batching" version that passes
multiple arguments to rm(1) while also mentioning `-delete'.

Both find(1)'s `-print0' and xarg(1)'s `-0' reference each other, I'd
say that is enough together with find(1)'s CAVEATS such that
`find -print0 | xargs -0' does not have to me shown in EXAMPLES again.

How about that?

Index: find.1
===
RCS file: /cvs/src/usr.bin/find/find.1,v
retrieving revision 1.96
diff -u -p -r1.96 find.1
--- find.1  6 Dec 2018 17:45:14 -0000   1.96
+++ find.1  22 Aug 2019 17:16:32 -
@@ -581,9 +581,9 @@ ending in a dot and single digit, but sk
 Find and remove all *.jpg and *.gif files under the current working
 directory:
 .Pp
-.Dl "$ find . \e( -name \e*.jpg -o -name \e*.gif \e) -exec rm {} \e;"
+.Dl "$ find . \e( -name \e*.jpg -o -name \e*.gif \e) -exec rm {} +"
 or
-.Dl "$ find . \e( -name \e*.jpg -o -name \e*.gif \e) -print0 | xargs -0r rm"
+.Dl "$ find . \e( -name \e*.jpg -o -name \e*.gif \e) -delete"
 .Sh SEE ALSO
 .Xr chflags 1 ,
 .Xr chmod 1 ,

Re: find.1: Use -delete in EXAMPLES

[Todd C . Miller]

On Thu, 22 Aug 2019 11:10:43 -0600, "Theo de Raadt" wrote:

> Todd C. Miller  wrote:
>
> > On Thu, 22 Aug 2019 11:06:12 -0600, "Theo de Raadt" wrote:
> > 
> > > Todd C. Miller  wrote:
> > >
> > > > The point of that example is to show how to safely use xargs.  Since
> > > > find now has its own built-in xargs support perhaps we should adapt
> > > > the example to use that instead.
> > >
> > > Does it not matter that the xargs solution is maximally portable in
> > > scripts, but the builtin isn't (widely implimented... but...)
> > 
> > I think these days the builtin xargs is more portable than the
> > non-standard -print0 option.
>
> That sounds weird.  -print0 is really old.

It was originally a GNUism but it never made it into POSIX.  We
added it a long time ago but I don't see it in Solaris or AIX.
Surprisingly, HP-UX supports it.

The built-in xargs was added to find instead in POSIX around 2001
or so.  There is some history at:
https://collaboration.opengroup.org/external/pasc.org/interpretations/unofficial/db/p1003.2/pasc-1003.2-210.html

As far as modern systems go, the built-in xargs is more portable.

 - todd

Re: find.1: Use -delete in EXAMPLES

[Theo de Raadt]

Todd C. Miller  wrote:

> On Thu, 22 Aug 2019 11:06:12 -0600, "Theo de Raadt" wrote:
> 
> > Todd C. Miller  wrote:
> >
> > > The point of that example is to show how to safely use xargs.  Since
> > > find now has its own built-in xargs support perhaps we should adapt
> > > the example to use that instead.
> >
> > Does it not matter that the xargs solution is maximally portable in
> > scripts, but the builtin isn't (widely implimented... but...)
> 
> I think these days the builtin xargs is more portable than the
> non-standard -print0 option.

That sounds weird.  -print0 is really old.

Re: find.1: Use -delete in EXAMPLES

[Todd C . Miller]

On Thu, 22 Aug 2019 11:06:12 -0600, "Theo de Raadt" wrote:

> Todd C. Miller  wrote:
>
> > The point of that example is to show how to safely use xargs.  Since
> > find now has its own built-in xargs support perhaps we should adapt
> > the example to use that instead.
>
> Does it not matter that the xargs solution is maximally portable in
> scripts, but the builtin isn't (widely implimented... but...)

I think these days the builtin xargs is more portable than the
non-standard -print0 option.

 - todd

Re: find.1: Use -delete in EXAMPLES

[Theo de Raadt]

Todd C. Miller  wrote:

> The point of that example is to show how to safely use xargs.  Since
> find now has its own built-in xargs support perhaps we should adapt
> the example to use that instead.

Does it not matter that the xargs solution is maximally portable in
scripts, but the builtin isn't (widely implimented... but...)

Re: find.1: Use -delete in EXAMPLES

[Todd C . Miller]

The point of that example is to show how to safely use xargs.  Since
find now has its own built-in xargs support perhaps we should adapt
the example to use that instead.

We can also list the -delete method as an alternative.  E.g.

$ find . \( -name \*.jpg -o -name \*.gif \) -exec rm {} +

or

$ find . \( -name \*.jpg -o -name \*.gif \) -delete

 - todd

Re: find.1: Use -delete in EXAMPLES

[Andreas Kusalananda Kähäri]

On Thu, Aug 22, 2019 at 12:37:28PM +0200, Klemens Nanni wrote:
> We support -delete since the following and I see no reason to prefer the
> current examples for the very same reasons tedu already outlined:
> 
>   find.c revision 1.21
>   date: 2017/01/03 21:31:16;  author: tedu;  state: Exp;  lines: +10 -4;
>   add -delete option which can simplify the common case of wanting to 
> delete
>   lots of files without the arcane -exec or error prone xargs.
>   code from freebsd.
>   ok millert
> 
> CAVEATS even goes into detail wrt. removing special files, so no
> information seems to be lost with this diff.
> 
> Feedback? OK?

You may want to single quote the patterns as '*.jpg' and '*.gif' as is
done a few lines further up in the manual. Also, since the text says
"files" (assuming this means "regular files"), one may want to add -type f
to the tests in the command.


Regards,

> 
> Index: find.1
> =======
> RCS file: /cvs/src/usr.bin/find/find.1,v
> retrieving revision 1.96
> diff -u -p -r1.96 find.1
> --- find.16 Dec 2018 17:45:14 -   1.96
> +++ find.122 Aug 2019 10:31:14 -
> @@ -581,9 +581,7 @@ ending in a dot and single digit, but sk
>  Find and remove all *.jpg and *.gif files under the current working
>  directory:
>  .Pp
> -.Dl "$ find . \e( -name \e*.jpg -o -name \e*.gif \e) -exec rm {} \e;"
> -or
> -.Dl "$ find . \e( -name \e*.jpg -o -name \e*.gif \e) -print0 | xargs -0r rm"
> +.Dl "$ find . \e( -name \e*.jpg -o -name \e*.gif \e) -delete"
>  .Sh SEE ALSO
>  .Xr chflags 1 ,
>  .Xr chmod 1 ,

Re: find.1: Use -delete in EXAMPLES

[Claudio Jeker]

On Thu, Aug 22, 2019 at 12:37:28PM +0200, Klemens Nanni wrote:
> We support -delete since the following and I see no reason to prefer the
> current examples for the very same reasons tedu already outlined:
> 
>   find.c revision 1.21
>   date: 2017/01/03 21:31:16;  author: tedu;  state: Exp;  lines: +10 -4;
>   add -delete option which can simplify the common case of wanting to 
> delete
>   lots of files without the arcane -exec or error prone xargs.
>   code from freebsd.
>   ok millert
> 
> CAVEATS even goes into detail wrt. removing special files, so no
> information seems to be lost with this diff.
> 
> Feedback? OK?

I think having those examples there is useful since the executed command
may be something different than rm. In my opinion examples are there
to show how to use more complex idioms safely and not just to show the
quickest way.
 
> Index: find.1
> ===
> RCS file: /cvs/src/usr.bin/find/find.1,v
> retrieving revision 1.96
> diff -u -p -r1.96 find.1
> --- find.16 Dec 2018 17:45:14 -   1.96
> +++ find.122 Aug 2019 10:31:14 -
> @@ -581,9 +581,7 @@ ending in a dot and single digit, but sk
>  Find and remove all *.jpg and *.gif files under the current working
>  directory:
>  .Pp
> -.Dl "$ find . \e( -name \e*.jpg -o -name \e*.gif \e) -exec rm {} \e;"
> -or
> -.Dl "$ find . \e( -name \e*.jpg -o -name \e*.gif \e) -print0 | xargs -0r rm"
> +.Dl "$ find . \e( -name \e*.jpg -o -name \e*.gif \e) -delete"
>  .Sh SEE ALSO
>  .Xr chflags 1 ,
>  .Xr chmod 1 ,
> 

-- 
:wq Claudio

Re: find.1: Use -delete in EXAMPLES

[Theo Buehler]

On Thu, Aug 22, 2019 at 12:37:28PM +0200, Klemens Nanni wrote:
> We support -delete since the following and I see no reason to prefer the
> current examples for the very same reasons tedu already outlined:
> 
>   find.c revision 1.21
>   date: 2017/01/03 21:31:16;  author: tedu;  state: Exp;  lines: +10 -4;
>   add -delete option which can simplify the common case of wanting to 
> delete
>   lots of files without the arcane -exec or error prone xargs.
>   code from freebsd.
>   ok millert
> 
> CAVEATS even goes into detail wrt. removing special files, so no
> information seems to be lost with this diff.

I think it's mostly a matter of portability but also of taste.

If I remember correctly, doing something like what you suggest (and
other tweaks to the examples) was discussed and rejected  at the time
the -delete option was added:
https://marc.info/?l=openbsd-tech=148342051832692=2

find.1: Use -delete in EXAMPLES

[Klemens Nanni]

We support -delete since the following and I see no reason to prefer the
current examples for the very same reasons tedu already outlined:

find.c revision 1.21
date: 2017/01/03 21:31:16;  author: tedu;  state: Exp;  lines: +10 -4;
add -delete option which can simplify the common case of wanting to 
delete
lots of files without the arcane -exec or error prone xargs.
code from freebsd.
ok millert

CAVEATS even goes into detail wrt. removing special files, so no
information seems to be lost with this diff.

Feedback? OK?

Index: find.1
===
RCS file: /cvs/src/usr.bin/find/find.1,v
retrieving revision 1.96
diff -u -p -r1.96 find.1
--- find.1  6 Dec 2018 17:45:14 -   1.96
+++ find.1  22 Aug 2019 10:31:14 -
@@ -581,9 +581,7 @@ ending in a dot and single digit, but sk
 Find and remove all *.jpg and *.gif files under the current working
 directory:
 .Pp
-.Dl "$ find . \e( -name \e*.jpg -o -name \e*.gif \e) -exec rm {} \e;"
-or
-.Dl "$ find . \e( -name \e*.jpg -o -name \e*.gif \e) -print0 | xargs -0r rm"
+.Dl "$ find . \e( -name \e*.jpg -o -name \e*.gif \e) -delete"
 .Sh SEE ALSO
 .Xr chflags 1 ,
 .Xr chmod 1 ,

Re: find -not

[Todd C. Miller]

On Wed, 05 Dec 2018 22:52:13 -0500, "Ted Unangst" wrote:

> Seen in the wild. Alias for ! that's friendlier to the shell.

OK millert@

 - todd

find -not

[Ted Unangst]

Seen in the wild. Alias for ! that's friendlier to the shell.

Index: find.1
===
RCS file: /cvs/src/usr.bin/find/find.1,v
retrieving revision 1.95
diff -u -p -r1.95 find.1
--- find.1  1 Aug 2018 07:09:15 -   1.95
+++ find.1  6 Dec 2018 03:50:10 -
@@ -524,6 +524,7 @@ This evaluates to true if the parenthesi
 true.
 .Pp
 .It Cm \&! Ar expression
+.It Cm -not Ar expression
 This is the unary NOT operator.
 It evaluates to true if the expression is false.
 .Pp
@@ -624,9 +625,10 @@ primaries
 and
 .Ic -print0 ,
 and operators
-.Fl or
-and
 .Fl and ,
+.Fl not ,
+and
+.Fl or ,
 are extensions to that specification.
 .Pp
 Historically, the
Index: option.c
===
RCS file: /cvs/src/usr.bin/find/option.c,v
retrieving revision 1.20
diff -u -p -r1.20 option.c
--- option.c3 Jan 2017 21:31:16 -   1.20
+++ option.c6 Dec 2018 03:44:12 -
@@ -80,6 +80,7 @@ static OPTION options[] = {
{ "-name",  N_NAME, c_name, O_ARGV },
{ "-newer", N_NEWER,c_newer,O_ARGV },
{ "-nogroup",   N_NOGROUP,  c_nogroup,  O_ZERO },
+   { "-not",   N_NOT,  c_not,  O_ZERO },
{ "-nouser",N_NOUSER,   c_nouser,   O_ZERO },
{ "-o", N_OR,   c_or,   O_ZERO },
{ "-ok",N_OK,   c_exec, O_ARGVP },

Re: [PATCH] find(1) man page, source comment: assumed -print

[Theo Buehler]

On Wed, Aug 01, 2018 at 07:19:13AM +0100, Jason McIntyre wrote:
> On Mon, Jul 30, 2018 at 05:19:40PM -0500, Kris Katterjohn wrote:
> > Hey,
> > 
> > A statement in the find(1) man page and a source comment in find.c are
> > outdated and incorrect.
> > 
> > The man page didn't mention that using -delete or -execdir prevents
> > -print from being assumed.
> > 
> > Similarly for a comment in find.c, but this didn't mention -delete,
> > -execdir, -ls or -print0.
> > 
> > (These primaries can be tested and/or you can see where isoutput is set
> > to 1 in function.c.)
> > 
> > I have a patch below to correct these.
> > 
> > Cheers,
> > Kris Katterjohn
> > 
> 
> morning.
> 
> ok by me. anyone want to pick this up, or ok it?

committed, thanks!

> jmc
> 
> > Index: find.1
> > =======
> > RCS file: /cvs/src/usr.bin/find/find.1,v
> > retrieving revision 1.93
> > diff -u -p -r1.93 find.1
> > --- find.1  3 Jan 2017 22:19:31 -   1.93
> > +++ find.1  30 Jul 2018 21:28:36 -
> > @@ -60,7 +60,9 @@ In the absence of an expression,
> >  is assumed.
> >  If an expression is given,
> >  but none of the primaries
> > +.Ic -delete ,
> >  .Ic -exec ,
> > +.Ic -execdir ,
> >  .Ic -ls ,
> >  .Ic -ok ,
> >  .Ic -print ,
> > Index: find.c
> > ===
> > RCS file: /cvs/src/usr.bin/find/find.c,v
> > retrieving revision 1.22
> > diff -u -p -r1.22 find.c
> > --- find.c  4 Jan 2017 09:21:26 -   1.22
> > +++ find.c  30 Jul 2018 21:28:36 -
> > @@ -86,9 +86,10 @@ find_formplan(char **argv)
> > }
> >  
> > /*
> > -* if the user didn't specify one of -print, -ok or -exec, then -print
> > -* is assumed so we bracket the current expression with parens, if
> > -* necessary, and add a -print node on the end.
> > +* if the user didn't specify one of -delete, -exec, -execdir,
> > +* -ls, -ok, -print or -print0, then -print is assumed so we
> > +* bracket the current expression with parens, if necessary,
> > +* and add a -print node on the end.
> >  */
> > if (!isoutput) {
> > if (plan == NULL) {
> > 
> 

[PATCH] find(1) man page, source comment: assumed -print

[Kris Katterjohn]

Hey,

A statement in the find(1) man page and a source comment in find.c are
outdated and incorrect.

The man page didn't mention that using -delete or -execdir prevents
-print from being assumed.

Similarly for a comment in find.c, but this didn't mention -delete,
-execdir, -ls or -print0.

(These primaries can be tested and/or you can see where isoutput is set
to 1 in function.c.)

I have a patch below to correct these.

Cheers,
Kris Katterjohn

Index: find.1
===
RCS file: /cvs/src/usr.bin/find/find.1,v
retrieving revision 1.93
diff -u -p -r1.93 find.1
--- find.1  3 Jan 2017 22:19:31 -   1.93
+++ find.1  30 Jul 2018 21:28:36 -
@@ -60,7 +60,9 @@ In the absence of an expression,
 is assumed.
 If an expression is given,
 but none of the primaries
+.Ic -delete ,
 .Ic -exec ,
+.Ic -execdir ,
 .Ic -ls ,
 .Ic -ok ,
 .Ic -print ,
Index: find.c
===
RCS file: /cvs/src/usr.bin/find/find.c,v
retrieving revision 1.22
diff -u -p -r1.22 find.c
--- find.c  4 Jan 2017 09:21:26 -   1.22
+++ find.c  30 Jul 2018 21:28:36 -
@@ -86,9 +86,10 @@ find_formplan(char **argv)
}
 
/*
-* if the user didn't specify one of -print, -ok or -exec, then -print
-* is assumed so we bracket the current expression with parens, if
-* necessary, and add a -print node on the end.
+* if the user didn't specify one of -delete, -exec, -execdir,
+* -ls, -ok, -print or -print0, then -print is assumed so we
+* bracket the current expression with parens, if necessary,
+* and add a -print node on the end.
 */
if (!isoutput) {
if (plan == NULL) {

[PATCH] find(1) man page: -exec evaluation

[Kris Katterjohn]

Hey,

The man page for find(1) does not mention when the -exec primary
evaluates to true.

-exec utility ... ; evaluates to true when the utility exits with a
zero exit status, while -exec utility ... {} + always evaluates to true.

I have a patch below with my attempt at a description.  I tried to make
the wording consistent with other parts of the man page.

Cheers,
Kris Katterjohn

Index: find.1
===
RCS file: /cvs/src/usr.bin/find/find.1,v
retrieving revision 1.93
diff -u -p -r1.93 find.1
--- find.1  3 Jan 2017 22:19:31 -   1.93
+++ find.1  30 Jul 2018 19:08:16 -
@@ -222,6 +222,10 @@ or a plus sign
 If terminated by a semicolon, the
 .Ar utility
 is executed once per path.
+This form of the primary evaluates to true if the invocation
+of
+.Ar utility
+exits with a zero exit status.
 If the string
 .Qq {}
 appears anywhere in the utility name or the
@@ -233,6 +237,7 @@ primary is evaluated are aggregated into
 .Ar utility
 will be invoked once per set, similar to
 .Xr xargs 1 .
+This form of the primary always evaluates to true.
 If any invocation exits with a non-zero exit status, then
 .Nm
 will eventually do so as well, but this does not cause

Re: make includes: use find -exec {} +

[Christian Weisgerber]

Christian Weisgerber:

> Admittedly, this is cosmetic:
> Use the modern POSIX idiom "-exec ... {} +" instead of find|xargs.

tb@ kindly pointed out to me that Klemens Nanni submitted a better
patch for this four months ago; "-exec +" allows us to combine the
two find invocations into one:
https://marc.info/?l=openbsd-tech=149649423503939

ok?

Index: include/Makefile
===
RCS file: /cvs/src/include/Makefile,v
retrieving revision 1.219
diff -u -p -r1.219 Makefile
--- include/Makefile17 Apr 2017 15:53:21 -  1.219
+++ include/Makefile6 Oct 2017 14:25:47 -
@@ -100,10 +100,9 @@ includes:
cd ${.CURDIR}/$$i && ${RUN_MAKE}; \
done
chown -RP ${BINOWN}:${BINGRP} ${DESTDIR}/usr/include
-   find ${DESTDIR}/usr/include -type f -print0 | \
-   xargs -0r chmod a=r
-   find ${DESTDIR}/usr/include \( -type d -o -type l \) -print0 | \
-   xargs -0r chmod -h u=rwx,go=rx
+   find ${DESTDIR}/usr/include \
+   -type f -exec chmod a=r {} + -o \
+   \( -type d -o -type l \) -exec chmod -h u=rwx,go=rx {} +
 
 copies:
@echo copies: ${LDIRS}
Index: share/zoneinfo/Makefile
===
RCS file: /cvs/src/share/zoneinfo/Makefile,v
retrieving revision 1.11
diff -u -p -r1.11 Makefile
--- share/zoneinfo/Makefile 24 Nov 2016 14:38:30 -  1.11
+++ share/zoneinfo/Makefile 6 Oct 2017 14:30:23 -
@@ -81,8 +81,9 @@ realinstall: ${DATA} ${REDO} ${YEARISTYP
(cd ${.CURDIR}/datfiles; \
${ZIC} -y ${YEARISTYPECOPY} -d ${TZDIR} -p ${POSIXRULES})
chown -R ${BINOWN}:${BINGRP} ${TZDIR}
-   find ${TZDIR} -type f -print0 | xargs -0r chmod a=r
-   find ${TZDIR} -type d -print0 | xargs -0r chmod a=rx
+   find ${TZDIR} \
+   -type f -exec chmod a=r {} + -o \
+   -type d -exec chmod a=rx {} +
${INSTALL} -c -o root -g bin -m 644 ${.CURDIR}/datfiles/iso3166.tab \
${DESTDIR}/usr/share/misc
${INSTALL} -c -o root -g bin -m 644 ${.CURDIR}/datfiles/zone.tab \

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de

Re: make includes: use find -exec {} +

[Klemens Nanni]

On Thu, Oct 05, 2017 at 08:08:32PM +, Christian Weisgerber wrote:
> Admittedly, this is cosmetic:
> Use the modern POSIX idiom "-exec ... {} +" instead of find|xargs.
> ok?
> 
> Index: include/Makefile
> ===
> RCS file: /cvs/src/include/Makefile,v
> retrieving revision 1.219
> diff -u -p -r1.219 Makefile
> --- include/Makefile  17 Apr 2017 15:53:21 -  1.219
> +++ include/Makefile  5 Oct 2017 19:35:58 -
> @@ -100,10 +100,9 @@ includes:
>   cd ${.CURDIR}/$$i && ${RUN_MAKE}; \
>   done
>   chown -RP ${BINOWN}:${BINGRP} ${DESTDIR}/usr/include
> - find ${DESTDIR}/usr/include -type f -print0 | \
> - xargs -0r chmod a=r
> - find ${DESTDIR}/usr/include \( -type d -o -type l \) -print0 | \
> - xargs -0r chmod -h u=rwx,go=rx
> + find ${DESTDIR}/usr/include -type f -exec chmod a=r {} +
> + find ${DESTDIR}/usr/include \( -type d -o -type l \) -exec \
> + chmod -h u=rwx,go=rx {} +
>  
>  copies:
>   @echo copies: ${LDIRS}
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de
> 
FWIW, I've suggested this a while ago already:
https://marc.info/?l=openbsd-tech=149649423503939=2

Re: make includes: use find -exec {} +

[Marc Espie]

On Thu, Oct 05, 2017 at 10:08:32PM +0200, Christian Weisgerber wrote:
> Admittedly, this is cosmetic:
> Use the modern POSIX idiom "-exec ... {} +" instead of find|xargs.
> ok?
> 
> Index: include/Makefile
> ===
> RCS file: /cvs/src/include/Makefile,v
> retrieving revision 1.219
> diff -u -p -r1.219 Makefile
> --- include/Makefile  17 Apr 2017 15:53:21 -  1.219
> +++ include/Makefile  5 Oct 2017 19:35:58 -
> @@ -100,10 +100,9 @@ includes:
>   cd ${.CURDIR}/$$i && ${RUN_MAKE}; \
>   done
>   chown -RP ${BINOWN}:${BINGRP} ${DESTDIR}/usr/include
> - find ${DESTDIR}/usr/include -type f -print0 | \
> - xargs -0r chmod a=r
> - find ${DESTDIR}/usr/include \( -type d -o -type l \) -print0 | \
> - xargs -0r chmod -h u=rwx,go=rx
> + find ${DESTDIR}/usr/include -type f -exec chmod a=r {} +
> + find ${DESTDIR}/usr/include \( -type d -o -type l \) -exec \
> + chmod -h u=rwx,go=rx {} +
>  
>  copies:
>   @echo copies: ${LDIRS}

Assuming you've run it, 'cause I'm not gonna retest, sure !

Re: make includes: use find -exec {} +

[Todd C. Miller]

On Thu, 05 Oct 2017 22:08:32 +0200, Christian Weisgerber wrote:

> Admittedly, this is cosmetic:
> Use the modern POSIX idiom "-exec ... {} +" instead of find|xargs.

Looks good, OK millert@

 - todd

make includes: use find -exec {} +

[Christian Weisgerber]

Admittedly, this is cosmetic:
Use the modern POSIX idiom "-exec ... {} +" instead of find|xargs.
ok?

Index: include/Makefile
===
RCS file: /cvs/src/include/Makefile,v
retrieving revision 1.219
diff -u -p -r1.219 Makefile
--- include/Makefile17 Apr 2017 15:53:21 -  1.219
+++ include/Makefile5 Oct 2017 19:35:58 -
@@ -100,10 +100,9 @@ includes:
cd ${.CURDIR}/$$i && ${RUN_MAKE}; \
done
chown -RP ${BINOWN}:${BINGRP} ${DESTDIR}/usr/include
-   find ${DESTDIR}/usr/include -type f -print0 | \
-   xargs -0r chmod a=r
-   find ${DESTDIR}/usr/include \( -type d -o -type l \) -print0 | \
-   xargs -0r chmod -h u=rwx,go=rx
+   find ${DESTDIR}/usr/include -type f -exec chmod a=r {} +
+   find ${DESTDIR}/usr/include \( -type d -o -type l \) -exec \
+   chmod -h u=rwx,go=rx {} +
 
 copies:
@echo copies: ${LDIRS}
-- 
Christian "naddy" Weisgerber  na...@mips.inka.de

Makefile: Use -exec not xargs(1) with find(1)

[Klemens Nanni]

-exec has been around since 2012 is cleaner and shorter, I don't see why
xargs(1) should be used for such simple cases. Besides that don't walk
the tree twice.

Index: include/Makefile
===
RCS file: /cvs/src/include/Makefile,v
retrieving revision 1.219
diff -u -p -r1.219 Makefile
--- include/Makefile17 Apr 2017 15:53:21 -  1.219
+++ include/Makefile3 Jun 2017 12:48:19 -
@@ -100,10 +100,9 @@ includes:
cd ${.CURDIR}/$$i && ${RUN_MAKE}; \
done
chown -RP ${BINOWN}:${BINGRP} ${DESTDIR}/usr/include
-   find ${DESTDIR}/usr/include -type f -print0 | \
-   xargs -0r chmod a=r
-   find ${DESTDIR}/usr/include \( -type d -o -type l \) -print0 | \
-   xargs -0r chmod -h u=rwx,go=rx
+   find ${DESTDIR}/usr/include \
+   \( -type f -exec chmod a=r {} + \) -o \
+   \( \( -type d -o -type l \) -exec chmod -h u=rwx,go=rx {} + \)

copies:
@echo copies: ${LDIRS}
Index: share/zoneinfo/Makefile
===
RCS file: /cvs/src/share/zoneinfo/Makefile,v
retrieving revision 1.11
diff -u -p -r1.11 Makefile
--- share/zoneinfo/Makefile 24 Nov 2016 14:38:30 -  1.11
+++ share/zoneinfo/Makefile 3 Jun 2017 12:48:19 -
@@ -81,8 +81,9 @@ realinstall: ${DATA} ${REDO} ${YEARISTYP
(cd ${.CURDIR}/datfiles; \
${ZIC} -y ${YEARISTYPECOPY} -d ${TZDIR} -p ${POSIXRULES})
chown -R ${BINOWN}:${BINGRP} ${TZDIR}
-   find ${TZDIR} -type f -print0 | xargs -0r chmod a=r
-   find ${TZDIR} -type d -print0 | xargs -0r chmod a=rx
+   find ${TZDIR} \
+   \( -type f -exec chmod a=r  {} + \) -o \
+   \( -type d -exec chmod a=rx {} + \)
${INSTALL} -c -o root -g bin -m 644 ${.CURDIR}/datfiles/iso3166.tab \
${DESTDIR}/usr/share/misc
${INSTALL} -c -o root -g bin -m 644 ${.CURDIR}/datfiles/zone.tab \

Re: find -delete

[Dmitrij D. Czarkoff]

"Ted Unangst" <t...@tedunangst.com> wrote:

> This option is not posix (not like that's stopped find accumulating a
> dozen extensions), but it is in gnu and freebsd (for 20 years). it's
> also somewhat popular among sysadmins and blogs, etc. and perhaps most
> importantly, it nicely solves one of the more troublesome caveats of
> find (which the man page actually covers twice because it's so common
> and easy to screw up). So I think it makes a good addition.

I find this option highly distasteful and largerly useless.  You
generally want to use find -exec when you know what you are doing, so
choosing between ; and + is not really an issue unless you are doing it
wrong.  (Of course it might be an issue if you don't use find very
often, but then -delete is a huge hazard because of its nuances.)

I am OK with adding it for compatibility reasons, but I oppose adding
examples of its usage to manual page.

Re: find -delete

[Mark Kettenis]

> Date: Sat, 7 Jan 2017 13:15:05 +
> From: Stuart Henderson <s...@spacehopper.org>
> 
> On 2017/01/07 11:32, Marc Espie wrote:
> > On Tue, Jan 03, 2017 at 11:15:39PM +0100, Mark Kettenis wrote:
> > > > From: "Ted Unangst" <t...@tedunangst.com>
> > > > Date: Tue, 03 Jan 2017 16:39:48 -0500
> > > > 
> > > > I copied this straight from freebsd. Not fixed, but feel free to 
> > > > correct as
> > > > desired.
> > > > 
> > > > This adds a third example showing -delete, mentioning that it's not 
> > > > standard,
> > > > but also hinting that it may work better than "rm -r" when you want to 
> > > > delete
> > > > directories.
> > > 
> > > I really think we should not encourage unportable code like that by
> > > giving an example in our manual page.
> > > 
> > > I'm even tempted to say that you should leave the "-exec rm {} \;"
> > > example alone.  The + here only works because rm(1) accepts multiple
> > > file arguments.
> > 
> > Huh ? of course rm accepts multiple file arguments. All "good" unix commands
> > accept multiple file arguments, fortunately. That's what makes +exec (or
> > xargs for that matter) work.
> 
> I think the reason the example uses ; is probably because we only
> added + relatively recently (2012), and adding an example using it
> back then would make it harder for people writing scripts that
> work on newer and older OpenBSD. Enough time has passed that this
> isn't really an issue any more. But I would very much like to keep
> an example using ; to demonstrate the shell quoting needed when
> people have to use a bad command that only takes one file argument.

Right.  That was more or less my point.  The man page is there to
explain how find(1) works, not to show the most efficient way to
delete a bunch of files.

Not that a clear and concise discussion of the various trade-offs of
\; vs. + vs. -delete would hurt.

Re: find -delete

[Stuart Henderson]

On 2017/01/07 11:32, Marc Espie wrote:
> On Tue, Jan 03, 2017 at 11:15:39PM +0100, Mark Kettenis wrote:
> > > From: "Ted Unangst" 
> > > Date: Tue, 03 Jan 2017 16:39:48 -0500
> > > 
> > > I copied this straight from freebsd. Not fixed, but feel free to correct 
> > > as
> > > desired.
> > > 
> > > This adds a third example showing -delete, mentioning that it's not 
> > > standard,
> > > but also hinting that it may work better than "rm -r" when you want to 
> > > delete
> > > directories.
> > 
> > I really think we should not encourage unportable code like that by
> > giving an example in our manual page.
> > 
> > I'm even tempted to say that you should leave the "-exec rm {} \;"
> > example alone.  The + here only works because rm(1) accepts multiple
> > file arguments.
> 
> Huh ? of course rm accepts multiple file arguments. All "good" unix commands
> accept multiple file arguments, fortunately. That's what makes +exec (or
> xargs for that matter) work.

I think the reason the example uses ; is probably because we only
added + relatively recently (2012), and adding an example using it
back then would make it harder for people writing scripts that
work on newer and older OpenBSD. Enough time has passed that this
isn't really an issue any more. But I would very much like to keep
an example using ; to demonstrate the shell quoting needed when
people have to use a bad command that only takes one file argument.

Re: find -delete

[Marc Espie]

On Tue, Jan 03, 2017 at 11:15:39PM +0100, Mark Kettenis wrote:
> > From: "Ted Unangst" 
> > Date: Tue, 03 Jan 2017 16:39:48 -0500
> > 
> > I copied this straight from freebsd. Not fixed, but feel free to correct as
> > desired.
> > 
> > This adds a third example showing -delete, mentioning that it's not 
> > standard,
> > but also hinting that it may work better than "rm -r" when you want to 
> > delete
> > directories.
> 
> I really think we should not encourage unportable code like that by
> giving an example in our manual page.
> 
> I'm even tempted to say that you should leave the "-exec rm {} \;"
> example alone.  The + here only works because rm(1) accepts multiple
> file arguments.

Huh ? of course rm accepts multiple file arguments. All "good" unix commands
accept multiple file arguments, fortunately. That's what makes +exec (or
xargs for that matter) work.