Re: ftp: make use of getline(3)

2021-02-15 Thread Jeremie Courreges-Anglas
On Sun, Feb 14 2021, Christian Weisgerber  wrote:
> Christian Weisgerber:
>
>> > Make use of getline(3) in ftp(1).
>> > 
>> > Replace fparseln(3) with getline(3).  This removes the only use
>> > of libutil.a(fparseln.o) from the ramdisk.
>> > Replace a complicated fgetln(3) idiom with the much simpler getline(3).
>> 
>> OK?
>
> ping?

That's much nicer indeed.  ok jca@

> I've been fetching distfiles with it, and I also built a bsd.rd and
> performed a http install with it.

chunked encoding still works fine too.

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



Re: video(4) multiple opens

2021-02-12 Thread Jeremie Courreges-Anglas
On Wed, Feb 10 2021, Martin Pieuchot  wrote:

[...]

> Which fields is the new lock protecting?  Why isn't the KERNEL_LOCK()
> enough?

When I mentioned this potential lack of locking to Marcus, I was
mistaken.  Some of the functions in video.c are called from syscalls
that are marked NOLOCK.  But now that I have added some printf
debugging, I can see that the kernel lock is held in places where
I didn't expect it to be held (videoioctl, videoread, videoclose...).
So there's probably a layer of locking that I am missing.

Marcus, sorry for my misleading diff. O:)

*crawls back under his rock*
-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE



Re: unwind(8): open DNSSEC trustanchor late

2021-02-06 Thread Jeremie Courreges-Anglas
On Sat, Feb 06 2021, Florian Obser  wrote:
> On Sat, Feb 06, 2021 at 01:23:35AM +0100, Jeremie Courreges-Anglas wrote:
>> On Fri, Jan 29 2021, Florian Obser  wrote:
>> > Last piece of the puzzle...
>> >
>> > Re-try to open DNSSEC trust anchor file if /var is not mounted yet.
>> > With this we are able to start unwind before the network is up and
>> > partitions are mounted.
>> 
>> Sorry for being late to the party, I just upgraded to the latest snaps
>> and DNS broke.  Reverting this diff unbreaks unwind(8) operations.
>> 
>
> Could you be more specific what broke?

Yes, sorry for the lack of details.  I just didn't get any reply from
unwind(8).

Debug output with "preference { recursor }":

--8<--
shannon /usr/src/sbin/unwind$ sudo obj/unwind -dvv &
[1] 22635
shannon /usr/src/sbin/unwind$ fstat | grep unwind
_unwind  unwind 54878 text /usr/obj   103939  -rwxr-xr-x r 11104216
_unwind  unwind 54878   wd /var   155539  drwxr-xr-x r  512
_unwind  unwind 54878 root /var   155539  drwxr-xr-x r  512
_unwind  unwind 548780 /  104248  crw--wrwttyp6
_unwind  unwind 548781 /  104248  crw--wrwttyp6
_unwind  unwind 548782 /  104248  crw--wrwttyp6
_unwind  unwind 548783* unix stream 0x0
_unwind  unwind 548784 kqueue 0x0 0 state: W
_unwind  unwind 548785* unix stream 0x0
_unwind  unwind 548786* internet dgram udp 127.0.0.1:53
_unwind  unwind 548787* internet6 dgram udp [::1]:53
_unwind  unwind 548788* internet stream tcp 0x0 127.0.0.1:53
_unwind  unwind 548789* internet6 stream tcp 0x0 [::1]:53
_unwind  unwind 54878   10* unix stream 0x0 /dev/unwind.sock
_unwind  unwind 54878   11* route raw 0 0x0
_unwind  unwind 54878   12 /var   234193  -rw-r--r--   rwp  376
_unwind  unwind 39065 text /usr/obj   103939  -rwxr-xr-x r 11104216
_unwind  unwind 39065   wd /usr/src   398245  drwxr-xr-x r 4096
_unwind  unwind 390650 /  104248  crw--wrwttyp6
_unwind  unwind 390651 /  104248  crw--wrwttyp6
_unwind  unwind 390652 /  104248  crw--wrwttyp6
_unwind  unwind 390653* unix stream 0x0
_unwind  unwind 390654 kqueue 0x0 0 state: W
_unwind  unwind 390655* unix stream 0x0
root unwind 22635 text /usr/obj   103939  -rwxr-xr-x r 11104216
root unwind 22635   wd /usr/src   398245  drwxr-xr-x r 4096
root unwind 226350 /  104248  crw--wrwttyp6
root unwind 226351 /  104248  crw--wrwttyp6
root unwind 226352 /  104248  crw--wrwttyp6
root unwind 226353* unix stream 0x0
root unwind 226354 kqueue 0x0 0 state: W
root unwind 226355* unix stream 0x0
root unwind 22635   14* route raw 0 0x0
shannon /usr/src/sbin/unwind$ dig openbsd.org @127.0.0.1
from: [127.0.0.1]:14381
;; ->>HEADER<<- opcode: QUERY, rcode: NOERROR, id: 26482
;; flags: rd ad ; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1
;; QUESTION SECTION:
openbsd.org.IN  A

;; ANSWER SECTION:

;; AUTHORITY SECTION:

;; ADDITIONAL SECTION:
; EDNS: version: 0; flags: ; udp: 4096
;; MSG SIZE  rcvd: 40

[127.0.0.1]:14381: openbsd.org. IN A ?
try_next_resolver: could not find (any more) working resolvers
from: [127.0.0.1]:14381
;; ->>HEADER<<- opcode: QUERY, rcode: NOERROR, id: 26482
;; flags: rd ad ; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1
;; QUESTION SECTION:
openbsd.org.IN  A

;; ANSWER SECTION:

;; AUTHORITY SECTION:

;; ADDITIONAL SECTION:
; EDNS: version: 0; flags: ; udp: 4096
;; MSG SIZE  rcvd: 40

[127.0.0.1]:14381: openbsd.org. IN A ?
try_next_resolver: could not find (any more) working resolvers
from: [127.0.0.1]:14381
;; ->>HEADER<<- opcode: QUERY, rcode: NOERROR, id: 26482
;; flags: rd ad ; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1
;; QUESTION SECTION:
openbsd.org.IN  A

;; ANSWER SECTION:

;; AUTHORITY SECTION:

;; ADDITIONAL SECTION:
; EDNS: version: 0; flags: ; udp: 4096
;; MSG SIZE  rcvd: 40

[127.0.0.1]:14381: openbsd.org. IN A ?
try_next_resolver: could not find (any more) working resolvers
[... etc etc]
^C
shannon /usr/src/sbin/unwind$
-->8--


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



Re: unwind(8): open DNSSEC trustanchor late

2021-02-05 Thread Jeremie Courreges-Anglas
On Fri, Jan 29 2021, Florian Obser  wrote:
> Last piece of the puzzle...
>
> Re-try to open DNSSEC trust anchor file if /var is not mounted yet.
> With this we are able to start unwind before the network is up and
> partitions are mounted.

Sorry for being late to the party, I just upgraded to the latest snaps
and DNS broke.  Reverting this diff unbreaks unwind(8) operations.

My unwind.conf:

  preference { recursor }

(Can't reproduce this problem with an empty config file.)

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



Re: getopt.3 bugs section

2021-01-09 Thread Jeremie Courreges-Anglas
On Sat, Jan 09 2021, Christian Weisgerber  wrote:
> Edgar Pettijohn:
>
>> In the BUGS section for the getopt(3) manual it mentions not using
>> single digits for options. I know spamd uses -4 and -6 there are
>> probably others. Should they be changed? Or is the manual mistaken?
>
> You misunderstand.  The manual warns against the use of digits to
> pass numerical arguments.  This usage exists in some historical
> cases, e.g. "nice -10" where <10> is the number 10.

IMO giving a *number* as an example would make things clearer.

Index: getopt.3
===
RCS file: /d/cvs/src/lib/libc/stdlib/getopt.3,v
retrieving revision 1.46
diff -u -p -p -u -r1.46 getopt.3
--- getopt.34 Jan 2016 19:43:13 -   1.46
+++ getopt.39 Jan 2021 13:39:06 -
@@ -326,7 +326,7 @@ It is possible to handle digits as optio
 This allows
 .Fn getopt
 to be used with programs that expect a number
-.Pq Dq Li \-3
+.Pq Dq Li \-389
 as an option.
 This practice is wrong, and should not be used in any current development.
 It is provided for backward compatibility


Other ideas for improvement:
- give a real life example

Index: getopt.3
===
RCS file: /d/cvs/src/lib/libc/stdlib/getopt.3,v
retrieving revision 1.46
diff -u -p -p -u -r1.46 getopt.3
--- getopt.34 Jan 2016 19:43:13 -   1.46
+++ getopt.39 Jan 2021 13:44:29 -
@@ -326,7 +326,7 @@ It is possible to handle digits as optio
 This allows
 .Fn getopt
 to be used with programs that expect a number
-.Pq Dq Li \-3
+.Pq Dq Li nice \-10 program
 as an option.
 This practice is wrong, and should not be used in any current development.
 It is provided for backward compatibility

- move this warning to CAVEATS, since it's not a bug

Index: getopt.3
===
RCS file: /d/cvs/src/lib/libc/stdlib/getopt.3,v
retrieving revision 1.46
diff -u -p -p -u -r1.46 getopt.3
--- getopt.34 Jan 2016 19:43:13 -   1.46
+++ getopt.39 Jan 2021 13:47:08 -
@@ -309,24 +309,12 @@ The
 .Fn getopt
 function appeared in
 .Bx 4.3 .
-.Sh BUGS
-The
-.Fn getopt
-function was once specified to return
-.Dv EOF
-instead of \-1.
-This was changed by
-.St -p1003.2-92
-to decouple
-.Fn getopt
-from
-.In stdio.h .
-.Pp
+.Sh CAVEATS
 It is possible to handle digits as option letters.
 This allows
 .Fn getopt
 to be used with programs that expect a number
-.Pq Dq Li \-3
+.Pq Dq Li nice \-10 program
 as an option.
 This practice is wrong, and should not be used in any current development.
 It is provided for backward compatibility
@@ -361,3 +349,15 @@ while ((ch = getopt(argc, argv, "0123456
prevoptind = optind;
 }
 .Ed
+.Sh BUGS
+The
+.Fn getopt
+function was once specified to return
+.Dv EOF
+instead of \-1.
+This was changed by
+.St -p1003.2-92
+to decouple
+.Fn getopt
+from
+.In stdio.h .


Would the mention of EOF vs -1 fit better in HISTORY or CAVEATS too?

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



Re: video(4) multiple opens

2021-01-06 Thread Jeremie Courreges-Anglas
On Wed, Jan 06 2021, Marcus Glocker  wrote:
> On Tue, Jan 05, 2021 at 11:54:31PM +0100, Jeremie Courreges-Anglas wrote:

[...]

>> Here's the diff.  IIUC the use of atomic operations isn't strictly
>> needed here since open(2) runs with the kernel lock, but the result
>> is easy to understand IMO.
>
> I don't agree with that, see my comments bellow in point a) and b).
>  
>> Thoughts?  ok?
>
> I think it doesn't harm to allow the same process to do multiple opens
> on video(1) as a first improvement.  But I'm not happy using
> atomic_cas_ptr(9) and atomic_swap_ptr(9) for this because:
>
>   a) As you already have mentioned, we don't require atomic
>  operations here.  I checked that those functions are very
>  rarely used in the kernel, and only there were atomic
>  operation is required.  I'm afraid that when we use those in
>  video(1), and somebody looks at the code later on to
>  eventually replace it, there will be confusion if there was
>  a specific reason why to use atomic functions.
>
>   b) IMO it doesn't simplify the code.  I first had to read the
>  manual pages to understand how those functions work.

Fair enough, I also felt that it was a bit premature.

> I would prefer to do something simpler, like in this adapted diff.
> If it works for you, I'm OK if you commit this instead.

Done, thanks.

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



video(4) multiple opens

2021-01-05 Thread Jeremie Courreges-Anglas


I hit a weird failure with firefox and BigBlueButton
(https://bigbluebutton.org/) where firefox can't use my webcam.
video(1) works, same for other webrtc sites in firefox, eg meet.jit.si.
ktrace shows that a single firefox process tries to open /dev/video0
more than once, and that fails with EBUSY.  The code lies in the
libwebrtc library

  
https://webrtc.googlesource.com/src/+/refs/heads/master/modules/video_capture/linux/

In my tests the multiple open() calls only happen at initialization
time, video streaming only uses one fd.

Back to our kernel, the current restrictive behavior was introduced with

  revision 1.18
  date: 2008/07/23 22:10:21;  author: mglocker;  state: Exp;  lines: +11 -4;
  If /dev/video* is already used by an application, return EBUSY to other
  applications.  Fixes a kernel panic.

  Reported by ian@

Meanwhile, the V4L2 API specifies that a device "can be opened more than
once" from multiple processes, with access to certain methods blocked
when an application starts reading from the device.

  
https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/open.html#multiple-opens

So the API says we're being too restrictive, but we don't want to go
back to uncontrolled access either.  I guess the ideal solution would be
to implement multiple process access with fine-grained checks, but...
I don't have time for that!

An easy improvement for my use case would be to allow the same process
to open a device multiple times.  It fixes firefox + BigBlueButton for
me and doesn't make concurrent accesses worse (multiple threads from the
same process can already do concurrent accesses, which is something
we might want to look at).

Here's the diff.  IIUC the use of atomic operations isn't strictly
needed here since open(2) runs with the kernel lock, but the result
is easy to understand IMO.

Thoughts?  ok?


Index: dev/video.c
===
RCS file: /d/cvs/src/sys/dev/video.c,v
retrieving revision 1.46
diff -u -p -r1.46 video.c
--- dev/video.c 28 Dec 2020 18:28:11 -  1.46
+++ dev/video.c 5 Jan 2021 22:34:04 -
@@ -28,6 +28,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 
 #include 
@@ -46,8 +48,7 @@ struct video_softc {
struct device   *sc_dev;/* hardware device struct */
struct video_hw_if  *hw_if; /* hardware interface */
char sc_dying;  /* device detached */
-#define VIDEO_OPEN 0x01
-   char sc_open;
+   struct process  *sc_owner;  /* owner process */
 
int  sc_fsize;
uint8_t *sc_fbuffer;
@@ -101,6 +102,7 @@ videoattach(struct device *parent, struc
sc->hw_hdl = sa->hdl;
sc->sc_dev = parent;
sc->sc_fbufferlen = 0;
+   sc->sc_owner = NULL;
 
if (sc->hw_if->get_bufsize)
sc->sc_fbufferlen = (sc->hw_if->get_bufsize)(sc->hw_hdl);
@@ -121,6 +123,7 @@ videoopen(dev_t dev, int flags, int fmt,
 {
int unit;
struct video_softc *sc;
+   struct process *owner;
 
unit = VIDEOUNIT(dev);
if (unit >= video_cd.cd_ndevs ||
@@ -128,9 +131,13 @@ videoopen(dev_t dev, int flags, int fmt,
 sc->hw_if == NULL)
return (ENXIO);
 
-   if (sc->sc_open & VIDEO_OPEN)
-   return (EBUSY);
-   sc->sc_open |= VIDEO_OPEN;
+   owner = atomic_cas_ptr(>sc_owner, NULL, p->p_p);
+   if (owner != NULL) {
+   if (owner == p->p_p)
+   return (0);
+   else
+   return (EBUSY);
+   }
 
sc->sc_vidmode = VIDMODE_NONE;
sc->sc_frames_ready = 0;
@@ -153,7 +160,7 @@ videoclose(dev_t dev, int flags, int fmt
if (sc->hw_if->close != NULL)
r = sc->hw_if->close(sc->hw_hdl);
 
-   sc->sc_open &= ~VIDEO_OPEN;
+   atomic_swap_ptr(>sc_owner, NULL);
 
return (r);
 }


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



Re: ftp(1): handle HTTP 308

2021-01-01 Thread Jeremie Courreges-Anglas
On Thu, Dec 31 2020, Lucas  wrote:
> Weekly bump

chrisz@ had a similar diff, already ok'd by kn@ and me.
Christopher, would you mind committing it?

> Index: fetch.c
> ===
> RCS file: /home/cvs/src/usr.bin/ftp/fetch.c,v
> retrieving revision 1.198
> diff -u -p -r1.198 fetch.c
> --- fetch.c   18 Oct 2020 20:35:18 -  1.198
> +++ fetch.c   24 Dec 2020 14:03:03 -
> @@ -843,6 +843,7 @@ noslash:
>   case 302:   /* Found */
>   case 303:   /* See Other */
>   case 307:   /* Temporary Redirect */
> + case 308:   /* Permanent Redirect */
>   isredirect++;
>   if (redirect_loop++ > 10) {
>   warnx("Too many redirections requested");
>

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



Re: Fewer uvmexp

2020-11-18 Thread Jeremie Courreges-Anglas
On Wed, Nov 18 2020, Martin Pieuchot  wrote:
> While auditing the various uses of the uvmexp fields I came across
> those under #ifdet notyet.  May I delete them so I don't have to give
> them some MP love?  Ok?

ok jca@, but while here shouldn't the rest of cpu_vm_init() go too?
Unless I'm missing something it doesn't have side effects except
computing ncolors, and ncolors is meant to be used by the code you're
removing.

> Index: arch/amd64//amd64/cpu.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/cpu.c,v
> retrieving revision 1.150
> diff -u -p -r1.150 cpu.c
> --- arch/amd64//amd64/cpu.c   13 Sep 2020 11:53:16 -  1.150
> +++ arch/amd64//amd64/cpu.c   18 Nov 2020 13:11:17 -
> @@ -443,17 +443,6 @@ cpu_vm_init(struct cpu_info *ci)
>   }
>   ncolors = max(ncolors, tcolors);
>   }
> -
> -#ifdef notyet
> - /*
> -  * Knowing the size of the largest cache on this CPU, re-color
> -  * our pages.
> -  */
> - if (ncolors <= uvmexp.ncolors)
> - return;
> - printf("%s: %d page colors\n", ci->ci_dev->dv_xname, ncolors);
> - uvm_page_recolor(ncolors);
> -#endif
>  }
>  
>  
> Index: arch/luna88k/luna88k/isr.c
> ===
> RCS file: /cvs/src/sys/arch/luna88k/luna88k/isr.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 isr.c
> --- arch/luna88k/luna88k/isr.c28 Jun 2017 10:31:48 -  1.11
> +++ arch/luna88k/luna88k/isr.c18 Nov 2020 13:11:27 -
> @@ -151,10 +151,6 @@ isrdispatch_autovec(int ipl)
>   panic("isrdispatch_autovec: bad ipl %d", ipl);
>  #endif
>  
> -#if 0/* XXX: already counted in machdep.c */
> - uvmexp.intrs++;
> -#endif
> -
>   list = _autovec[ipl];
>   if (LIST_EMPTY(list)) {
>   printf("isrdispatch_autovec: ipl %d unexpected\n", ipl);
>

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



Re: [PATCH] tcpdump: Fix missing argument from icmp_print call in print-skip.c

2020-11-03 Thread Jeremie Courreges-Anglas
On Tue, Nov 03 2020, Theo Buehler  wrote:
> On Tue, Nov 03, 2020 at 04:19:34PM +0530, Neeraj Pal wrote:
>> Hi all,
>> 
>> It seems that there is a typo, 2nd argument - length is missing from
>> the function call icmp_print in print-skip.c
>
> There is quite a bit more that is wrong with print-skip.c than just
> that (try to add it to the Makefile and compile it). It was unhooked
> from the build in 1996.
>
> Shouldn't it rather be sent to the attic?

I think it can be safely removed.  ok jca@

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



Re: [PATCH] Mention unsupported stacking in softraid(4)

2020-10-25 Thread Jeremie Courreges-Anglas
On Sun, Oct 25 2020, "Filippo Valsorda"  wrote:
> Based on the text in faq14.html, but using the manpage language.

Makes sense.  I'm not sure .Em is useful here, though.

ok jca@ if someone wants to pick this up, else I'll just commit it in
a few hours.

Thanks,

> diff --git share/man/man4/softraid.4 share/man/man4/softraid.4
> index 4fa72dd6e..98897c7dd 100644
> --- share/man/man4/softraid.4
> +++ share/man/man4/softraid.4
> @@ -201,6 +201,10 @@ There is no point in wasting a lot of time syncing 
> random data.
>  The RAID 5 discipline does not initialize parity upon creation, instead 
> parity
>  is only updated upon write.
>  .Pp
> +Stacking disciplines (CRYPTO on top of RAID 1, for example) is
> +.Em not supported
> +at this time.
> +.Pp
>  Currently there is no automated mechanism to recover from failed disks.
>  .Pp
>  Certain RAID levels can protect against some data loss
>

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



Re: Non-const basename: usr.bin/ftp

2020-10-17 Thread Jeremie Courreges-Anglas
On Thu, Oct 15 2020, Christian Weisgerber  wrote:
> Accommodate POSIX basename(3) that takes a non-const parameter and
> may modify the string buffer.
>
> I've tried to follow the conventions of the existing code.
>
> ok?
>
> Index: usr.bin/ftp/fetch.c
> ===
> RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
> retrieving revision 1.197
> diff -u -p -r1.197 fetch.c
> --- usr.bin/ftp/fetch.c   4 Jul 2020 11:23:35 -   1.197
> +++ usr.bin/ftp/fetch.c   15 Oct 2020 21:14:28 -
> @@ -192,7 +192,7 @@ file_get(const char *path, const char *o
>   int  fd, out = -1, rval = -1, save_errno;
>   volatile sig_t   oldintr, oldinti;
>   const char  *savefile;
> - char*buf = NULL, *cp;
> + char*buf = NULL, *cp, *pathbuf = NULL;
>   const size_t buflen = 128 * 1024;
>   off_thashbytes;
>   ssize_t  len, wlen;
> @@ -215,8 +215,12 @@ file_get(const char *path, const char *o
>   else {
>   if (path[strlen(path) - 1] == '/')  /* Consider no file */
>   savefile = NULL;/* after dir invalid. */
> - else
> - savefile = basename(path);
> + else {
> + pathbuf = strdup(path);
> + if (pathbuf == NULL)
> + errx(1, "Can't allocate memory for filename");
> + savefile = basename(pathbuf);
> + }
>   }
>  
>   if (EMPTYSTRING(savefile)) {
> @@ -294,6 +298,7 @@ file_get(const char *path, const char *o
>  
>  cleanup_copy:
>   free(buf);
> + free(pathbuf);
>   if (out >= 0 && out != fileno(stdout))
>   close(out);
>   close(fd);
> @@ -315,6 +320,7 @@ url_get(const char *origline, const char
>   int isunavail = 0, retryafter = -1;
>   struct addrinfo hints, *res0, *res;
>   const char *savefile;
> + char *pathbuf = NULL;
>   char *proxyurl = NULL;
>   char *credentials = NULL, *proxy_credentials = NULL;
>   int fd = -1, out = -1;
> @@ -412,8 +418,12 @@ noslash:
>   else {
>   if (path[strlen(path) - 1] == '/')  /* Consider no file */
>   savefile = NULL;/* after dir invalid. */
> - else
> - savefile = basename(path);
> + else {
> + pathbuf = strdup(path);
> + if (pathbuf == NULL)
> + errx(1, "Can't allocate memory for filename");
> + savefile = basename(pathbuf);
> + }
>   }
>  
>   if (EMPTYSTRING(savefile)) {
> @@ -1106,6 +1116,7 @@ cleanup_url_get:
>   if (out >= 0 && out != fileno(stdout))
>   close(out);
>   free(buf);
> + free(pathbuf);
>   free(proxyhost);
>   free(proxyurl);
>   free(newline);
> Index: usr.bin/ftp/util.c
> ===
> RCS file: /cvs/src/usr.bin/ftp/util.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 util.c
> --- usr.bin/ftp/util.c6 Jul 2020 17:11:29 -   1.93
> +++ usr.bin/ftp/util.c15 Oct 2020 21:31:55 -
> @@ -763,7 +763,7 @@ progressmeter(int flag, const char *file
>   off_t cursize, abbrevsize;
>   double elapsed;
>   int ratio, barlength, i, remaining, overhead = 30;
> - char buf[512];
> + char buf[512], *filenamebuf;
>  
>   if (flag == -1) {
>   clock_gettime(CLOCK_MONOTONIC, );
> @@ -782,11 +782,13 @@ progressmeter(int flag, const char *file
>   ratio = MAXIMUM(ratio, 0);
>   ratio = MINIMUM(ratio, 100);
>   if (!verbose && flag == -1) {
> - filename = basename(filename);
> - if (filename != NULL) {
> + filenamebuf = strdup(filename);
> + filename = basename(filenamebuf);
> + if (filenamebuf != NULL && filename != NULL) {

Can basename(3) handle a NULL input?  Yes, but I had to check, and the
semantics in this case are weird.  IMO it's better to do the error
checking in a more usual way, something like the diff below.

If you're fine with that, ok jca@


Index: util.c
===
RCS file: /d/cvs/src/usr.bin/ftp/util.c,v
retrieving revision 1.93
diff -u -p -p -u -r1.93 util.c
--- util.c  6 Jul 2020 17:11:29 -   1.93
+++ util.c  17 Oct 2020 12:20:27 -
@@ -763,7 +763,7 @@ progressmeter(int flag, const char *file
off_t cursize, abbrevsize;
double elapsed;
int ratio, barlength, i, remaining, overhead = 30;
-   char buf[512];
+   char buf[512], *filenamebuf;
 
if (flag == -1) {
clock_gettime(CLOCK_MONOTONIC, );
@@ -782,11 +782,12 @@ progressmeter(int flag, const char *file
ratio = MAXIMUM(ratio, 0);
ratio = 

Re: apmd(8) and hw.perfpolicy quirks

2020-09-27 Thread Jeremie Courreges-Anglas
On Thu, Sep 24 2020, Jeremie Courreges-Anglas  wrote:
> On Wed, Sep 23 2020, "Ted Unangst"  wrote:
>> On 2020-09-23, Jeremie Courreges-Anglas wrote:
>>
>>> ok?
>>
>> Seems fine.
>>
>>
>>> Note: I inlined the apmd(8)->apm(8) perfpolicy conversion for now, which
>>> brings a question.  I find it weird that there is a special "high"
>>> perfpolicy (effectively similar to perfpolicy=manual + setperf=100) but
>>> no "low" perfpolicy.  Is "high" useful to anyone?
>>
>> If you're benchmarking or something, it's convenient to toggle between
>> high and auto. There's less use for low, since that's what auto defaults to.
>
> Well you can do auto->high easily with apm(8) -H or sysctl
> hw.perfpolicy=manual hw.setperf=100.  I'm not sure a special "high"
> hw.perfpolicy choice brings much value and I would appreciate a simple
> "manual" vs "auto" situation.
>
> This has an impact on documentation.  sysctl(2) doesn't mention that
> setting hw.perfpolicy=high also locks hw.setperf=100.  The apm(8)
> manpage only talks about -H setting hw.setperf=100.  The description for
> apmd(8) -H says
>
>   Start apmd in *manual* performance adjustment mode,
>   initialising hw.setperf to 100.
>
> which is not the whole story.  If you use apm/apmd -H you can't later
> just run sysctl hw.setperf=50:
>
>   sysctl: hw.setperf: Operation not permitted

The diff below teaches apmd(8) -H to set hw.perfpolicy="manual" and
hw.setperf=100, instead of setting hw.perfpolicy="high".
- matches the manpage
- apm(8) reporting becomes accurate
- more symmetry with -L ("low")
- avoids the "sysctl: hw.setperf: Operation not permitted" quirk

Teaching apm(8) how to print hw.perfpolicy="high" then becomes low
priority.

While here, simplify the sysctl(2) calls: in this function we don't care
about the old values.

ok?


Index: apmd.c
===
--- apmd.c.orig
+++ apmd.c
@@ -650,27 +650,27 @@ void
 setperfpolicy(char *policy)
 {
int hw_perfpol_mib[] = { CTL_HW, HW_PERFPOLICY };
-   char oldpolicy[32];
-   size_t oldsz = sizeof(oldpolicy);
-   int setlo = 0;
+   int hw_perf_mib[] = { CTL_HW, HW_SETPERF };
+   int new_perf = -1;
 
if (strcmp(policy, "low") == 0) {
policy = "manual";
-   setlo = 1;
+   new_perf = 0;
+   } else if (strcmp(policy, "high") == 0) {
+   policy = "manual";
+   new_perf = 100;
}
 
-   if (sysctl(hw_perfpol_mib, 2, oldpolicy, ,
+   if (sysctl(hw_perfpol_mib, 2, NULL, NULL,
policy, strlen(policy) + 1) == -1)
logmsg(LOG_INFO, "cannot set hw.perfpolicy");
 
-   if (setlo == 1) {
-   int hw_perf_mib[] = {CTL_HW, HW_SETPERF};
-   int perf;
-   int new_perf = 0;
-   size_t perf_sz = sizeof(perf);
-   if (sysctl(hw_perf_mib, 2, , _sz, _perf, perf_sz) 
== -1)
-   logmsg(LOG_INFO, "cannot set hw.setperf");
-   }
+   if (new_perf == -1)
+   return;
+
+   if (sysctl(hw_perf_mib, 2, NULL, NULL,
+   _perf, sizeof(new_perf)) == -1)
+   logmsg(LOG_INFO, "cannot set hw.setperf");
 }
 
 void


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



Re: apmd(8) and hw.perfpolicy quirks

2020-09-24 Thread Jeremie Courreges-Anglas
On Wed, Sep 23 2020, "Ted Unangst"  wrote:
> On 2020-09-23, Jeremie Courreges-Anglas wrote:
>
>> ok?
>
> Seems fine.
>
>
>> Note: I inlined the apmd(8)->apm(8) perfpolicy conversion for now, which
>> brings a question.  I find it weird that there is a special "high"
>> perfpolicy (effectively similar to perfpolicy=manual + setperf=100) but
>> no "low" perfpolicy.  Is "high" useful to anyone?
>
> If you're benchmarking or something, it's convenient to toggle between
> high and auto. There's less use for low, since that's what auto defaults to.

Well you can do auto->high easily with apm(8) -H or sysctl
hw.perfpolicy=manual hw.setperf=100.  I'm not sure a special "high"
hw.perfpolicy choice brings much value and I would appreciate a simple
"manual" vs "auto" situation.

This has an impact on documentation.  sysctl(2) doesn't mention that
setting hw.perfpolicy=high also locks hw.setperf=100.  The apm(8)
manpage only talks about -H setting hw.setperf=100.  The description for
apmd(8) -H says

  Start apmd in *manual* performance adjustment mode,
  initialising hw.setperf to 100.

which is not the whole story.  If you use apm/apmd -H you can't later
just run sysctl hw.setperf=50:

  sysctl: hw.setperf: Operation not permitted

Initially I had a diff to teach apm(8) about hw.perfpolicy="high" but
I'm not sure it's the right direction any more.  Diff below fwiw, not
looking for oks.


Index: apm/apm.c
===
RCS file: /d/cvs/src/usr.sbin/apm/apm.c,v
retrieving revision 1.37
diff -u -p -r1.37 apm.c
--- apm/apm.c   23 Sep 2020 05:50:26 -  1.37
+++ apm/apm.c   24 Sep 2020 20:29:49 -
@@ -399,7 +399,8 @@ balony:
 
if (doperf)
printf("Performance adjustment mode: %s (%d MHz)\n",
-   perf_mode(reply.perfmode), reply.cpuspeed);
+   perfmode_to_perfpolicy(reply.perfmode),
+   reply.cpuspeed);
break;
default:
break;
Index: apmd/apm-proto.h
===
RCS file: /d/cvs/src/usr.sbin/apmd/apm-proto.h,v
retrieving revision 1.10
diff -u -p -r1.10 apm-proto.h
--- apmd/apm-proto.h23 Sep 2020 05:50:26 -  1.10
+++ apmd/apm-proto.h24 Sep 2020 20:29:49 -
@@ -51,6 +51,7 @@ enum apm_perfmode {
PERF_NONE = -1,
PERF_MANUAL,
PERF_AUTO,
+   PERF_HIGH,
 };
 
 struct apm_command {
@@ -66,8 +67,9 @@ struct apm_reply {
struct apm_power_info batterystate;
 };
 
-#define APMD_VNO   3
+#define APMD_VNO   4
 
 extern const char *battstate(int state);
 extern const char *ac_state(int state);
-extern const char *perf_mode(int mode);
+extern const char *perfmode_to_perfpolicy(int mode);
+extern enum apm_perfmode perfpolicy_to_perfmode(const char *);
Index: apmd/apmd.c
===
RCS file: /d/cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.98
diff -u -p -r1.98 apmd.c
--- apmd/apmd.c 24 Sep 2020 07:23:41 -  1.98
+++ apmd/apmd.c 24 Sep 2020 20:29:49 -
@@ -310,16 +310,11 @@ handle_client(int sock_fd, int ctl_fd)
break;
}
 
-   reply.perfmode = PERF_NONE;
-   if (sysctl(perfpol_mib, 2, perfpol, _sz, NULL, 0) == -1)
+   if (sysctl(perfpol_mib, 2, perfpol, _sz, NULL, 0) == -1) {
logmsg(LOG_INFO, "cannot read hw.perfpolicy");
-   else {
-   if (strcmp(perfpol, "manual") == 0 ||
-   strcmp(perfpol, "high") == 0) {
-   reply.perfmode = PERF_MANUAL;
-   } else if (strcmp(perfpol, "auto") == 0)
-   reply.perfmode = PERF_AUTO;
-   }
+   reply.perfmode = PERF_NONE;
+   } else
+   reply.perfmode = perfpolicy_to_perfmode(perfpol);
 
if (sysctl(cpuspeed_mib, 2, , _sz, NULL, 0) == -1) {
logmsg(LOG_INFO, "cannot read hw.cpuspeed");
Index: apmd/apmsubr.c
===
RCS file: /d/cvs/src/usr.sbin/apmd/apmsubr.c,v
retrieving revision 1.9
diff -u -p -r1.9 apmsubr.c
--- apmd/apmsubr.c  23 Sep 2020 05:50:26 -  1.9
+++ apmd/apmsubr.c  24 Sep 2020 20:29:49 -
@@ -31,6 +31,8 @@
 
 #include 
 #include 
+#include 
+
 #include "apm-proto.h"
 
 const char *
@@ -72,14 +74,29 @@ ac_state(int state)
 }
 
 const char *
-perf_mode(int mode)
+perfmode_to_perfpolicy(int mode)
 {
switch (mode) {
-   case PERF_MANUAL:
-   return "manual";
case PERF_AUTO:
return "auto";
+   case PERF_HIGH:
+   return "high&qu

Re: apmd(8) and hw.perfpolicy quirks

2020-09-23 Thread Jeremie Courreges-Anglas
On Wed, Sep 23 2020, Jeremie Courreges-Anglas  wrote:
> Prompted by a report from Miod: setting hw.setperf works only if the
> kernel doesn't have a usable cpu_setperf implementation.  The current
> apmd(8) code warns if setting hw.perfpolicy fails, but then handles
> back bogus values to apm(8) clients.

Some corrections:

> The easy fix is to just query the kernel about the actual hw.setperf
hw.perfpolicy

> value.  This fixes other incorrect apmd(8) assumptions:
> - hw.setperf is initially set to "manual"
hw.perfpolicy

> - hw.setperf can't change behind apmd's back
hw.perfpolicy

> ok?
>
>
> Note: I inlined the apmd(8)->apm(8) perfpolicy conversion for now, which
> brings a question.  I find it weird that there is a special "high"
> perfpolicy (effectively similar to perfpolicy=manual + setperf=100) but
> no "low" perfpolicy.  Is "high" useful to anyone?
>
> Index: apmd.c
> ===
> RCS file: /d/cvs/src/usr.sbin/apmd/apmd.c,v
> retrieving revision 1.97
> diff -u -p -r1.97 apmd.c
> --- apmd.c23 Sep 2020 05:50:26 -  1.97
> +++ apmd.c23 Sep 2020 10:46:37 -
> @@ -59,8 +59,6 @@
>  
>  int debug = 0;
>  
> -int doperf = PERF_NONE;
> -
>  extern char *__progname;
>  
>  void usage(void);
> @@ -255,7 +253,10 @@ handle_client(int sock_fd, int ctl_fd)
>   socklen_t fromlen;
>   struct apm_command cmd;
>   struct apm_reply reply;
> - int cpuspeed_mib[] = {CTL_HW, HW_CPUSPEED};
> + int perfpol_mib[] = { CTL_HW, HW_PERFPOLICY };
> + char perfpol[32];
> + size_t perfpol_sz = sizeof(perfpol);
> + int cpuspeed_mib[] = { CTL_HW, HW_CPUSPEED };
>   int cpuspeed = 0;
>   size_t cpuspeed_sz = sizeof(cpuspeed);
>  
> @@ -290,19 +291,16 @@ handle_client(int sock_fd, int ctl_fd)
>   reply.newstate = HIBERNATING;
>   break;
>   case SETPERF_LOW:
> - doperf = PERF_MANUAL;
>   reply.newstate = NORMAL;
>   logmsg(LOG_NOTICE, "setting hw.perfpolicy to low");
>   setperfpolicy("low");
>   break;
>   case SETPERF_HIGH:
> - doperf = PERF_MANUAL;
>   reply.newstate = NORMAL;
>   logmsg(LOG_NOTICE, "setting hw.perfpolicy to high");
>   setperfpolicy("high");
>   break;
>   case SETPERF_AUTO:
> - doperf = PERF_AUTO;
>   reply.newstate = NORMAL;
>   logmsg(LOG_NOTICE, "setting hw.perfpolicy to auto");
>   setperfpolicy("auto");
> @@ -312,11 +310,22 @@ handle_client(int sock_fd, int ctl_fd)
>   break;
>   }
>  
> - if (sysctl(cpuspeed_mib, 2, , _sz, NULL, 0) == -1)
> - logmsg(LOG_INFO, "cannot read hw.cpuspeed");
> + reply.perfmode = PERF_NONE;
> + if (sysctl(perfpol_mib, 2, perfpol, _sz, NULL, 0) == -1)
> + logmsg(LOG_INFO, "cannot read hw.perfpolicy");
> + else {
> + if (strcmp(perfpol, "manual") == 0 ||
> + strcmp(perfpol, "high") == 0) {
> + reply.perfmode = PERF_MANUAL;
> + } else if (strcmp(perfpol, "auto") == 0)
> + reply.perfmode = PERF_AUTO;
> + }
>  
> + if (sysctl(cpuspeed_mib, 2, , _sz, NULL, 0) == -1) {
> + logmsg(LOG_INFO, "cannot read hw.cpuspeed");
> + cpuspeed = 0;
> + }
>   reply.cpuspeed = cpuspeed;
> - reply.perfmode = doperf;
>   reply.vno = APMD_VNO;
>   if (send(cli_fd, , sizeof(reply), 0) != sizeof(reply))
>   logmsg(LOG_INFO, "reply to client botched");
> @@ -386,6 +395,7 @@ main(int argc, char *argv[])
>   const char *errstr;
>   int kq, nchanges;
>   struct kevent ev[2];
> + int doperf = PERF_NONE;
>  
>   while ((ch = getopt(argc, argv, "aACdHLsf:t:S:z:Z:")) != -1)
>   switch(ch) {

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



apmd(8) and hw.perfpolicy quirks

2020-09-23 Thread Jeremie Courreges-Anglas


Prompted by a report from Miod: setting hw.setperf works only if the
kernel doesn't have a usable cpu_setperf implementation.  The current
apmd(8) code warns if setting hw.perfpolicy fails, but then handles
back bogus values to apm(8) clients.

The easy fix is to just query the kernel about the actual hw.setperf
value.  This fixes other incorrect apmd(8) assumptions:
- hw.setperf is initially set to "manual"
- hw.setperf can't change behind apmd's back

ok?


Note: I inlined the apmd(8)->apm(8) perfpolicy conversion for now, which
brings a question.  I find it weird that there is a special "high"
perfpolicy (effectively similar to perfpolicy=manual + setperf=100) but
no "low" perfpolicy.  Is "high" useful to anyone?


Index: apmd.c
===
RCS file: /d/cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.97
diff -u -p -r1.97 apmd.c
--- apmd.c  23 Sep 2020 05:50:26 -  1.97
+++ apmd.c  23 Sep 2020 10:46:37 -
@@ -59,8 +59,6 @@
 
 int debug = 0;
 
-int doperf = PERF_NONE;
-
 extern char *__progname;
 
 void usage(void);
@@ -255,7 +253,10 @@ handle_client(int sock_fd, int ctl_fd)
socklen_t fromlen;
struct apm_command cmd;
struct apm_reply reply;
-   int cpuspeed_mib[] = {CTL_HW, HW_CPUSPEED};
+   int perfpol_mib[] = { CTL_HW, HW_PERFPOLICY };
+   char perfpol[32];
+   size_t perfpol_sz = sizeof(perfpol);
+   int cpuspeed_mib[] = { CTL_HW, HW_CPUSPEED };
int cpuspeed = 0;
size_t cpuspeed_sz = sizeof(cpuspeed);
 
@@ -290,19 +291,16 @@ handle_client(int sock_fd, int ctl_fd)
reply.newstate = HIBERNATING;
break;
case SETPERF_LOW:
-   doperf = PERF_MANUAL;
reply.newstate = NORMAL;
logmsg(LOG_NOTICE, "setting hw.perfpolicy to low");
setperfpolicy("low");
break;
case SETPERF_HIGH:
-   doperf = PERF_MANUAL;
reply.newstate = NORMAL;
logmsg(LOG_NOTICE, "setting hw.perfpolicy to high");
setperfpolicy("high");
break;
case SETPERF_AUTO:
-   doperf = PERF_AUTO;
reply.newstate = NORMAL;
logmsg(LOG_NOTICE, "setting hw.perfpolicy to auto");
setperfpolicy("auto");
@@ -312,11 +310,22 @@ handle_client(int sock_fd, int ctl_fd)
break;
}
 
-   if (sysctl(cpuspeed_mib, 2, , _sz, NULL, 0) == -1)
-   logmsg(LOG_INFO, "cannot read hw.cpuspeed");
+   reply.perfmode = PERF_NONE;
+   if (sysctl(perfpol_mib, 2, perfpol, _sz, NULL, 0) == -1)
+   logmsg(LOG_INFO, "cannot read hw.perfpolicy");
+   else {
+   if (strcmp(perfpol, "manual") == 0 ||
+   strcmp(perfpol, "high") == 0) {
+   reply.perfmode = PERF_MANUAL;
+   } else if (strcmp(perfpol, "auto") == 0)
+   reply.perfmode = PERF_AUTO;
+   }
 
+   if (sysctl(cpuspeed_mib, 2, , _sz, NULL, 0) == -1) {
+   logmsg(LOG_INFO, "cannot read hw.cpuspeed");
+   cpuspeed = 0;
+   }
reply.cpuspeed = cpuspeed;
-   reply.perfmode = doperf;
reply.vno = APMD_VNO;
if (send(cli_fd, , sizeof(reply), 0) != sizeof(reply))
logmsg(LOG_INFO, "reply to client botched");
@@ -386,6 +395,7 @@ main(int argc, char *argv[])
const char *errstr;
int kq, nchanges;
struct kevent ev[2];
+   int doperf = PERF_NONE;
 
while ((ch = getopt(argc, argv, "aACdHLsf:t:S:z:Z:")) != -1)
switch(ch) {

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



Re: httpd(8): don't leak iov

2020-09-20 Thread Jeremie Courreges-Anglas
On Sun, Sep 20 2020, Tobias Heider  wrote:
> iov is allocated with calloc.  I think we should free it after the imsg
> is sent.
>
> ok?

ok jca@

> Index: config.c
> ===
> RCS file: /cvs/src/usr.sbin/httpd/config.c,v
> retrieving revision 1.57
> diff -u -p -r1.57 config.c
> --- config.c  8 May 2019 19:57:45 -   1.57
> +++ config.c  20 Sep 2020 13:34:07 -
> @@ -387,8 +387,10 @@ config_setserver_fcgiparams(struct httpd
>   if (proc_composev(ps, PROC_SERVER, IMSG_CFG_FCGI, iov, c) != 0) {
>   log_warn("%s: failed to compose IMSG_CFG_FCGI imsg for "
>   "`%s'", __func__, srv_conf->name);
> + free(iov);
>   return (-1);
>   }
> + free(iov);
>  
>   return (0);
>  }
>

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



apm(8) and apmd(8): zap "cool" running mode remnants

2020-09-20 Thread Jeremie Courreges-Anglas


apmd "cool" mode was removed in 2014, and -C was made an undocumented
compat alias for -A ("auto").

The code still contains kinda misleading references to this "cool"
running mode, the diff below zaps those.  ok?


Index: apm/apm.c
===
RCS file: /d/cvs/src/usr.sbin/apm/apm.c,v
retrieving revision 1.36
diff -u -p -r1.36 apm.c
--- apm/apm.c   28 Jun 2019 13:32:46 -  1.36
+++ apm/apm.c   20 Sep 2020 12:24:43 -
@@ -185,14 +185,10 @@ main(int argc, char *argv[])
action = HIBERNATE;
break;
case 'A':
-   if (action != NONE)
-   usage();
-   action = SETPERF_AUTO;
-   break;
case 'C':
if (action != NONE)
usage();
-   action = SETPERF_COOL;
+   action = SETPERF_AUTO;
break;
case 'H':
if (action != NONE)
@@ -277,7 +273,6 @@ main(int argc, char *argv[])
case SETPERF_LOW:
case SETPERF_HIGH:
case SETPERF_AUTO:
-   case SETPERF_COOL:
if (fd == -1)
errx(1, "cannot connect to apmd, "
"not changing performance adjustment mode");
Index: apmd/apm-proto.h
===
RCS file: /d/cvs/src/usr.sbin/apmd/apm-proto.h,v
retrieving revision 1.9
diff -u -p -r1.9 apm-proto.h
--- apmd/apm-proto.h26 Mar 2012 20:17:45 -  1.9
+++ apmd/apm-proto.h20 Sep 2020 12:28:01 -
@@ -38,7 +38,6 @@ enum apm_action {
SETPERF_LOW,
SETPERF_HIGH,
SETPERF_AUTO,
-   SETPERF_COOL
 };
 
 enum apm_state {
@@ -52,7 +51,6 @@ enum apm_perfmode {
PERF_NONE = -1,
PERF_MANUAL,
PERF_AUTO,
-   PERF_COOL
 };
 
 struct apm_command {
Index: apmd/apmd.c
===
RCS file: /d/cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.96
diff -u -p -r1.96 apmd.c
--- apmd/apmd.c 13 Mar 2020 09:08:58 -  1.96
+++ apmd/apmd.c 20 Sep 2020 12:25:53 -
@@ -302,7 +302,6 @@ handle_client(int sock_fd, int ctl_fd)
setperfpolicy("high");
break;
case SETPERF_AUTO:
-   case SETPERF_COOL:
doperf = PERF_AUTO;
reply.newstate = NORMAL;
logmsg(LOG_NOTICE, "setting hw.perfpolicy to auto");
Index: apmd/apmsubr.c
===
RCS file: /d/cvs/src/usr.sbin/apmd/apmsubr.c,v
retrieving revision 1.8
diff -u -p -r1.8 apmsubr.c
--- apmd/apmsubr.c  15 Mar 2006 20:30:28 -  1.8
+++ apmd/apmsubr.c  20 Sep 2020 12:28:49 -
@@ -79,8 +79,6 @@ perf_mode(int mode)
return "manual";
case PERF_AUTO:
return "auto";
-   case PERF_COOL:
-   return "cool running";
default:
return "invalid";
}




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



Re: ftp: allow specifying supported protocols

2020-09-06 Thread Jeremie Courreges-Anglas
On Sun, Sep 06 2020, Theo Buehler  wrote:
> On Sun, Sep 06, 2020 at 12:55:17AM +0200, Jeremie Courreges-Anglas wrote:
>> On Sat, Sep 05 2020, Theo Buehler  wrote:
>> > I found this small diff useful more than once (admittedly for debugging).
>> > It allows specifying the protocols that may be used by ftp the same way
>> > as nc -Tprotocols works. For example:
>> >
>> > $ ftp -Sprotocols=tlsv1.2:tlsv1.1 https://example.com/file
>> >
>> > Perhaps someone else has use for it, too?
>> 
>> I think it's useful indeed.  ok jca@
>> 
>> One nit below,
>> 
>> > Index: ftp.1
>> > ===
>> > RCS file: /var/cvs/src/usr.bin/ftp/ftp.1,v
>> > retrieving revision 1.119
>> > diff -u -p -r1.119 ftp.1
>> > --- ftp.1  11 Feb 2020 18:41:39 -  1.119
>> > +++ ftp.1  5 Sep 2020 18:11:24 -
>> > @@ -261,6 +261,12 @@ Don't perform server certificate validat
>> >  Require the server to present a valid OCSP stapling in the TLS handshake.
>> >  .It Cm noverifytime
>> >  Disable validation of certificate times and OCSP validation.
>> > +.It Cm protocols Ns = Ns Ar protocol_list
>> > +Specify the supported TLS protocols that will be used by
>> > +.Nm
>> > +(see
>> > +.Xr tls_config_parse_protocols 3
>> > +for details).
>> >  .It Cm session Ns = Ns Ar /path/to/session
>> >  Specify a file to use for TLS session data.
>> >  If this file has a non-zero length, the session data will be read from 
>> > this file
>> > Index: main.c
>> > ===
>> > RCS file: /var/cvs/src/usr.bin/ftp/main.c,v
>> > retrieving revision 1.132
>> > diff -u -p -r1.132 main.c
>> > --- main.c 1 Sep 2020 12:33:48 -   1.132
>> > +++ main.c 1 Sep 2020 18:13:09 -
>> > @@ -209,6 +209,8 @@ char * const ssl_verify_opts[] = {
>> >"noverifytime",
>> >  #define SSL_SESSION   8
>> >"session",
>> > +#define SSL_PROTOCOLS 9
>> > +  "protocols",
>> >NULL
>> >  };
>> >  
>> > @@ -220,6 +222,7 @@ process_ssl_options(char *cp)
>> >  {
>> >const char *errstr;
>> >long long depth;
>> 
>> (Should probably be an int.)
>>
>> > +  uint32_t protocols;
>> 
>> Could be moved below str (smaller size on lp64, see style(9)).
>> 
>> >char *str;
>
> If we care about this, shouldn't I rather move *str up below *errstr?

Yes.  The diff below shows the overall changes I had in mind.  Please
pick it up if you like it, or just use your version.

Sorry for the bikeshed color concern, I should have brought this up
later in private (if at all).


Index: main.c
===
RCS file: /d/cvs/src/usr.bin/ftp/main.c,v
retrieving revision 1.132
diff -u -p -p -u -r1.132 main.c
--- main.c  1 Sep 2020 12:33:48 -   1.132
+++ main.c  6 Sep 2020 08:26:25 -
@@ -209,6 +209,8 @@ char * const ssl_verify_opts[] = {
"noverifytime",
 #define SSL_SESSION8
"session",
+#define SSL_PROTOCOLS  9
+   "protocols",
NULL
 };
 
@@ -219,8 +221,9 @@ static void
 process_ssl_options(char *cp)
 {
const char *errstr;
-   long long depth;
char *str;
+   int depth;
+   uint32_t protocols;
 
while (*cp) {
switch (getsubopt(, ssl_verify_opts, )) {
@@ -259,7 +262,7 @@ process_ssl_options(char *cp)
if (errstr)
errx(1, "certificate validation depth is %s",
errstr);
-   tls_config_set_verify_depth(tls_config, (int)depth);
+   tls_config_set_verify_depth(tls_config, depth);
break;
case SSL_MUSTSTAPLE:
tls_config_ocsp_require_stapling(tls_config);
@@ -278,6 +281,14 @@ process_ssl_options(char *cp)
tls_session_fd) == -1)
errx(1, "failed to set session: %s",
tls_config_error(tls_config));
+   break;
+   case SSL_PROTOCOLS:
+   if (str == NULL)
+   errx(1, "missing protocol name");
+   if (tls_config_parse_protocols(, str) != 0)
+   errx(1, "failed to parse TLS protocols");
+   if (tls_config_set_protocols(tls_config, protocols) != 
0)
+   errx(1, "failed to set TLS protocols");
break;
default:
errx(1, "unknown -S suboption `%s'",

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



Re: ftp: allow specifying supported protocols

2020-09-05 Thread Jeremie Courreges-Anglas
On Sat, Sep 05 2020, Theo Buehler  wrote:
> I found this small diff useful more than once (admittedly for debugging).
> It allows specifying the protocols that may be used by ftp the same way
> as nc -Tprotocols works. For example:
>
> $ ftp -Sprotocols=tlsv1.2:tlsv1.1 https://example.com/file
>
> Perhaps someone else has use for it, too?

I think it's useful indeed.  ok jca@

One nit below,

> Index: ftp.1
> ===
> RCS file: /var/cvs/src/usr.bin/ftp/ftp.1,v
> retrieving revision 1.119
> diff -u -p -r1.119 ftp.1
> --- ftp.1 11 Feb 2020 18:41:39 -  1.119
> +++ ftp.1 5 Sep 2020 18:11:24 -
> @@ -261,6 +261,12 @@ Don't perform server certificate validat
>  Require the server to present a valid OCSP stapling in the TLS handshake.
>  .It Cm noverifytime
>  Disable validation of certificate times and OCSP validation.
> +.It Cm protocols Ns = Ns Ar protocol_list
> +Specify the supported TLS protocols that will be used by
> +.Nm
> +(see
> +.Xr tls_config_parse_protocols 3
> +for details).
>  .It Cm session Ns = Ns Ar /path/to/session
>  Specify a file to use for TLS session data.
>  If this file has a non-zero length, the session data will be read from this 
> file
> Index: main.c
> ===
> RCS file: /var/cvs/src/usr.bin/ftp/main.c,v
> retrieving revision 1.132
> diff -u -p -r1.132 main.c
> --- main.c1 Sep 2020 12:33:48 -   1.132
> +++ main.c1 Sep 2020 18:13:09 -
> @@ -209,6 +209,8 @@ char * const ssl_verify_opts[] = {
>   "noverifytime",
>  #define SSL_SESSION  8
>   "session",
> +#define SSL_PROTOCOLS9
> + "protocols",
>   NULL
>  };
>  
> @@ -220,6 +222,7 @@ process_ssl_options(char *cp)
>  {
>   const char *errstr;
>   long long depth;

(Should probably be an int.)

> + uint32_t protocols;

Could be moved below str (smaller size on lp64, see style(9)).

>   char *str;
>  
>   while (*cp) {
> @@ -278,6 +281,14 @@ process_ssl_options(char *cp)
>   tls_session_fd) == -1)
>   errx(1, "failed to set session: %s",
>   tls_config_error(tls_config));
> + break;
> + case SSL_PROTOCOLS:
> + if (str == NULL)
> + errx(1, "missing protocol name");
> + if (tls_config_parse_protocols(, str) != 0)
> + errx(1, "failed to parse TLS protocols");
> + if (tls_config_set_protocols(tls_config, protocols) != 
> 0)
> + errx(1, "failed to set TLS protocols");
>   break;
>   default:
>   errx(1, "unknown -S suboption `%s'",
>


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



unwind(8): use SO_REUSEADDR

2020-08-29 Thread Jeremie Courreges-Anglas
On Sat, Aug 29 2020, Stuart Henderson  wrote:
> On 2020/08/27 15:28, Florian Obser wrote:
>> all heavy lifting done by sthen in unbound
>> 
>> tests?
>
> ok with me. only tested lightly (the machine I normally use does DNS for
> other machines too so runs unbound).
>
> related, any idea what's happening here?
>
> unwind[51500]: fatal in main: could not bind to 127.0.0.1 or ::1 on port 53: 
> Address already in use
>
> unbound is listening to *:53, but shouldn't other software be able to
> listen if bound to a specific address like 127.0.0.1:53? is this a bug
> somewhere or am I just missing something about UDP?

Once *:53 is bound you need SO_REUSEADDR to loosen the checks.  ktrace
says unbound(8) uses this unconditionally.  ChangeLog entry:

- Fix #531: Set SO_REUSEADDR so that the wildcard interface and a 
  more specific interface port 53 can be used at the same time, and
  one of the daemons is unbound.

I think unwind could use the same treatment.

Note that using SO_REUSEADDR still prevents two unwind copies from
binding to 127.0.0.1:53 / [::1]:53 (for that to work you'd need
SO_REUSEPORT instead).

Thoughts?  oks?


Index: unwind.c
===
RCS file: /d/cvs/src/sbin/unwind/unwind.c,v
retrieving revision 1.47
diff -u -p -p -u -r1.47 unwind.c
--- unwind.c25 May 2020 16:52:15 -  1.47
+++ unwind.c29 Aug 2020 17:07:49 -
@@ -726,6 +726,7 @@ open_ports(void)
 {
struct addrinfo  hints, *res0;
int  udp4sock = -1, udp6sock = -1, error;
+   int  opt = 1;
 
memset(, 0, sizeof(hints));
hints.ai_family = AF_INET;
@@ -736,6 +737,9 @@ open_ports(void)
if (!error && res0) {
if ((udp4sock = socket(res0->ai_family, res0->ai_socktype,
res0->ai_protocol)) != -1) {
+   if (setsockopt(udp4sock, SOL_SOCKET, SO_REUSEADDR,
+   , sizeof(opt)) == -1)
+   log_warn("setting SO_REUSEADDR on socket");
if (bind(udp4sock, res0->ai_addr, res0->ai_addrlen)
== -1) {
close(udp4sock);
@@ -751,6 +755,9 @@ open_ports(void)
if (!error && res0) {
if ((udp6sock = socket(res0->ai_family, res0->ai_socktype,
res0->ai_protocol)) != -1) {
+   if (setsockopt(udp6sock, SOL_SOCKET, SO_REUSEADDR,
+   , sizeof(opt)) == -1)
+   log_warn("setting SO_REUSEADDR on socket");
if (bind(udp6sock, res0->ai_addr, res0->ai_addrlen)
== -1) {
close(udp6sock);


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



Re: [Patch] Change httpd's handling of request "Host:" headers

2020-08-10 Thread Jeremie Courreges-Anglas
On Mon, Aug 10 2020, Ross L Richardson  wrote:
> Leo,
>
> On Mon, Aug 10, 2020 at 08:46:19AM +0200, Leo Unglaub wrote:
>> Hey,
>> i love your patch. The current behavour always bothered me because it caused
>> servers to display "wrong" sites as defaults for all requests missing the
>> Host header. I really like your patch and it works fine for me on my
>> servers.
>>[...]
>
> Thanks for testing.
>
> I forgot to mention that, by requiring an exact match, the patch should
> fix the port problem [see below] too.
>
> The problem:
>   $ printf "HEAD / HTTP/1.0\r\nHost: www.openbsd.org:22\r\n\r\n" \
>   | nc www.openbsd.org 80 | sed 1q
>   HTTP/1.0 200 OK

If you consider that a problem.  For example apache ignores any
client-provided port number.

  https://httpd.apache.org/docs/2.4/vhosts/details.html#namebased

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



Re: [Patch] Change httpd's handling of request "Host:" headers

2020-08-10 Thread Jeremie Courreges-Anglas
On Sun, Aug 09 2020, Ross L Richardson  wrote:
> At present, if a request contains no "Host:" header [HTTP pre-1.1] or
> if the supplied header does not match any of the servers configured
> in httpd.conf, the request is directed to the first server.  This
> isn't documented, AFAICT.
>
> For example, if httpd.conf has just one server
>   server "www.example.com"
> then we currently get
>   $ printf "HEAD / HTTP/1.0\r\nHost: www.openbsd.org\r\n\r\n" \
>   | nc www.example.com www | sed 1q 
>   HTTP/1.0 200 OK
>
> This behaviour strikes me as wrong (or at least sub-optimal) in the
> case of non-matching "Host:" headers.  The simplistic patch below
> changes things to return a 404 status if no matching server is found.
>
> [If status code 400 (bad request) is preferred, "goto fail;"
> could be used.]
>
> Justification:
> - This seems more correct, and is consistent with the "fail closed"
>   approach.
> - There is a net gain in functionality, as use of glob/patterns
>   wildcards can easily re-establish the current behaviour.  In
>   contrast, there's no way at present to disable the implicit
>   match-anything behaviour.

The first server in my httpd config uses "root "/nonexistent".  This
results in proper 404 replies, so there is a way to disable the current
behavior.  I probably inferred this from the examples in the manpage.

My gut feeling is that the existing behavior is useful (you can
copy/paste the existing example in the manpage and serve files right
away under multiple host names) and I see no reason to break it.
Did you check whether this breaks existing mirrors?

> If this is adopted, it should be document in current.html
> A followup patch could merge this if statement with the one above it.
>
> Several other issues exist in "Host:" header handling.
>
> Ross
> --
>
> Index: server_http.c
> ===
> RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
> retrieving revision 1.140
> diff -u -p -r1.140 server_http.c
> --- server_http.c 3 Aug 2020 10:59:53 -   1.140
> +++ server_http.c 9 Aug 2020 04:37:08 -
> @@ -1200,7 +1200,7 @@ server_response(struct httpd *httpd, str
>   struct server_config*srv_conf = >srv_conf;
>   struct kv   *kv, key, *host;
>   struct str_find  sm;
> - int  portval = -1, ret;
> + int  hostmatch = 0, portval = -1, ret;
>   char*hostval, *query;
>   const char  *errstr = NULL;
>  
> @@ -1277,16 +1277,20 @@ server_response(struct httpd *httpd, str
>   /* Replace host configuration */
>   clt->clt_srv_conf = srv_conf;
>   srv_conf = NULL;
> + hostmatch = 1;
>   break;
>   }
>   }
>   }
>  
> - if (srv_conf != NULL) {
> + if (host == NULL) {
>   /* Use the actual server IP address */
>   if (server_http_host(>clt_srv_ss, hostname,
>   sizeof(hostname)) == NULL)
>   goto fail;
> + } else if (!hostmatch) {
> + server_abort_http(clt, 404, "not found");
> + return (-1);
>   } else {
>   /* Host header was valid and found */
>   if (strlcpy(hostname, host->kv_value, sizeof(hostname)) >=
>

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



Re: allow TCP connections to IPv6 anycast addresses

2020-08-08 Thread Jeremie Courreges-Anglas
On Sat, Aug 08 2020, Florian Obser  wrote:
> On Fri, Aug 07, 2020 at 11:52:46PM +0200, Jeremie Courreges-Anglas wrote:
>> If you don't want to remove M_ACAST from sys/mbuf.h, can you please at
>> least change the comment?  /* obsolete */ or something.
>
> Good point, I forgot to ask about what to do with the flag.
> I think we can remove it, from what I understand %b in printf(9) works
> fine with a sparse decoding string.

Should be fine indeed.

> It compiles but I have no idea how to test it in ddb.

show mbuf addr in a function that uses an mbuf?

> OK? Better to leave out the comment?

I think the comment can be dropped along with the #define.  Userland
shouldn't be poking at this.

ok jca@

> diff --git sys/mbuf.h sys/mbuf.h
> index d52896d3be8..3ddd1b89d66 100644
> --- sys/mbuf.h
> +++ sys/mbuf.h
> @@ -190,7 +190,7 @@ struct mbuf {
>  /* mbuf pkthdr flags, also in m_flags */
>  #define M_VLANTAG0x0020  /* ether_vtag is valid */
>  #define M_LOOP   0x0040  /* packet has been sent from local 
> machine */
> -#define M_ACAST  0x0080  /* received as IPv6 anycast */
> + /* 0x0080 used to be M_ACAST */
>  #define M_BCAST  0x0100  /* sent/received as link-level 
> broadcast */
>  #define M_MCAST  0x0200  /* sent/received as link-level 
> multicast */
>  #define M_CONF   0x0400  /* payload was encrypted 
> (ESP-transport) */
> @@ -203,14 +203,13 @@ struct mbuf {
>  #ifdef _KERNEL
>  #define M_BITS \
>  ("\20\1M_EXT\2M_PKTHDR\3M_EOR\4M_EXTWR\5M_PROTO1\6M_VLANTAG\7M_LOOP" \
> -"\10M_ACAST\11M_BCAST\12M_MCAST\13M_CONF\14M_AUTH\15M_TUNNEL" \
> +"\11M_BCAST\12M_MCAST\13M_CONF\14M_AUTH\15M_TUNNEL" \
>  "\16M_ZEROIZE\17M_COMP\20M_LINK0")
>  #endif
>  
>  /* flags copied when copying m_pkthdr */
>  #define  M_COPYFLAGS 
> (M_PKTHDR|M_EOR|M_PROTO1|M_BCAST|M_MCAST|M_CONF|M_COMP|\
> -  M_AUTH|M_LOOP|M_TUNNEL|M_LINK0|M_VLANTAG|M_ACAST|\
> -  M_ZEROIZE)
> +  M_AUTH|M_LOOP|M_TUNNEL|M_LINK0|M_VLANTAG|M_ZEROIZE)
>  
>  /* Checksumming flags */
>  #define  M_IPV4_CSUM_OUT 0x0001  /* IPv4 checksum needed */

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



Re: allow TCP connections to IPv6 anycast addresses

2020-08-07 Thread Jeremie Courreges-Anglas
On Fri, Aug 07 2020, Florian Obser  wrote:
> No longer prevent TCP connections to IPv6 anycast addresses.
>
> RFC 4291 dropped this requirement from RFC 3513:
>o  An anycast address must not be used as the source address of an
>   IPv6 packet.
>
> And from that requirement draft-itojun-ipv6-tcp-to-anycast rightly
> concluded that TCP connections must be prevented.
>
> The draft also states:
>
> The proposed method MUST be removed when one of the following events
> happens in the future:
>
> o  Restriction imposed on IPv6 anycast address is loosened, so that
>anycast address can be placed into source address field of the IPv6
>header[...]

Also worth reading: https://tools.ietf.org/html/rfc4942#section-2.1.6

> OK?

ok jca@

If you don't want to remove M_ACAST from sys/mbuf.h, can you please at
least change the comment?  /* obsolete */ or something.

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



Re: ssh(1), getrrsetbyname(3), SSHFP and DNSSEC

2020-07-29 Thread Jeremie Courreges-Anglas
On Wed, Jul 29 2020, Florian Obser  wrote:
> On Wed, Jul 29, 2020 at 03:51:55PM +0200, Jeremie Courreges-Anglas wrote:
>> So I did a few tests and read some unwind(8) code, and it *appears* safe
>> to use unwind(8) along with "options trust-ad".
>
> Yes.
>
>> 
>> First you mention fallback to DHCP-learned resolvers.  Those you should
>> probably not trust indeed, but it looks like unwind(8) attempts to use
>> them to perform its own validation.  So the value of the AD flag in
>> unwind(8)'s response should be trustworthy.  At least that's what I see
>> when I test unwind with "preference { dhcp }".
>>
>
> unwind always does its own validation or decides it can't do
> validation. In both cases you can trust the AD flag.
> The AD flag is set if and only if unwind did a successful validation.
>
> You cannot trust the RCODE (and answer) when AD is *NOT* set. That
> seems like a strange and obvious statement but it is the main
> difference to validating unbound. When you enable validation in
> unbound and someone messes with your packets you will get a SERVFAIL.
>
> You can trick unwind into believing that DNSSEC validation is not
> possible in your current network location. It will still resolve but
> it will NOT set AD.

Thanks for confirming.

> In the context of the current discussion this means that you can't
> validate SSHFPs, but you can still try to connect to your ssh server
> if you have the fingerprint in known_hosts. The later is still save
> even if people play tricks with DNS in your current location.

Yep.

>> And then there's the asr fallback, tested with "preference { stub }".
>> unwind(8) allocates its own asr context, ignoring the global
>> /etc/resolv.conf and thus "options trust-ad".  IIUC it then hands off
>
> Yes. But even if it did not it would not matter since it will clear
> AD.
>
>> the answer to libunbound which cooks its own soup.  So it looks like
>
> No, the asr fallback is there for when your local network is truly
> fucked - think last century equipment that knows DNS is udp/53 only and
> max 512 bytes. While we already lower standards considerably in unwind
> when we use libunbound compared to unbound the network still needs to
> provide some things. Working edns0 is one of them, it's near
> impossible to turn of in libunbound.
>
>> unwind needs no special code to disable/ignore "options trust-ad".
>> 
>> I have no idea how unwind/libunbound behaves when using forwarders, the
>> "accept bogus" feature, (opportunistic) DoT or other stuff I'm probably
>
> Nope, as stated above, you can trust AD.
>
>> overlooking.  But AFAICS using unwind + "options trust-ad" brings no
>> problem.  cc'ing Florian, input welcome.
>> 
>> I think the issue with trust-ad is the list of resolvers that end up in
>> resolv.conf.  I would expect people who use a resolver on localhost to
>> also have other resolvers listed in their /etc/resolv.conf, because of
>> a conscious choice (bsd.rd) or just because it should be more reliable.
>> It seems to me that the resolv.conf(5) manpage addition should cover
>> that.  Or should I make it more explicit?
>> 
>> Index: share/man/man5/resolv.conf.5
>> ===
>> RCS file: /d/cvs/src/share/man/man5/resolv.conf.5,v
>> retrieving revision 1.60
>> diff -u -p -r1.60 resolv.conf.5
>> --- share/man/man5/resolv.conf.5 25 Apr 2020 14:22:04 -  1.60
>> +++ share/man/man5/resolv.conf.5 25 Jul 2020 02:08:17 -
>> @@ -285,6 +285,15 @@ first as an absolute name before any sea
>>  .It Cm tcp
>>  Forces the use of TCP for queries.
>>  Normal behaviour is to query via UDP but fall back to TCP on failure.
>> +.It Cm trust-ad
>> +Sets the AD flag in DNS queries, and preserve the value of the AD flag
>> +in DNS replies (this flag is stripped by default).
>> +Useful when applications want to ensure that the DNS resources they
>> +look up have been signed with DNSSEC and validated by the
>> +.Ic nameserver .
>> +This option should be used only if the transport between the
>> +.Ic nameserver
>> +and the resolver is secure.
>
> The last sentence is the problem in my opinion.

Note: that sentence was adapted from getrrsetbyname(3), stripping the
example list precisely because I didn't want to be specific.  The devil
is in the details.  I'm not even sure loopback communication be
considered secure.

> trust-ad is a button
> mere mortals cannot push.

Do mere mortals care about or rely on DNSSEC? ;)

> The typical answer is: Oh, I use a vpn tunel and my nameserver is
> behind the tunnel. Does the tunel terminate on the nameserver?
> My enterprise vpn hands me 4 resolvers. They are in a different
> subnet than the vpn concentrator.

Well one of the reasons I'm proposing a knob is that I doubt we can find
a one-size-fits-all solution.

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



Re: ssh(1), getrrsetbyname(3), SSHFP and DNSSEC

2020-07-29 Thread Jeremie Courreges-Anglas
On Sun, Jul 26 2020, Jeremie Courreges-Anglas  wrote:
> On Sat, Jul 25 2020, Sebastian Benoit  wrote:

[...]

>> If you enable trust-ad on a system that moves around, e.g. your laptop, you
>> will experience failures, which is why unwind tests for this and falls back
>> to non-trusting dhcp learned resolvers in such cases. In fact it also falls
>> back to the stub resolver, i.e. our libc resolver. Which makes me think that
>> trust-ad should not be used when you run unwind (because the whole point of
>> the fallback is that dnssec does not work). But thats a documentation issue.

One important thing to keep in mind: trust-ad is not so much about being
able to do DNSSEC validation.  It's about trusting the validation done
by the resolver(s).  So if the resolver can't do validation and ensures
that AD flag is clear in the reply there is no breach of contract.

> Thanks for pointing this out.  I would expect from unwind(8) that it
> always clears the AD bit from its responses if it could not validate
> them.  Inclusing when it falls back to dynamically learned resolvers or
> the libc resolver.

So I did a few tests and read some unwind(8) code, and it *appears* safe
to use unwind(8) along with "options trust-ad".

First you mention fallback to DHCP-learned resolvers.  Those you should
probably not trust indeed, but it looks like unwind(8) attempts to use
them to perform its own validation.  So the value of the AD flag in
unwind(8)'s response should be trustworthy.  At least that's what I see
when I test unwind with "preference { dhcp }".

And then there's the asr fallback, tested with "preference { stub }".
unwind(8) allocates its own asr context, ignoring the global
/etc/resolv.conf and thus "options trust-ad".  IIUC it then hands off
the answer to libunbound which cooks its own soup.  So it looks like
unwind needs no special code to disable/ignore "options trust-ad".

I have no idea how unwind/libunbound behaves when using forwarders, the
"accept bogus" feature, (opportunistic) DoT or other stuff I'm probably
overlooking.  But AFAICS using unwind + "options trust-ad" brings no
problem.  cc'ing Florian, input welcome.

I think the issue with trust-ad is the list of resolvers that end up in
resolv.conf.  I would expect people who use a resolver on localhost to
also have other resolvers listed in their /etc/resolv.conf, because of
a conscious choice (bsd.rd) or just because it should be more reliable.
It seems to me that the resolv.conf(5) manpage addition should cover
that.  Or should I make it more explicit?

Index: share/man/man5/resolv.conf.5
===
RCS file: /d/cvs/src/share/man/man5/resolv.conf.5,v
retrieving revision 1.60
diff -u -p -r1.60 resolv.conf.5
--- share/man/man5/resolv.conf.525 Apr 2020 14:22:04 -  1.60
+++ share/man/man5/resolv.conf.525 Jul 2020 02:08:17 -
@@ -285,6 +285,15 @@ first as an absolute name before any sea
 .It Cm tcp
 Forces the use of TCP for queries.
 Normal behaviour is to query via UDP but fall back to TCP on failure.
+.It Cm trust-ad
+Sets the AD flag in DNS queries, and preserve the value of the AD flag
+in DNS replies (this flag is stripped by default).
+Useful when applications want to ensure that the DNS resources they
+look up have been signed with DNSSEC and validated by the
+.Ic nameserver .
+This option should be used only if the transport between the
+.Ic nameserver
+and the resolver is secure.
 .El
 .El
 .Pp

Lazy me would be tempted to just let asr trust 127.0.0.1 / ::1 by
default and let others deal with the edge cases, but where is the fun in
that... :)

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



Re: ssh(1), getrrsetbyname(3), SSHFP and DNSSEC

2020-07-29 Thread Jeremie Courreges-Anglas
On Mon, Jul 27 2020, Jesper Wallin  wrote:

[...]

> I still think the RES_USE_AD option might be a useful though, for when
> you want to decide on an application-level whether to trust AD or not?

RES_TRUSTAD can also be used for this, but as the proposed documentation
points out it would be better to let the admin decide.  One
(dis)advantage of using that name is that some external applications
already grok it (eg ports/mail/postfix).

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



Re: ssh(1), getrrsetbyname(3), SSHFP and DNSSEC

2020-07-26 Thread Jeremie Courreges-Anglas
On Sun, Jul 26 2020, Jesper Wallin  wrote:
> On Sat, Jul 25, 2020 at 02:01:06PM +0200, Jeremie Courreges-Anglas wrote:
>> 
>> For those two reasons I think the feature should be opt-in.
>
> Yeah, I agree with you.  My first approach was to have it check what
> kind of DNS record that was requested, and add the AD-flag only if type
> was SSHFP, but that felt even uglier.  I also wasn't so sure my approach
> was the right one after reading the RFCs Peter J. Philipp mentioned.

The quote from RFC6840 seems clear to me, care to share why you had some
doubts if they still exist?

> Perhaps another approach would be to make use of the currently unused
> flags argument in getrrsetbyname(3)?  This way, only getrrsetbyname(3)
> and certain requests are affected by it.

I thought about that too, but getrrsetbyname(3) isn't the only function
of interest.  There's also res_query(3) and friends, which are in more
widely use in the larger ecosystem.  I guess we could restrict AD flag
tweaking to APIs where the caller can actually access the AD flag in the
response, but the "default vs opt-in" question is still present.

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



Re: ssh(1), getrrsetbyname(3), SSHFP and DNSSEC

2020-07-26 Thread Jeremie Courreges-Anglas
On Sat, Jul 25 2020, Sebastian Benoit  wrote:
> Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2020.07.25 14:01:06 +0200:
>> On Fri, Jul 17 2020, Jesper Wallin  wrote:
>> > Hi all,
>> >
>> > I recently decided to add SSHFP records for my servers, since I never
>> > memorize or write down my key fingerprints.  I learned that if I
>> > want ssh(1) to trust these records, DNSSEC needs to be enabled for my
>> > zone.  To validate these records, ssh(1) is using getrrsetbyname(3),
>> > which checks if the AD (Authentic Data) flag is set in the response.
>> >
>> > To get a response with the AD flag set, the request itself also needs
>> > to have the AD flag set.  It turns out that getrrsetbyname(3) doesn't
>> > set this and will therefore never get a validated response, unless the
>> > resolver is configured to always send it, no matter what the client
>> > requested.
>> >
>> > It seems like unwind(8) behaves this way but it also responds with the
>> > RRSIG records, which is extra overhead and ignored by getrrsetbyname(3).
>> >
>> > This was mentioned a few years ago [0] and the solution suggested was
>> > to add the RES_USE_DNSSEC to _res.options, which also makes the resolver
>> > respond with the extra RRSIG records.
>> >
>> > Instead, by only setting the AD flag, both the request and the response
>> > has the same size as without the flag set.  The patch below will add
>> > RES_USE_AD as an option to _res.options and set it by default.
>> > This is also the default behaviour in dig(1), which I understand is a
>> > bit different, but that sure added some confusion while debugging this.
>> >
>> > This let you run unbound(8) or any other validating resolver on your
>> > local network and getrrsetbyname(3) will trust it.  Do read the CAVEATS
>> > in the manual of getrrsetbyname(3) though.
>> >
>> > As a side note, I noticed that the default value of _res.options was the
>> > same value as RES_DEFAULT, so I changed it to RES_DEFAULT instead, for
>> > the sake of consistency.
>> >
>> > Thoughts?
>> 
>> Thanks for addressing this longstanding issue.
>> 
>> I think using the AD bit in queries is a good idea. IIRC Peter J.
>> Philipp (cc'd) suggested using it but I was not thrilled because:
>> 
>> 1. the meaning of the AD bit in queries is relatively recent (2013
>>   I think[0])
>> 2. getrrsetbyname also collects signature records, and for this you need
>>   EDNS + the DO bit set.  Implementing this in was not 100% trivial,
>>   I think we had something working but Eric or I were not 100% happy
>>   with it.
>> 
>> 1. is probably not a concern, after all you're supposed to use
>> a trustworthy resolver, which should be modern and understand the
>> purpose of the AD bit in queries.
>> 
>> 2. is probably not a concern either.  I guess that all getrrsetbyname(3)
>> callers only care about the target records and the AD bit, not about the
>> signature records.  (Why would they use it for anyway?).  In the base
>> system, only ssh(1) and traceroute(8) use getrrsetbyname(3).
>> AFAIK no other system provides getrrsetbyname(3), and ISC has removed
>> getrrsetbyname and the whole lwres API in 2017[1].  So I'd say we're
>> free to improve our version of getrrsetbyname(3) as we see fit.
>
> This is a concern for the stub resolver, because edns and AD does not work
> everywhere.
> Indeed when we switched unbound to validate by default we
> learned that this is not a good idea for everyone - which lead to the
> development of unwind(8).

Do you by chance have any data regarding fallout because of the AD bit
set in queries?  I would expect it to be ignored when not supported.
EDNS and the DNSSEC DO bit is a different story indeed.

>> Now let's discuss the implementation.  There are two main things
>> I dislike with your diff:
>> - it affects all DNS queries done through libc, even if the user doesn't
>>   care at all about DNSSEC.  I suspect there's potential for fallout
>>   here, but I have no data to back this.
>> - trusting the AD bit by default looks a bit too easy.  The
>>   documentation says that people should only trust the AD bit /
>>   RRSET_VALIDATED if the communication between libc and the resolver is
>>   secure, but who actually reads the documentation?
>> 
>> For those two reasons I think the feature should be opt-in.  That's what
>> Florian Weimer did in glibc, with the implementation of RES_TRUSTAD[2][3].
>> The diff below implements the same sema

Re: ssh(1), getrrsetbyname(3), SSHFP and DNSSEC

2020-07-25 Thread Jeremie Courreges-Anglas
On Fri, Jul 17 2020, Jesper Wallin  wrote:
> Hi all,
>
> I recently decided to add SSHFP records for my servers, since I never
> memorize or write down my key fingerprints.  I learned that if I
> want ssh(1) to trust these records, DNSSEC needs to be enabled for my
> zone.  To validate these records, ssh(1) is using getrrsetbyname(3),
> which checks if the AD (Authentic Data) flag is set in the response.
>
> To get a response with the AD flag set, the request itself also needs
> to have the AD flag set.  It turns out that getrrsetbyname(3) doesn't
> set this and will therefore never get a validated response, unless the
> resolver is configured to always send it, no matter what the client
> requested.
>
> It seems like unwind(8) behaves this way but it also responds with the
> RRSIG records, which is extra overhead and ignored by getrrsetbyname(3).
>
> This was mentioned a few years ago [0] and the solution suggested was
> to add the RES_USE_DNSSEC to _res.options, which also makes the resolver
> respond with the extra RRSIG records.
>
> Instead, by only setting the AD flag, both the request and the response
> has the same size as without the flag set.  The patch below will add
> RES_USE_AD as an option to _res.options and set it by default.
> This is also the default behaviour in dig(1), which I understand is a
> bit different, but that sure added some confusion while debugging this.
>
> This let you run unbound(8) or any other validating resolver on your
> local network and getrrsetbyname(3) will trust it.  Do read the CAVEATS
> in the manual of getrrsetbyname(3) though.
>
> As a side note, I noticed that the default value of _res.options was the
> same value as RES_DEFAULT, so I changed it to RES_DEFAULT instead, for
> the sake of consistency.
>
> Thoughts?

Thanks for addressing this longstanding issue.

I think using the AD bit in queries is a good idea. IIRC Peter J.
Philipp (cc'd) suggested using it but I was not thrilled because:

1. the meaning of the AD bit in queries is relatively recent (2013
  I think[0])
2. getrrsetbyname also collects signature records, and for this you need
  EDNS + the DO bit set.  Implementing this in was not 100% trivial,
  I think we had something working but Eric or I were not 100% happy
  with it.

1. is probably not a concern, after all you're supposed to use
a trustworthy resolver, which should be modern and understand the
purpose of the AD bit in queries.

2. is probably not a concern either.  I guess that all getrrsetbyname(3)
callers only care about the target records and the AD bit, not about the
signature records.  (Why would they use it for anyway?).  In the base
system, only ssh(1) and traceroute(8) use getrrsetbyname(3).
AFAIK no other system provides getrrsetbyname(3), and ISC has removed
getrrsetbyname and the whole lwres API in 2017[1].  So I'd say we're
free to improve our version of getrrsetbyname(3) as we see fit.

Now let's discuss the implementation.  There are two main things
I dislike with your diff:
- it affects all DNS queries done through libc, even if the user doesn't
  care at all about DNSSEC.  I suspect there's potential for fallout
  here, but I have no data to back this.
- trusting the AD bit by default looks a bit too easy.  The
  documentation says that people should only trust the AD bit /
  RRSET_VALIDATED if the communication between libc and the resolver is
  secure, but who actually reads the documentation?

For those two reasons I think the feature should be opt-in.  That's what
Florian Weimer did in glibc, with the implementation of RES_TRUSTAD[2][3].
The diff below implements the same semantics:
- by default, don't send queries the AD flag, and don't trust its value
  in replies
- if "options trust-ad" is set in resolv.conf(5), set the AD flag in
  queries and allow applications to look at the AD flag in replies.

There's one more thing I'd like to check in the res_* API, but I'm
sitting on this idea since too long already, and I'm happy with my test
results so far.

Thoughts?  Comments?

[0] https://tools.ietf.org/html/rfc6840#section-5.7
[1] 
https://gitlab.isc.org/isc-projects/bind9/-/commit/8eb88aafee951859264e36c315b1289cd8c2088b
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1164339
[3] 
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=446997ff1433d33452b81dfa9e626b8dccf101a4



Index: include/resolv.h
===
RCS file: /d/cvs/src/include/resolv.h,v
retrieving revision 1.22
diff -u -p -r1.22 resolv.h
--- include/resolv.h14 Jan 2019 06:23:06 -  1.22
+++ include/resolv.h23 Jul 2020 13:28:15 -
@@ -186,6 +186,8 @@ struct __res_state_ext {
 #defineRES_INSECURE2   0x0800  /* type 2 security disabled */
 #defineRES_NOALIASES   0x1000  /* shuts off HOSTALIASES 
feature */
 #defineRES_USE_INET6   0x2000  /* use/map IPv6 in 
gethostbyname() */
+/* GNU extension: use higher bit to avoid conflict with 

ksh(1) set -o pipefail

2020-07-06 Thread Jeremie Courreges-Anglas


Requested by ajacoutot@, here's an attempt at implementing set -o
pipefail.  As pointed by sthen@ this option should be included in the
next POSIX standard:

  https://www.austingroupbugs.net/view.php?id=789

There are several ways to implement it, the diff I showed to Antoine was
based on option 2) in

  https://www.austingroupbugs.net/view.php?id=789#c4102

whereas the diff below implements option 1) described by kre@NetBSD,
which looks like the most sensible approach.

No, this doesn't add support for a PIPESTATUS array, I'm not sure we need it.

This diff includes manpage and regress bits.
Thoughts, oks?


Index: bin/ksh/jobs.c
===
RCS file: /d/cvs/src/bin/ksh/jobs.c,v
retrieving revision 1.61
diff -u -p -r1.61 jobs.c
--- bin/ksh/jobs.c  28 Jun 2019 13:34:59 -  1.61
+++ bin/ksh/jobs.c  6 Jul 2020 18:10:43 -
@@ -70,6 +70,7 @@ struct proc {
 #define JF_REMOVE  0x200   /* flagged for removal (j_jobs()/j_notify()) */
 #define JF_USETTYMODE  0x400   /* tty mode saved if process exits normally */
 #define JF_SAVEDTTYPGRP0x800   /* j->saved_ttypgrp is valid */
+#define JF_PIPEFAIL0x1000  /* pipefail on when job was started */
 
 typedef struct job Job;
 struct job {
@@ -421,6 +422,8 @@ exchild(struct op *t, int flags, volatil
 */
j->flags = (flags & XXCOM) ? JF_XXCOM :
((flags & XBGND) ? 0 : (JF_FG|JF_USETTYMODE));
+   if (Flag(FPIPEFAIL))
+   j->flags |= JF_PIPEFAIL;
timerclear(>usrtime);
timerclear(>systime);
j->state = PRUNNING;
@@ -1084,7 +1087,30 @@ j_waitj(Job *j,
 
j_usrtime = j->usrtime;
j_systime = j->systime;
-   rv = j->status;
+
+   if (j->flags & JF_PIPEFAIL) {
+   Proc *p;
+   int status;
+
+   rv = 0;
+   for (p = j->proc_list; p != NULL; p = p->next) {
+   switch (p->state) {
+   case PEXITED:
+   status = WEXITSTATUS(p->status);
+   break;
+   case PSIGNALLED:
+   status = 128 + WTERMSIG(p->status);
+   break;
+   default:
+   status = 0;
+   break;
+   }
+   if (status)
+   rv = status;
+   }
+   } else
+   rv = j->status;
+
 
if (!(flags & JW_ASYNCNOTIFY) &&
(!Flag(FMONITOR) || j->state != PSTOPPED)) {
Index: bin/ksh/ksh.1
===
RCS file: /d/cvs/src/bin/ksh/ksh.1,v
retrieving revision 1.208
diff -u -p -r1.208 ksh.1
--- bin/ksh/ksh.1   26 Nov 2019 22:49:01 -  1.208
+++ bin/ksh/ksh.1   6 Jul 2020 16:04:34 -
@@ -361,7 +361,9 @@ token to form pipelines, in which the st
 last is piped (see
 .Xr pipe 2 )
 to the standard input of the following command.
-The exit status of a pipeline is that of its last command.
+The exit status of a pipeline is that of its last command, unless the
+.Ic pipefail
+option is set.
 A pipeline may be prefixed by the
 .Ql \&!
 reserved word, which causes the exit status of the pipeline to be logically
@@ -3664,6 +3666,10 @@ See the
 and
 .Ic pwd
 commands above for more details.
+.It Ic pipefail
+The exit status of a pipeline is the exit status of the rightmost
+command in the pipeline that doesn't return 0, or 0 if all commands
+returned a 0 exit status.
 .It Ic posix
 Enable POSIX mode.
 See
Index: bin/ksh/misc.c
===
RCS file: /d/cvs/src/bin/ksh/misc.c,v
retrieving revision 1.73
diff -u -p -r1.73 misc.c
--- bin/ksh/misc.c  28 Jun 2019 13:34:59 -  1.73
+++ bin/ksh/misc.c  6 Jul 2020 16:04:34 -
@@ -147,6 +147,7 @@ const struct option sh_options[] = {
{ "notify", 'b',OF_ANY },
{ "nounset",'u',OF_ANY },
{ "physical", 0,OF_ANY }, /* non-standard */
+   { "pipefail", 0,OF_ANY }, /* non-standard */
{ "posix",0,OF_ANY }, /* non-standard */
{ "privileged", 'p',OF_ANY },
{ "restricted", 'r',OF_CMDLINE },
Index: bin/ksh/sh.h
===
RCS file: /d/cvs/src/bin/ksh/sh.h,v
retrieving revision 1.75
diff -u -p -r1.75 sh.h
--- bin/ksh/sh.h20 Feb 2019 23:59:17 -  1.75
+++ bin/ksh/sh.h6 Jul 2020 16:13:59 -
@@ -158,6 +158,7 @@ enum sh_flag {
FNOTIFY,/* -b: asynchronous job completion notification */
FNOUNSET,   /* -u: using an unset var is an error */
FPHYSICAL,  /* -o physical: don't do 

ftp(1) double free

2020-06-20 Thread Jeremie Courreges-Anglas


Small bug, but still...

naddy@ reported a double free when interrupting (^C) ftp(1) at the end
or url_get().  If that happens, the SIGINT handler longjmps and the
cleanup path is taken a second time.  To avoid this, restore the
previous SIGINT handler in each possible error path.  I chose to restore
it earlier in the successful path to avoid too much duplication.
file_get() gets the same treatment.

Note that naddy hit this because of another bug causing ftp_close()
to stall, and this depends on the server you're talking to.  You can
insert a sleep(10); call before "return (rval);" to help reproduce the
crash.

ok?


Index: fetch.c
===
RCS file: /d/cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.195
diff -u -p -p -u -r1.195 fetch.c
--- fetch.c 20 Jun 2020 09:59:48 -  1.195
+++ fetch.c 21 Jun 2020 00:12:10 -
@@ -261,6 +261,7 @@ file_get(const char *path, const char *o
for (cp = buf; len > 0; len -= wlen, cp += wlen) {
if ((wlen = write(out, cp, len)) == -1) {
warn("Writing %s", savefile);
+   signal(SIGINT, oldintr);
signal(SIGINFO, oldinti);
goto cleanup_copy;
}
@@ -274,6 +275,7 @@ file_get(const char *path, const char *o
}
}
save_errno = errno;
+   signal(SIGINT, oldintr);
signal(SIGINFO, oldinti);
if (hash && !progress && bytes > 0) {
if (bytes < mark)
@@ -288,7 +290,6 @@ file_get(const char *path, const char *o
progressmeter(1, NULL);
if (verbose)
ptransfer(0);
-   (void)signal(SIGINT, oldintr);
 
rval = 0;
 
@@ -1032,6 +1033,7 @@ noslash:
oldinti = signal(SIGINFO, psummary);
if (chunked) {
error = save_chunked(fin, tls, out, buf, buflen);
+   signal(SIGINT, oldintr);
signal(SIGINFO, oldinti);
if (error == -1)
goto cleanup_url_get;
@@ -1041,6 +1043,7 @@ noslash:
for (cp = buf; len > 0; len -= wlen, cp += wlen) {
if ((wlen = write(out, cp, len)) == -1) {
warn("Writing %s", savefile);
+   signal(SIGINT, oldintr);
signal(SIGINFO, oldinti);
goto cleanup_url_get;
}
@@ -1054,6 +1057,7 @@ noslash:
}
}
save_errno = errno;
+   signal(SIGINT, oldintr);
signal(SIGINFO, oldinti);
if (hash && !progress && bytes > 0) {
if (bytes < mark)
@@ -1080,7 +1084,6 @@ noslash:
 
if (verbose)
ptransfer(0);
-   (void)signal(SIGINT, oldintr);
 
rval = 0;
goto cleanup_url_get;


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



Re: official ports vs DEBUG_PACKAGES

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

Take a look at -fdebug-prefix-map.

  https://reproducible-builds.org/docs/build-path/

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



Re: net80211: fix capinfo in assoc request

2020-05-18 Thread Jeremie Courreges-Anglas
On Thu, May 14 2020, Stefan Sperling  wrote:
> The capablities info field in an association request contains an ESS bit
> which is set if the sender is an access point (there are other cases but
> they don't matter for us; see 802.11-2012 8.4.1.4 if you are interested).
>
> This bit is set when OpenBSD clients send an association request to an AP.
> This seems wrong. The ESS bit should be zero when sent by clients.
>
> Noticed while looking over packet captures for an unrelated issue.
>
> ok?

Makes sense and works for me, ok jca@

> diff 4a0fa473f5ea308b63ffd39645f73b2195291973 /usr/src
> blob - 7952471d5bb369c9bb844966425fffc892a71038
> file + sys/net80211/ieee80211_output.c
> --- sys/net80211/ieee80211_output.c
> +++ sys/net80211/ieee80211_output.c
> @@ -1384,7 +1384,7 @@ ieee80211_get_assoc_req(struct ieee80211com *ic, struc
>   return NULL;
>  
>   frm = mtod(m, u_int8_t *);
> - capinfo = IEEE80211_CAPINFO_ESS;
> + capinfo = 0;
>   if (ic->ic_flags & IEEE80211_F_WEPON)
>   capinfo |= IEEE80211_CAPINFO_PRIVACY;
>   if ((ic->ic_flags & IEEE80211_F_SHPREAMBLE) &&
>

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



acpi(4): battery and A/C status after resume

2020-05-17 Thread Jeremie Courreges-Anglas
On Sat, Jan 25 2020, Jeremie Courreges-Anglas  wrote:
> So I have this diff for apmd -z/-Z that uses APM_POWER_CHANGE events to
> trigger autosuspend.  It works fine except for one glitch: if I plug the
> AC cable and then resume, apmd will receive another APM_POWER_CHANGE
> event and read the power info only to find that the AC is *unplugged*.
> So apmd happily suspends my system again.  If I resume once more, the AC
> status is correctly detected and apmd(8) doesn't suspend the system again.
>
> What happens is that acpibat_notify runs because of ACPIDEV_POLL, it
> grabs battery data and sends an APM_POWER_CHANGE event to apmd.  At this
> point apmd may see out of date data from acpiac(4).
>
> acpi_sleep_task later forces a refresh of (in order) acpiac(4),
> acpibat(4) and acpisbs(4), but in my use case it's too late, the window
> with stale acpiac(4) data is too long.

Even though apmd(8) -z/-Z isn't negatively affected any more, this
problem holds.  acpibat(4) can send APM_POWER_CHANGE events to userland
before the acpi thread refreshes acpiac(4) status. You can easily check
this by running apmd -d and going through zzz cycles with A/C
plugged/unplugged.

I think we should show a consistent state to userland, even if the apm
emulation in acpi(4) is implemented using several drivers,

> The diff below refreshes AC status with notifications at DVACT_WAKEUP
> time.  Is is correct that DVACT_WAKEUP handlers will always run before
> the ACPI task queue is processed?  "No" seems unlikely but I'd rather
> ask...

The answer was "yes"...

> I hear we prefer running ACPI code in its dedicated thread but there are
> other drivers in dev/acpi that run ACPI code in DVACT_WAKEUP, so I'm
> assuming it is fine. :)

Indeed it's fine.  The acpi thread runs acpi_sleep_state() and thus the
DVACT_WAKEUP handlers.  I guess that stuff looked like pure magic to me
at that time.


The diff below teaches acpiac(4), acpibat(4) and acpisbs(4) to refresh
their state using DVACT_WAKEUP handlers instead of using the *_notify
handlers in acpi_sleep_task().  This ensures a consistent state right
after resume, and makes those drivers less special.

I'm reusing the *_refresh functions in the new handlers, so send
APM_POWER_CHANGE events from the *_notify functions instead.

Finally, at the end of acpi_sleep_task() queue an APM_POWER_CHANGE event
so that userland can look at fresh power info as soon as possible.

Thoughts?  oks?


Index: acpi.c
===
RCS file: /d/cvs/src/sys/dev/acpi/acpi.c,v
retrieving revision 1.385
diff -u -p -r1.385 acpi.c
--- acpi.c  14 May 2020 13:07:10 -  1.385
+++ acpi.c  17 May 2020 12:08:11 -
@@ -1942,20 +1942,11 @@ void
 acpi_sleep_task(void *arg0, int sleepmode)
 {
struct acpi_softc *sc = arg0;
-   struct acpi_ac *ac;
-   struct acpi_bat *bat;
-   struct acpi_sbs *sbs;
 
/* System goes to sleep here.. */
acpi_sleep_state(sc, sleepmode);
-
-   /* AC and battery information needs refreshing */
-   SLIST_FOREACH(ac, >sc_ac, aac_link)
-   aml_notify(ac->aac_softc->sc_devnode, 0x80);
-   SLIST_FOREACH(bat, >sc_bat, aba_link)
-   aml_notify(bat->aba_softc->sc_devnode, 0x80);
-   SLIST_FOREACH(sbs, >sc_sbs, asbs_link)
-   aml_notify(sbs->asbs_softc->sc_devnode, 0x80);
+   /* Tell userland to recheck A/C and battery status */
+   acpi_record_event(sc, APM_POWER_CHANGE);
 }
 
 #endif /* SMALL_KERNEL */
Index: acpiac.c
===
RCS file: /d/cvs/src/sys/dev/acpi/acpiac.c,v
retrieving revision 1.32
diff -u -p -r1.32 acpiac.c
--- acpiac.c9 May 2020 00:40:48 -   1.32
+++ acpiac.c9 May 2020 00:55:46 -
@@ -33,13 +33,18 @@
 
 int  acpiac_match(struct device *, void *, void *);
 void acpiac_attach(struct device *, struct device *, void *);
+int  acpiac_activate(struct device *, int);
 int  acpiac_notify(struct aml_node *, int, void *);
 
 void acpiac_refresh(void *);
 int acpiac_getpsr(struct acpiac_softc *);
 
 struct cfattach acpiac_ca = {
-   sizeof(struct acpiac_softc), acpiac_match, acpiac_attach
+   sizeof(struct acpiac_softc),
+   acpiac_match,
+   acpiac_attach,
+   NULL,
+   acpiac_activate,
 };
 
 struct cfdriver acpiac_cd = {
@@ -92,6 +97,21 @@ acpiac_attach(struct device *parent, str
acpiac_notify, sc, ACPIDEV_NOPOLL);
 }
 
+int
+acpiac_activate(struct device *self, int act)
+{
+   struct acpiac_softc *sc = (struct acpiac_softc *)self;
+
+   switch (act) {
+   case DVACT_WAKEUP:
+   acpiac_refresh(sc);
+   dnprintf(10, "A/C status: %d\n", sc->sc_ac_stat);
+   break;
+   }
+
+   return (0);
+}
+
 void
 acpiac_refresh(void *arg)

Re: acpibat(4): remaining battery power with multiple batteries

2020-05-11 Thread Jeremie Courreges-Anglas
On Mon, May 11 2020, "Theo de Raadt"  wrote:
> You sure about that?  In this code, acpi.c is collecting abstracted
> information from the 3 battery drivers.

(One A/C driver and two battery drivers.)

> Maybe one of the drivers
> isn't providing enough information?
>
> Mark Kettenis  wrote:
>
>> This wouldn't do the right thing if you have both acpibat(4) and
>> acpisbs(4), but I think that is already broken.

acpibat(4) doesn't attach if acpisbs(4) is present:

  int
  acpibat_match(struct device *parent, void *match, void *aux)
  {
struct acpi_attach_args *aa = aux;
struct cfdata   *cf = match;

if (((struct acpi_softc *)parent)->sc_havesbs)
return (0);


I guess this could be made more explicit by splitting those loops.

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



Re: acpibat(4): remaining battery power with multiple batteries

2020-05-11 Thread Jeremie Courreges-Anglas
On Fri, May 08 2020, Jeremie Courreges-Anglas  wrote:
> Overall remaining battery power is currently the average of the
> remaining power (in percents) of each battery.  This is misleading if
> your laptop uses a large external battery which drains out first, and
> a smaller internal battery (common scheme on eg thinkpad machines).
> Overall battery power decreases much faster when you switch to the
> smaller battery.  This odd behavior was reported by a friend a few weeks
> ago.
>
> The diff below attempts to fix this: compute the sum of the remaining
> power and capacity, *then* compute the overall remaining percentage.
> No regression on my thinkpad x270 with two batteries of similar
> capacity.  I don't know whether the original reporter will be able to
> test this soon.
>
> The code touches the acpisbs(4) bits, hopefully without changing the
> current behavior.  acpisbs(4) currently doesn't support multiple
> batteries.
>
> Test reports, feedback, oks welcome.

I got successful test reports from benno@ and fellow lidstah (original
reporter).  With a 6600mAh external battery and a 2200mAh internal
battery, lidstah sees an overall battery percentage at ~22% instead of
~50% when the external battery drains out.

Still looking for oks :)


Index: acpi.c
===
RCS file: /d/cvs/src/sys/dev/acpi/acpi.c,v
retrieving revision 1.383
diff -u -p -p -u -r1.383 acpi.c
--- acpi.c  8 May 2020 11:18:01 -   1.383
+++ acpi.c  8 May 2020 15:04:50 -
@@ -3503,7 +3503,7 @@ acpiioctl(dev_t dev, u_long cmd, caddr_t
struct acpi_sbs *sbs;
struct apm_power_info *pi = (struct apm_power_info *)data;
int bats;
-   unsigned int remaining, rem, minutes, rate;
+   unsigned int capacity, remaining, minutes, rate;
int s;
 
if (!acpi_cd.cd_ndevs || APMUNIT(dev) != 0 ||
@@ -3554,7 +3554,8 @@ acpiioctl(dev_t dev, u_long cmd, caddr_t
pi->battery_life = 0;
pi->minutes_left = 0;
bats = 0;
-   remaining = rem = 0;
+   capacity = 0;
+   remaining = 0;
minutes = 0;
rate = 0;
SLIST_FOREACH(bat, >sc_bat, aba_link) {
@@ -3565,11 +3566,9 @@ acpiioctl(dev_t dev, u_long cmd, caddr_t
continue;
 
bats++;
-   rem = (bat->aba_softc->sc_bst.bst_capacity * 100) /
-   bat->aba_softc->sc_bix.bix_last_capacity;
-   if (rem > 100)
-   rem = 100;
-   remaining += rem;
+   capacity += bat->aba_softc->sc_bix.bix_last_capacity;
+   remaining += min(bat->aba_softc->sc_bst.bst_capacity,
+   bat->aba_softc->sc_bix.bix_last_capacity);
 
if (bat->aba_softc->sc_bst.bst_rate == BST_UNKNOWN)
continue;
@@ -3587,10 +3586,9 @@ acpiioctl(dev_t dev, u_long cmd, caddr_t
continue;
 
bats++;
-   rem = sbs->asbs_softc->sc_battery.rel_charge;
-   if (rem > 100)
-   rem = 100;
-   remaining += rem;
+   capacity += 100;
+   remaining += min(100,
+   sbs->asbs_softc->sc_battery.rel_charge);
 
if (sbs->asbs_softc->sc_battery.run_time ==
ACPISBS_VALUE_UNKNOWN)
@@ -3613,7 +3611,7 @@ acpiioctl(dev_t dev, u_long cmd, caddr_t
pi->minutes_left = 60 * minutes / rate;
 
/* running on battery */
-   pi->battery_life = remaining / bats;
+   pi->battery_life = remaining * 100 / capacity;
if (pi->battery_life > 50)
pi->battery_state = APM_BATT_HIGH;
else if (pi->battery_life > 25)

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



acpiac(4): no need to look at _STA

2020-05-08 Thread Jeremie Courreges-Anglas



acpiac(4) obtains the AC status using _PSR.  _STA is queried too but
nothing is done with the result.  AFAICS ACPI 6.3 doesn't say anything
about a relationship between _STA and _PSR.

Diff below removes the unused _STA query and renames the surrounding
plumbing.

Thoughts, oks?


Index: acpiac.c
===
RCS file: /d/cvs/src/sys/dev/acpi/acpiac.c,v
retrieving revision 1.31
diff -u -p -p -u -r1.31 acpiac.c
--- acpiac.c1 Jul 2018 19:40:49 -   1.31
+++ acpiac.c8 May 2020 17:10:18 -
@@ -36,7 +36,7 @@ void acpiac_attach(struct device *, stru
 int  acpiac_notify(struct aml_node *, int, void *);
 
 void acpiac_refresh(void *);
-int acpiac_getsta(struct acpiac_softc *);
+int acpiac_getpsr(struct acpiac_softc *);
 
 struct cfattach acpiac_ca = {
sizeof(struct acpiac_softc), acpiac_match, acpiac_attach
@@ -70,7 +70,7 @@ acpiac_attach(struct device *parent, str
sc->sc_acpi = (struct acpi_softc *)parent;
sc->sc_devnode = aa->aaa_node;
 
-   acpiac_getsta(sc);
+   acpiac_getpsr(sc);
printf(": AC unit ");
if (sc->sc_ac_stat == PSR_ONLINE)
printf("online\n");
@@ -97,27 +97,23 @@ acpiac_refresh(void *arg)
 {
struct acpiac_softc *sc = arg;
 
-   acpiac_getsta(sc);
+   acpiac_getpsr(sc);
sc->sc_sens[0].value = sc->sc_ac_stat;
acpi_record_event(sc->sc_acpi, APM_POWER_CHANGE);
 }
 
 int
-acpiac_getsta(struct acpiac_softc *sc)
+acpiac_getpsr(struct acpiac_softc *sc)
 {
-   int64_t sta;
+   int64_t psr;
 
-   if (aml_evalname(sc->sc_acpi, sc->sc_devnode, "_STA", 0, NULL, NULL)) {
-   dnprintf(10, "%s: no _STA\n",
-   DEVNAME(sc));
-   }
-
-   if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "_PSR", 0, NULL, 
)) {
+   if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "_PSR", 0, NULL, 
)) {
dnprintf(10, "%s: no _PSR\n",
DEVNAME(sc));
return (1);
}
-   sc->sc_ac_stat = sta;
+   sc->sc_ac_stat = psr;
+
return (0);
 }
 

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



acpibat(4): remaining battery power with multiple batteries

2020-05-08 Thread Jeremie Courreges-Anglas


Overall remaining battery power is currently the average of the
remaining power (in percents) of each battery.  This is misleading if
your laptop uses a large external battery which drains out first, and
a smaller internal battery (common scheme on eg thinkpad machines).
Overall battery power decreases much faster when you switch to the
smaller battery.  This odd behavior was reported by a friend a few weeks
ago.

The diff below attempts to fix this: compute the sum of the remaining
power and capacity, *then* compute the overall remaining percentage.
No regression on my thinkpad x270 with two batteries of similar
capacity.  I don't know whether the original reporter will be able to
test this soon.

The code touches the acpisbs(4) bits, hopefully without changing the
current behavior.  acpisbs(4) currently doesn't support multiple
batteries.

Test reports, feedback, oks welcome.


Index: acpi.c
===
RCS file: /d/cvs/src/sys/dev/acpi/acpi.c,v
retrieving revision 1.383
diff -u -p -p -u -r1.383 acpi.c
--- acpi.c  8 May 2020 11:18:01 -   1.383
+++ acpi.c  8 May 2020 15:04:50 -
@@ -3503,7 +3503,7 @@ acpiioctl(dev_t dev, u_long cmd, caddr_t
struct acpi_sbs *sbs;
struct apm_power_info *pi = (struct apm_power_info *)data;
int bats;
-   unsigned int remaining, rem, minutes, rate;
+   unsigned int capacity, remaining, minutes, rate;
int s;
 
if (!acpi_cd.cd_ndevs || APMUNIT(dev) != 0 ||
@@ -3554,7 +3554,8 @@ acpiioctl(dev_t dev, u_long cmd, caddr_t
pi->battery_life = 0;
pi->minutes_left = 0;
bats = 0;
-   remaining = rem = 0;
+   capacity = 0;
+   remaining = 0;
minutes = 0;
rate = 0;
SLIST_FOREACH(bat, >sc_bat, aba_link) {
@@ -3565,11 +3566,9 @@ acpiioctl(dev_t dev, u_long cmd, caddr_t
continue;
 
bats++;
-   rem = (bat->aba_softc->sc_bst.bst_capacity * 100) /
-   bat->aba_softc->sc_bix.bix_last_capacity;
-   if (rem > 100)
-   rem = 100;
-   remaining += rem;
+   capacity += bat->aba_softc->sc_bix.bix_last_capacity;
+   remaining += min(bat->aba_softc->sc_bst.bst_capacity,
+   bat->aba_softc->sc_bix.bix_last_capacity);
 
if (bat->aba_softc->sc_bst.bst_rate == BST_UNKNOWN)
continue;
@@ -3587,10 +3586,9 @@ acpiioctl(dev_t dev, u_long cmd, caddr_t
continue;
 
bats++;
-   rem = sbs->asbs_softc->sc_battery.rel_charge;
-   if (rem > 100)
-   rem = 100;
-   remaining += rem;
+   capacity += 100;
+   remaining += min(100,
+   sbs->asbs_softc->sc_battery.rel_charge);
 
if (sbs->asbs_softc->sc_battery.run_time ==
ACPISBS_VALUE_UNKNOWN)
@@ -3613,7 +3611,7 @@ acpiioctl(dev_t dev, u_long cmd, caddr_t
pi->minutes_left = 60 * minutes / rate;
 
/* running on battery */
-   pi->battery_life = remaining / bats;
+   pi->battery_life = remaining * 100 / capacity;
if (pi->battery_life > 50)
pi->battery_state = APM_BATT_HIGH;
else if (pi->battery_life > 25)

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



Re: ksh: Use typedef for function pointer

2020-05-08 Thread Jeremie Courreges-Anglas
On Thu, May 07 2020, Michael Forney  wrote:
> I originally submitted this patch as a portability fix to Brian
> Callahan's oksh, but he suggested I submit it here instead.

Out of curiosity, do you run (o)ksh on machines where this matters?

> Conversion of function pointer to void pointer is not allowed in
> ISO C, though POSIX requires it for dlsym(). However, here we are
> also comparing function pointer to void pointer with the == operator
> without using a cast, which is a constraint error[0].
>
> Rather than add a cast, we can just use a typedef here for the
> function pointer type, avoiding any C extensions, and adding a bit
> of type-safety.

Aside from the increased portability, the code doesn't look worse, and
fewer uses of void * is nice.  I'll commit this soon if I don't hear
objections (oks also welcome, obviously).

> [0] https://port70.net/~nsz/c/c11/n1570.html#6.5.9p2
> ---
>  bin/ksh/emacs.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/bin/ksh/emacs.c b/bin/ksh/emacs.c
> index aa2cceb657d..b63735956a3 100644
> --- a/bin/ksh/emacs.c
> +++ b/bin/ksh/emacs.c
> @@ -41,8 +41,10 @@ static Areaaedit;
>  #define  KEOL1   /* ^M, ^J */
>  #define  KINTR   2   /* ^G, ^C */
>  
> +typedef int (*kb_func)(int);
> +
>  struct   x_ftab {
> - int (*xf_func)(int c);
> + kb_func xf_func;
>   const char  *xf_name;
>   short   xf_flags;
>  };
> @@ -861,7 +863,7 @@ x_eot_del(int c)
>   return (x_del_char(c));
>  }
>  
> -static void *
> +static kb_func
>  kb_find_hist_func(char c)
>  {
>   struct kb_entry *k;
> @@ -1315,7 +1317,7 @@ kb_del(struct kb_entry *k)
>  }
>  
>  static struct kb_entry *
> -kb_add_string(void *func, void *args, char *str)
> +kb_add_string(kb_func func, void *args, char *str)
>  {
>   unsigned intele, count;
>   struct kb_entry *k;
> @@ -1350,7 +1352,7 @@ kb_add_string(void *func, void *args, char *str)
>  }
>  
>  static struct kb_entry *
> -kb_add(void *func, ...)
> +kb_add(kb_func func, ...)
>  {
>   va_list ap;
>   unsigned char   ch;

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



Re: ld.so and ldconfig manpage nits

2020-05-08 Thread Jeremie Courreges-Anglas
On Fri, May 08 2020, Miod Vallat  wrote:
> This ensures consistent spelling of set-{user,group}-ID, and also
> mentions LD_DEBUG is ignored by ld.so for such binaries.

Committed, thanks!

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



Re: pfctl_parser.c vs. __KAME

2020-05-03 Thread Jeremie Courreges-Anglas
On Sun, May 03 2020, Alexandr Nedvedicky  wrote:
> Hello,

Hi Sashan,

> the question has popped up while on recent code review of some Solaris 
> specific
> bug fixes: do we still need a code in diff below or is it OK to proceed
> and commit the diff?
>
> The chunk below uses bytes 2 and 3 to derive a scope of link local address.  
> It
> seems to me those bytes/octets are always zero in IPv6 link scope addresses
> these days, unless I'm missing something.  I was not able to identify its
> counter part in current kernel, which would make octets 2 & 3 non-zero, so it
> makes me thinking it should be OK to remove the whole chunk below.

See in6_embedscope and in6_recoverscope, IN6_IS_SCOPE_EMBED etc.  Dunno
about this specific piece of code in pfctl, but the "kame hack" is still
here and you really want to double check before removing such chunks in
userland.

>
> thanks and
> regards
> sashan
>
> note __KAME__ is defined, the chunk below is getting compiled currently.
>
> 8<---8<---8<--8<
> diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
> index cef0aa2474f..24175de046c 100644
> --- a/sbin/pfctl/pfctl_parser.c
> +++ b/sbin/pfctl/pfctl_parser.c
> @@ -1361,21 +1361,6 @@ ifa_load(void)
>   err(1, "%s: calloc", __func__);
>   n->af = ifa->ifa_addr->sa_family;
>   n->ifa_flags = ifa->ifa_flags;
> -#ifdef __KAME__
> - if (n->af == AF_INET6 &&
> - IN6_IS_ADDR_LINKLOCAL(&((struct sockaddr_in6 *)
> - ifa->ifa_addr)->sin6_addr) &&
> - ((struct sockaddr_in6 *)ifa->ifa_addr)->sin6_scope_id ==
> - 0) {
> - struct sockaddr_in6 *sin6;
> -
> - sin6 = (struct sockaddr_in6 *)ifa->ifa_addr;
> - sin6->sin6_scope_id = sin6->sin6_addr.s6_addr[2] << 8 |
> - sin6->sin6_addr.s6_addr[3];
> - sin6->sin6_addr.s6_addr[2] = 0;
> - sin6->sin6_addr.s6_addr[3] = 0;
> - }
> -#endif
>   n->ifindex = 0;
>   if (n->af == AF_LINK)
>   n->ifindex = ((struct sockaddr_dl *)
>

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



Re: resolv.conf(5) says options inet6 does nothing

2020-04-25 Thread Jeremie Courreges-Anglas
On Sat, Apr 25 2020, Jeremie Courreges-Anglas  wrote:
> On Fri, Apr 24 2020, Jeremie Courreges-Anglas  wrote:
>> On Thu, Apr 23 2020, Eric Faurot  wrote:
>>> On Thu, Apr 23, 2020 at 10:34:39AM -0600, Theo de Raadt wrote:
>>>> It says the keyword gets parsed, but then does performs no action.
>>>> 
>>>> But that is different from not parsing it.
>>>> 
>>>> Additionally, this explains an option which other systems support, and
>>>> by explaining it this way, it is also explaining our behaviour in case
>>>> of inet6 vs inet4 conditions.
>>>> 
>>>> So... I think it should stay.  Eric, do you have an opinion?
>>>
>>> The doc lies because the inet6 option does not set the RES_USE_INET6
>>> flag as stated.  I think we should leave the entry in the doc but fix
>>> the wording to say it's there for historical reasons and does nothing.
>>
>> In the diff below I document the behavior on other operating systems
>> since this looks useful to Claudio.  How does it read?
>>
>> (I really think this should go but if it doesn't I don't want it to stay
>> misleading as it is now.)
>>
>>> If we want to resurrect that option at some point, maybe we can
>>> consider making it set the RES_USE_INET6 flag but that feels like a
>>> bad idea right now, and this flag is apparently deprecated.
>>
>> I wholeheartedly with this, in my book RES_USE_INET6 and "options inet6"
>> are just early IPv6 experiments that went nowhere.
>> "options inet6" would break most programs that use gethostbyname(3).
>>
>>> But we should also fix the manpage for res_init(3) as the description
>>> of the flag is wrong too.
>>
>> Diff below.  Thoughts, oks?
>
> Committed, thanks.
>
> Eric suggested that we mark this option as deprecated.  Proposal:

New proposal after discussing this with Theo.


Index: net/res_init.3
===
RCS file: /cvs/src/lib/libc/net/res_init.3,v
retrieving revision 1.2
diff -u -p -r1.2 res_init.3
--- net/res_init.3  25 Apr 2020 14:30:05 -  1.2
+++ net/res_init.3  25 Apr 2020 15:59:51 -
@@ -183,6 +183,10 @@ feature.
 With this option
 .Xr gethostbyname 3
 will return IPv6 addresses if available.
+This option is deprecated, software should use the
+.Xr getaddrinfo 3
+interface instead of modifying the behavior of
+.Xr gethostbyname 3 .
 On some operating systems this option also causes IPv4 addresses to be
 returned as IPv4-mapped IPv6 addresses.
 For example, 10.1.1.1 will be returned as :::10.1.1.1.

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



Re: resolv.conf(5) says options inet6 does nothing

2020-04-25 Thread Jeremie Courreges-Anglas
On Sat, Apr 25 2020, "Theo de Raadt"  wrote:
> This wording doesn't make any sense to me.
>
> When we use the term deprecate, we are talking about interfaces.
> But gethostbyname isn't actually deprecated.  getaddrinfo is
> an alternative interface rather than gethostbyname.  But gethostbyname
> isn't deprecated.
> I don't understand what the addition mean.

"This option" kinda implies we're talking about RES_USE_INET6.
Why couldn't it be deprecated?

What would you suggest?  "Using this option together with
gethostbyname(3) is deprecated/discouraged in favor of getaddrinfo(3)"?

> At most I think it can
> be said the option is ineffective?

This option is effective, last commit just made this clear.

>> Index: net/res_init.3
>> ===
>> RCS file: /cvs/src/lib/libc/net/res_init.3,v
>> retrieving revision 1.2
>> diff -u -p -r1.2 res_init.3
>> --- net/res_init.3   25 Apr 2020 14:30:05 -  1.2
>> +++ net/res_init.3   25 Apr 2020 14:38:39 -
>> @@ -183,6 +183,9 @@ feature.
>>  With this option
>>  .Xr gethostbyname 3
>>  will return IPv6 addresses if available.
>> +This option is deprecated in favor of the
>> +.Xr getaddrinfo 3
>> +interface.
>>  On some operating systems this option also causes IPv4 addresses to be
>>  returned as IPv4-mapped IPv6 addresses.
>>  For example, 10.1.1.1 will be returned as :::10.1.1.1.
>> 
>> 
>> -- 
>> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
>

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



Re: resolv.conf(5) says options inet6 does nothing

2020-04-25 Thread Jeremie Courreges-Anglas
On Fri, Apr 24 2020, Jeremie Courreges-Anglas  wrote:
> On Thu, Apr 23 2020, Eric Faurot  wrote:
>> On Thu, Apr 23, 2020 at 10:34:39AM -0600, Theo de Raadt wrote:
>>> It says the keyword gets parsed, but then does performs no action.
>>> 
>>> But that is different from not parsing it.
>>> 
>>> Additionally, this explains an option which other systems support, and
>>> by explaining it this way, it is also explaining our behaviour in case
>>> of inet6 vs inet4 conditions.
>>> 
>>> So... I think it should stay.  Eric, do you have an opinion?
>>
>> The doc lies because the inet6 option does not set the RES_USE_INET6
>> flag as stated.  I think we should leave the entry in the doc but fix
>> the wording to say it's there for historical reasons and does nothing.
>
> In the diff below I document the behavior on other operating systems
> since this looks useful to Claudio.  How does it read?
>
> (I really think this should go but if it doesn't I don't want it to stay
> misleading as it is now.)
>
>> If we want to resurrect that option at some point, maybe we can
>> consider making it set the RES_USE_INET6 flag but that feels like a
>> bad idea right now, and this flag is apparently deprecated.
>
> I wholeheartedly with this, in my book RES_USE_INET6 and "options inet6"
> are just early IPv6 experiments that went nowhere.
> "options inet6" would break most programs that use gethostbyname(3).
>
>> But we should also fix the manpage for res_init(3) as the description
>> of the flag is wrong too.
>
> Diff below.  Thoughts, oks?

Committed, thanks.

Eric suggested that we mark this option as deprecated.  Proposal:


Index: net/res_init.3
===
RCS file: /cvs/src/lib/libc/net/res_init.3,v
retrieving revision 1.2
diff -u -p -r1.2 res_init.3
--- net/res_init.3  25 Apr 2020 14:30:05 -  1.2
+++ net/res_init.3  25 Apr 2020 14:38:39 -
@@ -183,6 +183,9 @@ feature.
 With this option
 .Xr gethostbyname 3
 will return IPv6 addresses if available.
+This option is deprecated in favor of the
+.Xr getaddrinfo 3
+interface.
 On some operating systems this option also causes IPv4 addresses to be
 returned as IPv4-mapped IPv6 addresses.
 For example, 10.1.1.1 will be returned as :::10.1.1.1.


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



Re: resolv.conf(5) says options inet6 does nothing

2020-04-24 Thread Jeremie Courreges-Anglas
On Thu, Apr 23 2020, Eric Faurot  wrote:
> On Thu, Apr 23, 2020 at 10:34:39AM -0600, Theo de Raadt wrote:
>> It says the keyword gets parsed, but then does performs no action.
>> 
>> But that is different from not parsing it.
>> 
>> Additionally, this explains an option which other systems support, and
>> by explaining it this way, it is also explaining our behaviour in case
>> of inet6 vs inet4 conditions.
>> 
>> So... I think it should stay.  Eric, do you have an opinion?
>
> The doc lies because the inet6 option does not set the RES_USE_INET6
> flag as stated.  I think we should leave the entry in the doc but fix
> the wording to say it's there for historical reasons and does nothing.

In the diff below I document the behavior on other operating systems
since this looks useful to Claudio.  How does it read?

(I really think this should go but if it doesn't I don't want it to stay
misleading as it is now.)

> If we want to resurrect that option at some point, maybe we can
> consider making it set the RES_USE_INET6 flag but that feels like a
> bad idea right now, and this flag is apparently deprecated.

I wholeheartedly with this, in my book RES_USE_INET6 and "options inet6"
are just early IPv6 experiments that went nowhere.
"options inet6" would break most programs that use gethostbyname(3).

> But we should also fix the manpage for res_init(3) as the description
> of the flag is wrong too.

Diff below.  Thoughts, oks?


Index: share/man/man5/resolv.conf.5
===
RCS file: /d/cvs/src/share/man/man5/resolv.conf.5,v
retrieving revision 1.59
diff -u -p -r1.59 resolv.conf.5
--- share/man/man5/resolv.conf.524 Jan 2020 06:16:47 -  1.59
+++ share/man/man5/resolv.conf.524 Apr 2020 08:22:25 -
@@ -259,12 +259,13 @@ as is often the case with
 .Xr pppoe 4
 or with tunnels.
 .It Cm inet6
-Enables support for IPv6-only applications, by setting RES_USE_INET6 in
-_res.options (see
-.Xr res_init 3 ) .
 On
 .Ox
 this option does nothing.
+On some operating systems, this option enables IPv6 support in
+.Xr gethostbyname 3
+by setting RES_USE_INET6 in _res.options (see
+.Xr res_init 3 ) .
 .It Cm insecure1
 Do not require IP source address on the reply packet to be equal to the
 server's address.
Index: lib/libc/net/res_init.3
===
RCS file: /d/cvs/src/lib/libc/net/res_init.3,v
retrieving revision 1.1
diff -u -p -r1.1 res_init.3
--- lib/libc/net/res_init.3 30 Aug 2019 18:33:17 -  1.1
+++ lib/libc/net/res_init.3 24 Apr 2020 08:25:46 -
@@ -180,12 +180,14 @@ In the past, it turned off the legacy
 .Ev HOSTALIASES
 feature.
 .It Dv RES_USE_INET6
-Enables support for IPv6-only applications.
-This causes IPv4 addresses to be returned as an IPv4 mapped address.
+With this option
+.Xr gethostbyname 3
+will return IPv6 addresses if available.
+On some operating systems this option also causes IPv4 addresses to be
+returned as IPv4-mapped IPv6 addresses.
 For example, 10.1.1.1 will be returned as :::10.1.1.1.
-On
-.Ox
-this option does nothing.
+IPv4-mapped IPv6 addresses are not supported on
+.Ox .
 .It Dv RES_USE_EDNS0
 Attach an OPT pseudo-RR for the EDNS0 extension,
 as specified in RFC 2671.

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



Re: iwm(4) timeout cancellation fix

2020-04-23 Thread Jeremie Courreges-Anglas
On Thu, Apr 23 2020, Stefan Sperling  wrote:
> I have observed a uvm fault in ieee80211_mira_probe_timeout_up() while
> testing with iwm(4) and tcpbench:
>
> void
> ieee80211_mira_probe_timeout_up(void *arg)
> {
>   struct ieee80211_mira_node *mn = arg;
>   int s;
>
>   s = splnet();
>   mn->probe_timer_expired[IEEE80211_MIRA_PROBE_TO_UP] = 1;
>   DPRINTFN(3, ("probe up timeout fired\n"));
>   splx(s);
> }
>
> One obvious possibility is that the 'mn' pointer became invalid before the
> timeout was executed. But I am not certain what happened exactly; the info
> in ddb was inconclusive since the console switching ran into splassert
> failures and I didn't see a good backtrace. But r12 in 'show regs' contained
> the address of ieee80211_mira_probe_timeout_up() and it looked like the
> kernel was in softclock context.
>
> In any case, it looks like cancelling timeouts before scheduling the
> iwm_newstate_task can lead to a race:
>
>  - Timeouts are cancelled and iwm_newstate_task is scheduled
>  - Tx done interrupts feed frames to MiRA which adds a new timeout
>  - iwm_newstate_task runs and switches state without cancelling this timeout
>  
> So cancel timeouts when we are actually switching state in the task.
>
> While here, initialize MiRA timeouts and other rate scaling state earlier,
> when the node is allocated.
>
> ok?

Works fine so far on

  iwm0 at pci2 dev 0 function 0 "Intel Dual Band Wireless-AC 8265" rev 0x78, msi
  iwm0: hw rev 0x230, fw ver 34.0.1, address f8:59:71:xx:xx:xx

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



Re: resolv.conf(5) says options inet6 does nothing

2020-04-23 Thread Jeremie Courreges-Anglas
On Thu, Apr 23 2020, Jason McIntyre  wrote:
> On Thu, Apr 23, 2020 at 05:17:08PM +0200, Solene Rapenne wrote:
>> Is there a reason to keep this part in resolv.conf(5) about an option
>> doing nothing?
>> 
>> > options inet6
>> > Enables support for IPv6-only applications, by setting RES_USE_INET6
>> > in _res.options (see res_init(3)). On OpenBSD this option does
>> > nothing.
>> 
>> If we can remove it, here is the diff.
>> 
>
> hi.
>
> i guess if you did this, you'd need to look at res_init.3 too.

"options inet6" in resolv.conf(5) does bothing, it doesn't "set
RES_USE_INET6 in _res.options".  I think we can just delete any mention
of "options inet6" in resolv.conf(5).

RES_USE_INET6 as documented in res_init(3) should be investigated some
more.  It actually does *something* in the current code, but it
doesn't enable the IPv4-mapped IPv6 addresses which we don't support.

Maybe this option should just be deleted (probably safer to wait after
6.7 is released).  People should use getaddrinfo(3) instead.


> jmc
>
>> Index: resolv.conf.5
>> ===
>> RCS file: /home/cvs/src/share/man/man5/resolv.conf.5,v
>> retrieving revision 1.59
>> diff -u -p -r1.59 resolv.conf.5
>> --- resolv.conf.524 Jan 2020 06:16:47 -  1.59
>> +++ resolv.conf.523 Apr 2020 15:09:42 -
>> @@ -258,13 +258,6 @@ particularly if there is a reduced MTU,
>>  as is often the case with
>>  .Xr pppoe 4
>>  or with tunnels.
>> -.It Cm inet6
>> -Enables support for IPv6-only applications, by setting RES_USE_INET6 in
>> -_res.options (see
>> -.Xr res_init 3 ) .
>> -On
>> -.Ox
>> -this option does nothing.
>>  .It Cm insecure1
>>  Do not require IP source address on the reply packet to be equal to the
>>  server's address.
>> 
>

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



Re: find(1) -exec + and ARG_MAX

2020-04-09 Thread 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

2020-04-09 Thread 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



find(1) -exec + and ARG_MAX

2020-04-08 Thread 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: Kill cdev_mousewr_init()

2020-04-03 Thread Jeremie Courreges-Anglas
On Fri, Apr 03 2020, Martin Pieuchot  wrote:
> Unused macro, found while auditing d_poll() functions, ok?

ok

> Index: sys/conf.h
> ===
> RCS file: /cvs/src/sys/sys/conf.h,v
> retrieving revision 1.148
> diff -u -p -r1.148 conf.h
> --- sys/conf.h22 Jan 2020 23:06:05 -  1.148
> +++ sys/conf.h2 Apr 2020 15:29:59 -
> @@ -196,13 +196,6 @@ extern struct cdevsw cdevsw[];
>   (dev_type_stop((*))) enodev, 0, dev_init(c,n,poll), \
>   (dev_type_mmap((*))) enodev , 0, 0, dev_init(c,n,kqfilter) }
>  
> -/* open, close, read, write, ioctl, poll, nokqfilter */
> -#define  cdev_mousewr_init(c,n) { \
> - dev_init(c,n,open), dev_init(c,n,close), dev_init(c,n,read), \
> - dev_init(c,n,write), dev_init(c,n,ioctl), \
> - (dev_type_stop((*))) enodev, 0, dev_init(c,n,poll), \
> - (dev_type_mmap((*))) enodev }
> -
>  #define  cdev_notdef() { \
>   (dev_type_open((*))) enodev, (dev_type_close((*))) enodev, \
>   (dev_type_read((*))) enodev, (dev_type_write((*))) enodev, \
>

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



rpki-client: output.c static/const tweaks

2020-03-09 Thread Jeremie Courreges-Anglas


Claudio suggested[0] to restrict the visibility of three helper
functions in this file.  The diff below goes a bit further, sprinkling
some static and const magic to help the compiler generate better code.

ok?

[0] https://marc.info/?l=openbsd-tech=158375920102498=2


Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.25
diff -u -p -r1.25 extern.h
--- extern.h9 Mar 2020 23:50:01 -   1.25
+++ extern.h10 Mar 2020 00:09:25 -
@@ -370,9 +370,6 @@ extern int   outformats;
 extern char*outputdir;
 
 int outputfiles(struct vrp_tree *v);
-FILE   *output_createtmp(char *);
-voidoutput_cleantmp(void);
-int output_finish(FILE *);
 int output_bgpd(FILE *, struct vrp_tree *);
 int output_bird1v4(FILE *, struct vrp_tree *);
 int output_bird1v6(FILE *, struct vrp_tree *);
Index: output.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/output.c,v
retrieving revision 1.8
diff -u -p -r1.8 output.c
--- output.c9 Mar 2020 23:50:01 -   1.8
+++ output.c10 Mar 2020 00:09:25 -
@@ -29,12 +29,12 @@
 #include "extern.h"
 
 char   *outputdir;
-charoutput_tmpname[PATH_MAX];
-charoutput_name[PATH_MAX];
-
 int outformats;
 
-struct outputs {
+static char output_tmpname[PATH_MAX];
+static char output_name[PATH_MAX];
+
+static const struct outputs {
int  format;
char*name;
int (*fn)(FILE *, struct vrp_tree *);
@@ -48,8 +48,11 @@ struct outputs {
{ 0, NULL }
 };
 
-voidsig_handler(int);
-voidset_signal_handler(void);
+static FILE*output_createtmp(char *);
+static void output_cleantmp(void);
+static int  output_finish(FILE *);
+static void sig_handler(int);
+static void set_signal_handler(void);
 
 int
 outputfiles(struct vrp_tree *v)
@@ -89,7 +92,7 @@ outputfiles(struct vrp_tree *v)
return rc;
 }
 
-FILE *
+static FILE *
 output_createtmp(char *name)
 {
FILE *f;
@@ -113,7 +116,7 @@ output_createtmp(char *name)
return f;
 }
 
-int
+static int
 output_finish(FILE *out)
 {
if (fclose(out) != 0)
@@ -124,7 +127,7 @@ output_finish(FILE *out)
return 0;
 }
 
-void
+static void
 output_cleantmp(void)
 {
if (*output_tmpname)
@@ -135,7 +138,7 @@ output_cleantmp(void)
 /*
  * Signal handler that clears the temporary files.
  */
-void
+static void
 sig_handler(int sig __unused)
 {
output_cleantmp();
@@ -145,7 +148,7 @@ sig_handler(int sig __unused)
 /*
  * Set signal handler on panic signals.
  */
-void
+static void
 set_signal_handler(void)
 {
struct sigaction sa;


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



Re: rpki-client: check fflush on output files

2020-03-09 Thread Jeremie Courreges-Anglas
On Sat, Mar 07 2020, Claudio Jeker  wrote:
> On Sat, Mar 07, 2020 at 08:35:39AM +0100, Jeremie Courreges-Anglas wrote:
>> On Fri, Mar 06 2020, "Theo de Raadt"  wrote:
>> >> Jeremie Courreges-Anglas  wrote:
>> >> > 
>> >> > 
>> >> > Checking the return value of fprintf is good but not enough to ensure
>> >> > that data has properly been written to the file without an error.  To do
>> >> > that we can use fflush(3) in a single place.
>> [redacted]
>> >> How about checking the return value of fclose() in output_finish() 
>> >> instead?
>> > Oh you want to reach the error-reporting code...
>> 
>> And the cleanup-on-error code.  Doing it here looks more appealing than
>> adding
>>  if (fflush(out) != 0)
>>  return -1;
>> 
>> at the end of all the output_* functions.
>> 
>> If you're careful about write errors you can avoid feeding an
>> incomplete/garbage file to your BGP daemon.  The code was already
>> careful, but did not account for buffering.  This is what this patch
>> tries to address.
>> 
>> >> > Build-tested only.  ok?
>> >> > 
>> >> > Bonus: in output_finish(), "out = NULL;" is pointless, so zap it.
>> >> > I suspect it's a remnant from a time where the output FILE * was
>> >> > a global.  That would be a separate commit.
>> 
>> Diff below again for convenience,
>> 
>> 
>> Index: output.c
>> ===
>> RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
>> retrieving revision 1.6
>> diff -u -p -p -u -r1.6 output.c
>> --- output.c 6 Mar 2020 17:36:42 -   1.6
>> +++ output.c 6 Mar 2020 23:04:18 -
>> @@ -71,7 +71,7 @@ outputfiles(struct vrp_tree *v)
>>  rc = 1;
>>  continue;
>>  }
>> -if ((*outputs[i].fn)(fout, v) != 0) {
>> +if ((*outputs[i].fn)(fout, v) != 0 || fflush(fout) != 0) {
>>  logx("output for %s format failed", outputs[i].name);
>>  fclose(fout);
>>  output_cleantmp();
>> @@ -112,8 +112,6 @@ void
>>  output_finish(FILE *out)
>>  {
>>  fclose(out);
>> -out = NULL;
>> -
>>  rename(output_tmpname, output_name);
>>  output_tmpname[0] = '\0';
>>  }
>> 
>
> I think it would be better to pick up the fclose error in output_finish()
> and while doing that also check for rename() errors. At least those errors
> should be logged.

Agreed.  Here's an updated diff that tests the return value of
output_finish().  Suggestions welcome for the error message in this
case.

It also drops the extra "out = NULL;", and replaces logx() calls with
warn(3) in this file.  I see no reason to drop those messages and errno
information.  logx() seems used mostly for statistics.

Thoughts, tests / oks?


Index: extern.h
===
RCS file: /d/cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.24
diff -u -p -r1.24 extern.h
--- extern.h6 Mar 2020 17:36:42 -   1.24
+++ extern.h9 Mar 2020 08:19:20 -
@@ -372,7 +372,7 @@ extern char* outputdir;
 int outputfiles(struct vrp_tree *v);
 FILE   *output_createtmp(char *);
 voidoutput_cleantmp(void);
-voidoutput_finish(FILE *);
+int output_finish(FILE *);
 int output_bgpd(FILE *, struct vrp_tree *);
 int output_bird1v4(FILE *, struct vrp_tree *);
 int output_bird1v6(FILE *, struct vrp_tree *);
Index: output.c
===
RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
retrieving revision 1.6
diff -u -p -r1.6 output.c
--- output.c6 Mar 2020 17:36:42 -   1.6
+++ output.c9 Mar 2020 08:20:39 -
@@ -67,18 +67,23 @@ outputfiles(struct vrp_tree *v)
 
fout = output_createtmp(outputs[i].name);
if (fout == NULL) {
-   logx("cannot create %s", outputs[i].name);
+   warn("cannot create %s", outputs[i].name);
rc = 1;
continue;
}
if ((*outputs[i].fn)(fout, v) != 0) {
-   logx("output for %s format failed", outputs[i].name);
+   warn("output for %s format failed", outputs[i].name);
fclose(fout);
output_cleantmp()

Re: rpki-client: check fflush on output files

2020-03-06 Thread Jeremie Courreges-Anglas
On Fri, Mar 06 2020, "Theo de Raadt"  wrote:
>> Jeremie Courreges-Anglas  wrote:
>> > 
>> > 
>> > Checking the return value of fprintf is good but not enough to ensure
>> > that data has properly been written to the file without an error.  To do
>> > that we can use fflush(3) in a single place.
[redacted]
>> How about checking the return value of fclose() in output_finish() instead?
> Oh you want to reach the error-reporting code...

And the cleanup-on-error code.  Doing it here looks more appealing than
adding
if (fflush(out) != 0)
return -1;

at the end of all the output_* functions.

If you're careful about write errors you can avoid feeding an
incomplete/garbage file to your BGP daemon.  The code was already
careful, but did not account for buffering.  This is what this patch
tries to address.

>> > Build-tested only.  ok?
>> > 
>> > Bonus: in output_finish(), "out = NULL;" is pointless, so zap it.
>> > I suspect it's a remnant from a time where the output FILE * was
>> > a global.  That would be a separate commit.

Diff below again for convenience,


Index: output.c
===
RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
retrieving revision 1.6
diff -u -p -p -u -r1.6 output.c
--- output.c6 Mar 2020 17:36:42 -   1.6
+++ output.c6 Mar 2020 23:04:18 -
@@ -71,7 +71,7 @@ outputfiles(struct vrp_tree *v)
rc = 1;
continue;
}
-   if ((*outputs[i].fn)(fout, v) != 0) {
+   if ((*outputs[i].fn)(fout, v) != 0 || fflush(fout) != 0) {
logx("output for %s format failed", outputs[i].name);
fclose(fout);
output_cleantmp();
@@ -112,8 +112,6 @@ void
 output_finish(FILE *out)
 {
fclose(out);
-   out = NULL;
-
rename(output_tmpname, output_name);
output_tmpname[0] = '\0';
 }


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



rpki-client: check fflush on output files

2020-03-06 Thread Jeremie Courreges-Anglas


Checking the return value of fprintf is good but not enough to ensure
that data has properly been written to the file without an error.  To do
that we can use fflush(3) in a single place.

Build-tested only.  ok?

Bonus: in output_finish(), "out = NULL;" is pointless, so zap it.
I suspect it's a remnant from a time where the output FILE * was
a global.  That would be a separate commit.


Index: output.c
===
RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
retrieving revision 1.6
diff -u -p -p -u -r1.6 output.c
--- output.c6 Mar 2020 17:36:42 -   1.6
+++ output.c6 Mar 2020 19:09:13 -
@@ -71,7 +71,7 @@ outputfiles(struct vrp_tree *v)
rc = 1;
continue;
}
-   if ((*outputs[i].fn)(fout, v) != 0) {
+   if ((*outputs[i].fn)(fout, v) != 0 || fflush(fout) != 0) {
logx("output for %s format failed", outputs[i].name);
fclose(fout);
output_cleantmp();
@@ -112,8 +112,6 @@ void
 output_finish(FILE *out)
 {
fclose(out);
-   out = NULL;
-
rename(output_tmpname, output_name);
output_tmpname[0] = '\0';
 }



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



Re: BIRD 1.x/2.x support at rpki-client

2020-03-06 Thread Jeremie Courreges-Anglas
On Fri, Mar 06 2020, Sebastian Benoit  wrote:
> Job Snijders(j...@openbsd.org) on 2020.03.06 17:31:13 +:
>> I have a small suggestion, in some deployments I saw the convention to
>> name it as following so it is clear the data came from user provided
>> data rather than internal bird structures 
>> 
>> I tested Benno's patch against BIRD 1.6.6 - wfm.
>
> thanks.
>
> I did not commit this bit, and i have not substantiated opinion on it.
> If thats how bird users do it, commit it ;)

I guess the manpage needs an update (-T tablename).

>> 
>> Index: main.c
>> ===
>> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
>> retrieving revision 1.58
>> diff -u -p -r1.58 main.c
>> --- main.c   11 Feb 2020 18:41:39 -  1.58
>> +++ main.c   6 Mar 2020 17:25:56 -
>> @@ -156,7 +156,7 @@ static void  build_chain(const struct aut
>>  static void build_crls(const struct auth *, struct crl_tree *,
>>  STACK_OF(X509_CRL) **);
>>  
>> -const char  *bird_tablename = "roa";
>> +const char  *bird_tablename = "ROAS";
>>  
>>  int  verbose;
>>  
>> 
>

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



Re: BIRD 1.x/2.x support at rpki-client

2020-03-06 Thread Jeremie Courreges-Anglas
On Fri, Mar 06 2020, Sebastian Benoit  wrote:
> Robert Scheck(rob...@fedoraproject.org) on 2020.03.06 14:02:26 +0100:
>> On Fri, 06 Mar 2020, Job Snijders wrote:
>> > I believe Robert is referring to this snippet of code:
>> > 
>> > 
>> > https://patch-diff.githubusercontent.com/raw/kristapsdz/rpki-client/pull/21.patch
>> 
>> Exactly.
>
> Ah, i thought it was some diff to bird! Thanks, i'll tkae care of it.

One tweak, fwiw:

--8<--
if (fprintf(out, "define force_roa_table_update = %ld;\n\n"
"roa4 table %s4;\nroa6 table %s6;\n\n"
"protocol static {\n\troa4 { table %s4; };\n\n",
(long) now, bird_tablename, bird_tablename, bird_tablename) < 0)
return -1;
-->8--

The pattern to print time_t is to use %lld and a (long long) cast.

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



Re: apmd: fix autoaction timeout

2020-02-17 Thread Jeremie Courreges-Anglas
On Sat, Feb 15 2020, Jeremie Courreges-Anglas  wrote:
> On Fri, Feb 14 2020, Scott Cheloha  wrote:
>> On Thu, Feb 13, 2020 at 02:08:32PM +0100, Jeremie Courreges-Anglas wrote:
>>> On Wed, Feb 12 2020, Scott Cheloha  wrote:
>>> > On Wed, Feb 12, 2020 at 01:35:22PM +0100, Jeremie Courreges-Anglas wrote:
>>> >> On Wed, Feb 12 2020, Jeremie Courreges-Anglas  wrote:

[...]

>>> >> On top of the previous diff, here's a diff to block autoaction for 60
>>> >> seconds after:
>>> >> - autoaction triggers; this prevents apmd from sending multiple suspend
>>> >> requests before the system goes to sleep
>>> >> - a resume happens; this gives you 60 seconds to fetch and plug your AC
>>> >> cable if you notice you're low on power
>>> >> 
>>> >> A side effect is that apmd now ignores stale acpiac(4) data at resume
>>> >> time, so it apmd doesn't suspend the system again when you resume with
>>> >> a low battery and AC plugged.
>>> >> 
>>> >> cc'ing Scott since he has a thing for everything time-related. :)
>>> >
>>> > For the first case, is there any way you can detect that a suspend is
>>> > in-progress but not done yet?
>>> 
>>> Well, apmd could record that it asked the kernel for a suspend/hibernate
>>> and skip autoaction as long as it doesn't get a resume event.
>>
>> Hmmm.  So what happens if the suspend/hibernate fails?  Could apmd(8)
>> get stuck waiting for a resume that will never happen?
>
> Well, if suspend fails maybe there's no point in having apmd retry
> a suspend. Also what would get stuck is only the autoaction behavior,
> the rest of apmd would keep on working as usual.
>
> acpi_sleep_state seems to properly send a RESUME event even if
> suspend/hibernate fails, except in one error case.
>
> But depending on a resume event is not portable, the APM code in i386
> and loongson doesn't notify userland about resumes. Something that ought
> to be fixed.

Looks like i386 apm(4) actually sends resume events, and I teached
loongson to send an APM_NORMAL_RESUME event too.  So unthrottling
autoaction using resume events has a chance to work on all relevant
platforms.

If autoaction asks for a suspend and the suspend fails and the kernel
fails to send a resume event, autoaction will stay disabled in apmd(8).
I think that's reasonable, after all, why would a second suspend request
succeed?

The diff below (on top of -current):
- blocks autoaction after it kicks in, until a resume event is received.
  This prevents autoaction from sending multiple suspend requests before
  suspend happens, and avoids spurious suspend/resume cycles.
- blocks autoaction for 60 seconds after a resume event, so that the
  user has time to control the system / disable apmd(8) if needed, etc

Thanks for the feedback so far.
Comments, tests and oks welcome.


Index: apmd.c
===
--- apmd.c.orig
+++ apmd.c
@@ -368,18 +368,20 @@ resumed(int ctl_fd)
 }
 
 #define TIMO (10*60)   /* 10 minutes */
+#define AUTOACTION_GRACE_PERIOD (60)   /* 1mn after resume */
 
 int
 main(int argc, char *argv[])
 {
const char *fname = _PATH_APM_CTLDEV;
int ctl_fd, sock_fd, ch, suspends, standbys, hibernates, resumes;
-   int autoaction = 0;
+   int autoaction = 0, autoaction_inflight = 0;
int autolimit = 0;
int statonly = 0;
int powerstatus = 0, powerbak = 0, powerchange = 0;
int noacsleep = 0;
struct timespec ts = {TIMO, 0}, sts = {0, 0};
+   struct timespec last_resume = { 0, 0 };
struct apm_power_info pinfo;
const char *sockname = _PATH_APM_SOCKET;
const char *errstr;
@@ -566,6 +568,8 @@ main(int argc, char *argv[])
powerstatus = powerbak;
powerchange = 1;
}
+   clock_gettime(CLOCK_MONOTONIC, _resume);
+   autoaction_inflight = 0;
resumes++;
break;
case APM_POWER_CHANGE:
@@ -577,17 +581,30 @@ main(int argc, char *argv[])
 
if (!powerstatus && autoaction &&
autolimit > (int)pinfo.battery_life) {
+   struct timespec graceperiod, now;
+
+   graceperiod = last_resume;
+   graceperiod.tv_sec += AUTOACTION_GRACE_PERIOD;
+   clock_gettime(CLOCK_MONOTONIC, );
+
logmsg(LOG_NOTICE,
"estimated battery life %d%%&quo

loongson: apm(4) resume event

2020-02-15 Thread Jeremie Courreges-Anglas


I'd like to use resume events in apmd(8) to throttle autoaction (-z/-Z).
The acpi(4) code already sends the appropriate APM_NORMAL_RESUME event to
userland, and the i386 apm(4) seems to do the same.

Could someone with a loongson please build a kernel with the following
diff?  If you want to test runtime, run apmd -d and look for an

  apmevent 3 index xxx

message after resume.

oks welcome too.


Index: sys/arch/loongson/dev/apm.c
===
RCS file: /d/cvs/src/sys/arch/loongson/dev/apm.c,v
retrieving revision 1.33
diff -u -p -r1.33 apm.c
--- sys/arch/loongson/dev/apm.c 31 Dec 2019 13:48:31 -  1.33
+++ sys/arch/loongson/dev/apm.c 15 Feb 2020 07:54:08 -
@@ -434,5 +434,7 @@ apm_suspend(int state)
wsdisplay_resume();
 #endif
 
+   apm_record_event(APM_NORMAL_RESUME, "System", "resumed from sleep");
+
return rv;
 }


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



Re: apmd: fix autoaction timeout

2020-02-14 Thread Jeremie Courreges-Anglas
On Fri, Feb 14 2020, Scott Cheloha  wrote:
> On Thu, Feb 13, 2020 at 02:08:32PM +0100, Jeremie Courreges-Anglas wrote:
>> On Wed, Feb 12 2020, Scott Cheloha  wrote:
>> > On Wed, Feb 12, 2020 at 01:35:22PM +0100, Jeremie Courreges-Anglas wrote:
>> >> On Wed, Feb 12 2020, Jeremie Courreges-Anglas  wrote:
>> >> > On Sat, Jan 25 2020, Jeremie Courreges-Anglas  wrote:
>> >> >> The diff below improves the way apmd -z/-Z may trigger.
>> >> >>
>> >> >> I think the current behavior is bogus, incrementing and checking
>> >> >> apmtimeout like this doesn't make much sense.
>> >> >>
>> >> >> Here's a proposal:
>> >> >> - on APM_POWER_CHANGE events, check the battery level and trigger
>> >> >>   autoaction if needed.  This should be enough to make autoaction just
>> >> >>   work with drivers like acpibat(4).
>> >> >> - on kevent timeout (10mn by default now, maybe too long), also check
>> >> >>   the battery level and suspend if needed.  This should be useful only
>> >> >>   if your battery driver doesn't send any APM_POWER_CHANGE event.
>> >> >>
>> >> >> While here I also tweaked the warning.
>> >> >
>> >> > This has been committed, thanks Ted.
>> >> >
>> >> >> Some more context:
>> >> >> - a subsequent diff would reorder the code to handle similarly the 
>> >> >> "!rv"
>> >> >>   and "ev->ident == ctl_fd" paths
>> >> >
>> >> > Diff below.
>> >> >
>> >> >> - I think we want some throttling mechanism, like wait for 1mn after we
>> >> >>   resume before autosuspending again.  But I want to fix obvious
>> >> >>   problems first.
>> >> 
>> >> On top of the previous diff, here's a diff to block autoaction for 60
>> >> seconds after:
>> >> - autoaction triggers; this prevents apmd from sending multiple suspend
>> >> requests before the system goes to sleep
>> >> - a resume happens; this gives you 60 seconds to fetch and plug your AC
>> >> cable if you notice you're low on power
>> >> 
>> >> A side effect is that apmd now ignores stale acpiac(4) data at resume
>> >> time, so it apmd doesn't suspend the system again when you resume with
>> >> a low battery and AC plugged.
>> >> 
>> >> cc'ing Scott since he has a thing for everything time-related. :)
>> >
>> > For the first case, is there any way you can detect that a suspend is
>> > in-progress but not done yet?
>> 
>> Well, apmd could record that it asked the kernel for a suspend/hibernate
>> and skip autoaction as long as it doesn't get a resume event.
>
> Hmmm.  So what happens if the suspend/hibernate fails?  Could apmd(8)
> get stuck waiting for a resume that will never happen?

Well, if suspend fails maybe there's no point in having apmd retry
a suspend. Also what would get stuck is only the autoaction behavior,
the rest of apmd would keep on working as usual.

acpi_sleep_state seems to properly send a RESUME event even if
suspend/hibernate fails, except in one error case.

But depending on a resume event is not portable, the APM code in i386
and loongson doesn't notify userland about resumes. Something that ought
to be fixed.

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



Re: apmd: fix autoaction timeout

2020-02-13 Thread Jeremie Courreges-Anglas
On Thu, Feb 13 2020, Jeremie Courreges-Anglas  wrote:

[...]

> - documents the 60 seconds grace period in the manpage

That part was not accurate.  Updated wording:

  "After a resume, the effect of those options is inhibited for 60 seconds."


Index: apmd.c
===
--- apmd.c.orig
+++ apmd.c
@@ -368,18 +368,20 @@ resumed(int ctl_fd)
 }
 
 #define TIMO (10*60)   /* 10 minutes */
+#define AUTOACTION_GRACE_PERIOD (60)   /* 1mn after resume */
 
 int
 main(int argc, char *argv[])
 {
const char *fname = _PATH_APM_CTLDEV;
int ctl_fd, sock_fd, ch, suspends, standbys, hibernates, resumes;
-   int autoaction = 0;
+   int autoaction = 0, autoaction_inflight = 0;
int autolimit = 0;
int statonly = 0;
int powerstatus = 0, powerbak = 0, powerchange = 0;
int noacsleep = 0;
struct timespec ts = {TIMO, 0}, sts = {0, 0};
+   struct timespec last_resume = { 0, 0 };
struct apm_power_info pinfo;
const char *sockname = _PATH_APM_SOCKET;
const char *errstr;
@@ -566,6 +568,8 @@ main(int argc, char *argv[])
powerstatus = powerbak;
powerchange = 1;
}
+   clock_gettime(CLOCK_MONOTONIC, _resume);
+   autoaction_inflight = 0;
resumes++;
break;
case APM_POWER_CHANGE:
@@ -577,17 +581,30 @@ main(int argc, char *argv[])
 
if (!powerstatus && autoaction &&
autolimit > (int)pinfo.battery_life) {
+   struct timespec graceperiod, now;
+
+   graceperiod = last_resume;
+   graceperiod.tv_sec += AUTOACTION_GRACE_PERIOD;
+   clock_gettime(CLOCK_MONOTONIC, );
+
logmsg(LOG_NOTICE,
"estimated battery life %d%%"
-   " below configured limit %d%%",
-   pinfo.battery_life,
-   autolimit
+   " below configured limit %d%%%s%s",
+   pinfo.battery_life, autolimit,
+   !autoaction_inflight ? "" : ", in flight",
+   timespeccmp(, , >) ?
+   "" : ", grace period"
);
 
-   if (autoaction == AUTO_SUSPEND)
-   suspends++;
-   else
-   hibernates++;
+   if (!autoaction_inflight &&
+   timespeccmp(, , >)) {
+   if (autoaction == AUTO_SUSPEND)
+   suspends++;
+   else
+   hibernates++;
+   /* Block autoaction until next resume */
+   autoaction_inflight = 1;
+   }
}
break;
default:
Index: apmd.8
===
--- apmd.8.orig
+++ apmd.8
@@ -128,6 +128,7 @@ If both
 and
 .Fl z
 are specified, the last one will supersede the other.
+After a resume, the effect of those options is inhibited for 60 seconds.
 .El
 .Pp
 When a client requests a suspend or stand-by state,


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



Re: apmd: fix autoaction timeout

2020-02-13 Thread Jeremie Courreges-Anglas
On Wed, Feb 12 2020, Scott Cheloha  wrote:
> On Wed, Feb 12, 2020 at 01:35:22PM +0100, Jeremie Courreges-Anglas wrote:
>> On Wed, Feb 12 2020, Jeremie Courreges-Anglas  wrote:
>> > On Sat, Jan 25 2020, Jeremie Courreges-Anglas  wrote:
>> >> The diff below improves the way apmd -z/-Z may trigger.
>> >>
>> >> I think the current behavior is bogus, incrementing and checking
>> >> apmtimeout like this doesn't make much sense.
>> >>
>> >> Here's a proposal:
>> >> - on APM_POWER_CHANGE events, check the battery level and trigger
>> >>   autoaction if needed.  This should be enough to make autoaction just
>> >>   work with drivers like acpibat(4).
>> >> - on kevent timeout (10mn by default now, maybe too long), also check
>> >>   the battery level and suspend if needed.  This should be useful only
>> >>   if your battery driver doesn't send any APM_POWER_CHANGE event.
>> >>
>> >> While here I also tweaked the warning.
>> >
>> > This has been committed, thanks Ted.
>> >
>> >> Some more context:
>> >> - a subsequent diff would reorder the code to handle similarly the "!rv"
>> >>   and "ev->ident == ctl_fd" paths
>> >
>> > Diff below.
>> >
>> >> - I think we want some throttling mechanism, like wait for 1mn after we
>> >>   resume before autosuspending again.  But I want to fix obvious
>> >>   problems first.
>> 
>> On top of the previous diff, here's a diff to block autoaction for 60
>> seconds after:
>> - autoaction triggers; this prevents apmd from sending multiple suspend
>> requests before the system goes to sleep
>> - a resume happens; this gives you 60 seconds to fetch and plug your AC
>> cable if you notice you're low on power
>> 
>> A side effect is that apmd now ignores stale acpiac(4) data at resume
>> time, so it apmd doesn't suspend the system again when you resume with
>> a low battery and AC plugged.
>> 
>> cc'ing Scott since he has a thing for everything time-related. :)
>
> For the first case, is there any way you can detect that a suspend is
> in-progress but not done yet?

Well, apmd could record that it asked the kernel for a suspend/hibernate
and skip autoaction as long as it doesn't get a resume event.

> I think that'd be cleaner (in some ways) than an autoaction cooldown
> timer.
>
> Whenever I want to add an arbitrary delay that isn't per se required
> by an interface I wonder whether I'm working around a deficiency in
> the state machine instead of addressing the root cause.
>
> Sometimes it can't be helped, but I have to ask.

Initially I only cared about the second case, and then noticed that
APM_POWER_CHANGE events can happen at any time.  Reusing the 60 seconds
timer looked appealing (cheap) but please see the updated diff below.

> For the second case, I thought the design of autoaction was to (a)
> note that the battery was below a particular threshold and (b) take
> action to avert data loss.  If you resume from suspend with battery
> below the threshold and no AC I think you would *want* autoaction to
> trigger.  Like, it sounds like the state machine is working as
> designed.
>
> If the machine is immediately suspending after resume shouldn't you
> just plug it in before reattempting resume?  Isn't that better than
> having the battery die on you?

We can't know when the battery will fail to feed the system.  I suspect
that the resume sequence itself may drain more power than 60 seconds
spent idling (wild guess, no power meter at hand).  So I see no
convincing reason to prevent any use of the system.

Regarding the user experience, I think the user should be put into
control.  60 seconds is enough to plug the power cable or take a quick
look at a document, or even kill apmd if the laptop is really needed
like, *right now*.  I know I've been in this kind of situation several
times.

So here's an updated diff that:
- disables autoaction for 60 seconds after resume.  This is still done
  in a naive way, autoaction won't kick in exactly after 60 seconds
  after resume.  Good enough for now, I think.
- prevents autoaction to kick in several times before suspend/hibernate
- improves naming (suggestions welcome)
- logs why autoaction doesn't kick in
- documents the 60 seconds grace period in the manpage

This still works around the issue with stale acpiac(4) data at resume
time.

Thanks for your input so far, I hope I have addressed your concerns.

Comments / oks?


Index: apmd.c
===
--- apmd.c.orig
+++ apmd.c
@@ -368,18 +368,20 @@ resumed(int ctl_fd)
 

Re: apmd: fix autoaction timeout

2020-02-12 Thread Jeremie Courreges-Anglas
On Wed, Feb 12 2020, Jeremie Courreges-Anglas  wrote:
> On Sat, Jan 25 2020, Jeremie Courreges-Anglas  wrote:
>> The diff below improves the way apmd -z/-Z may trigger.
>>
>> I think the current behavior is bogus, incrementing and checking
>> apmtimeout like this doesn't make much sense.
>>
>> Here's a proposal:
>> - on APM_POWER_CHANGE events, check the battery level and trigger
>>   autoaction if needed.  This should be enough to make autoaction just
>>   work with drivers like acpibat(4).
>> - on kevent timeout (10mn by default now, maybe too long), also check
>>   the battery level and suspend if needed.  This should be useful only
>>   if your battery driver doesn't send any APM_POWER_CHANGE event.
>>
>> While here I also tweaked the warning.
>
> This has been committed, thanks Ted.
>
>> Some more context:
>> - a subsequent diff would reorder the code to handle similarly the "!rv"
>>   and "ev->ident == ctl_fd" paths
>
> Diff below.
>
>> - I think we want some throttling mechanism, like wait for 1mn after we
>>   resume before autosuspending again.  But I want to fix obvious
>>   problems first.

On top of the previous diff, here's a diff to block autoaction for 60
seconds after:
- autoaction triggers; this prevents apmd from sending multiple suspend
requests before the system goes to sleep
- a resume happens; this gives you 60 seconds to fetch and plug your AC
cable if you notice you're low on power

A side effect is that apmd now ignores stale acpiac(4) data at resume
time, so it apmd doesn't suspend the system again when you resume with
a low battery and AC plugged.

cc'ing Scott since he has a thing for everything time-related. :)

ok?



Index: apmd.c
===
--- apmd.c.orig
+++ apmd.c
@@ -380,6 +380,7 @@ main(int argc, char *argv[])
int powerstatus = 0, powerbak = 0, powerchange = 0;
int noacsleep = 0;
struct timespec ts = {TIMO, 0}, sts = {0, 0};
+   struct timespec autoaction_timestamp = { 0, 0 };
struct apm_power_info pinfo;
const char *sockname = _PATH_APM_SOCKET;
const char *errstr;
@@ -566,6 +567,7 @@ main(int argc, char *argv[])
powerstatus = powerbak;
powerchange = 1;
}
+   clock_gettime(CLOCK_MONOTONIC, _timestamp);
resumes++;
break;
case APM_POWER_CHANGE:
@@ -577,6 +579,8 @@ main(int argc, char *argv[])
 
if (!powerstatus && autoaction &&
autolimit > (int)pinfo.battery_life) {
+   struct timespec delay, now;
+
logmsg(LOG_NOTICE,
"estimated battery life %d%%"
" below configured limit %d%%",
@@ -584,10 +588,17 @@ main(int argc, char *argv[])
autolimit
);
 
-   if (autoaction == AUTO_SUSPEND)
-   suspends++;
-   else
-   hibernates++;
+   delay = autoaction_timestamp;
+   delay.tv_sec += 60;
+   clock_gettime(CLOCK_MONOTONIC, );
+   if (timespeccmp(, , >)) {
+   if (autoaction == AUTO_SUSPEND)
+   suspends++;
+   else
+   hibernates++;
+   clock_gettime(CLOCK_MONOTONIC,
+   _timestamp);
+   }
}
break;
default:

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



Re: apmd: fix autoaction timeout

2020-02-12 Thread Jeremie Courreges-Anglas
On Sat, Jan 25 2020, Jeremie Courreges-Anglas  wrote:
> The diff below improves the way apmd -z/-Z may trigger.
>
> I think the current behavior is bogus, incrementing and checking
> apmtimeout like this doesn't make much sense.
>
> Here's a proposal:
> - on APM_POWER_CHANGE events, check the battery level and trigger
>   autoaction if needed.  This should be enough to make autoaction just
>   work with drivers like acpibat(4).
> - on kevent timeout (10mn by default now, maybe too long), also check
>   the battery level and suspend if needed.  This should be useful only
>   if your battery driver doesn't send any APM_POWER_CHANGE event.
>
> While here I also tweaked the warning.

This has been committed, thanks Ted.

> Some more context:
> - a subsequent diff would reorder the code to handle similarly the "!rv"
>   and "ev->ident == ctl_fd" paths

Diff below.

> - I think we want some throttling mechanism, like wait for 1mn after we
>   resume before autosuspending again.  But I want to fix obvious
>   problems first.
[...]

Further unify the handling of kevent(2) timeouts and apm events:
fake an APM_POWER_CHANGE event on timeouts.

I think assert(3) is appropriate here but could be convinced otherwise.

ok?


Index: apmd.c
===
--- apmd.c.orig
+++ apmd.c
@@ -37,6 +37,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -497,7 +498,7 @@ main(int argc, char *argv[])
error("kevent", NULL);
 
for (;;) {
-   int rv;
+   int rv, event, index;
 
sts = ts;
 
@@ -528,6 +529,46 @@ main(int argc, char *argv[])
continue;
} else if (rv == 0) {
/* wakeup for timeout: take status */
+   event = APM_POWER_CHANGE;
+   index = -1;
+   } else {
+   assert(rv == 1 && ev->ident == ctl_fd);
+   event = APM_EVENT_TYPE(ev->data);
+   index = APM_EVENT_INDEX(ev->data);
+   }
+
+   logmsg(LOG_DEBUG, "apmevent %04x index %d", event, index);
+
+   switch (event) {
+   case APM_SUSPEND_REQ:
+   case APM_USER_SUSPEND_REQ:
+   case APM_CRIT_SUSPEND_REQ:
+   case APM_BATTERY_LOW:
+   suspends++;
+   break;
+   case APM_USER_STANDBY_REQ:
+   case APM_STANDBY_REQ:
+   standbys++;
+   break;
+   case APM_USER_HIBERNATE_REQ:
+   hibernates++;
+   break;
+#if 0
+   case APM_CANCEL:
+   suspends = standbys = 0;
+   break;
+#endif
+   case APM_NORMAL_RESUME:
+   case APM_CRIT_RESUME:
+   case APM_SYS_STANDBY_RESUME:
+   powerbak = power_status(ctl_fd, 0, );
+   if (powerstatus != powerbak) {
+   powerstatus = powerbak;
+   powerchange = 1;
+   }
+   resumes++;
+   break;
+   case APM_POWER_CHANGE:
powerbak = power_status(ctl_fd, 0, );
if (powerstatus != powerbak) {
powerstatus = powerbak;
@@ -548,63 +589,9 @@ main(int argc, char *argv[])
else
hibernates++;
}
-   } else if (ev->ident == ctl_fd) {
-   logmsg(LOG_DEBUG, "apmevent %04x index %d",
-   (int)APM_EVENT_TYPE(ev->data),
-   (int)APM_EVENT_INDEX(ev->data));
-
-   switch (APM_EVENT_TYPE(ev->data)) {
-   case APM_SUSPEND_REQ:
-   case APM_USER_SUSPEND_REQ:
-   case APM_CRIT_SUSPEND_REQ:
-   case APM_BATTERY_LOW:
-   suspends++;
-   break;
-   case APM_USER_STANDBY_REQ:
-   case APM_STANDBY_REQ:
-   standbys++;
-   break;
-   case APM_USER_HIBERNATE_REQ:
-   hibernates++;
-   break;
-#if 0
-   case APM_CANCEL:
-   suspends = standbys = 0;
-   break;
-#endif
-   case APM_NORMAL_RESUME:
-   case APM_CRIT_RESUME:
-   case

Re: sys/cdefs.h: fix __predict_false fallback implementation

2020-02-11 Thread Jeremie Courreges-Anglas
On Tue, Feb 11 2020, "Todd C. Miller"  wrote:
> On Tue, 11 Feb 2020 21:44:21 +, Nicholas Marriott wrote:
>
>> Looks like the existing code is OK, you still want to test the original
>> expression even if you are predicting it is false, no?
>
> Right, the code is correct as written.

Woops, brain fart on my side, thanks.

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



sys/cdefs.h: fix __predict_false fallback implementation

2020-02-11 Thread Jeremie Courreges-Anglas


Found while looking at __ISO_C_VISIBLE.  I'm not sure which compilers
would be affected.  The fallback could simply be
 #define __predict_true(exp)(exp)
 #define __predict_false(exp)   (exp)
but I settled for a minimal change.

ok?


Index: cdefs.h
===
RCS file: /d/cvs/src/sys/sys/cdefs.h,v
retrieving revision 1.43
diff -u -p -p -u -r1.43 cdefs.h
--- cdefs.h 29 Oct 2018 17:10:40 -  1.43
+++ cdefs.h 11 Feb 2020 18:27:18 -
@@ -193,7 +193,7 @@
 #define __predict_false(exp)   __builtin_expect(((exp) != 0), 0)
 #else
 #define __predict_true(exp)((exp) != 0)
-#define __predict_false(exp)   ((exp) != 0)
+#define __predict_false(exp)   ((exp) == 0)
 #endif
 
 /* Delete pseudo-keywords wherever they are not available or needed. */


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



Re: macppc base-clang -msvr4-struct-return

2020-02-11 Thread Jeremie Courreges-Anglas
On Tue, Feb 11 2020, George Koehler  wrote:
> On Tue, 11 Feb 2020 15:20:00 +0100
> Jeremie Courreges-Anglas  wrote:
>
>> fwiw I'm already ok with the diff George sent for ports/devel/llvm.
>> I'm mostly ok with this one but it would be nice to know whether
>> base-clang can rebuild itself.  :)
>
> base-clang can't rebuild itself in the normal way.  I have been
> exchanging mails with Todd Mortimer, who has been testing my diff with
> a faster macppc machine.  The diff changes the ABI between
> /usr/bin/clang and /usr/lib/libc++.so.3.0, so when we install a new
> libc++ built by clang -msvr4-struct-return, but still have a clang
> built as if by -maix-struct-return, then clang crashes and can't
> rebuild itself!  It might be possible to use a static-link clang to
> cross the ABI change.
>
> A backtrace from clang pointed to a function in libc++ that returns a
> std::string::iterator, a small struct where sizeof(iterator) == 4.

So the steps would be:
- build and install a new clang
- bump the major of libc++, build and install it
- rebuild and reinstall clang
- build new snap

Assuming those steps are correct, you could ask Theo to handle the ABI
break.  A note in current.html could be useful too.

> This was not a problem with ports-clang, because we use ports-gcc to
> build ports-clang; and ports-clang uses libestdc++ (from ports-gcc),
> not libc++ (from base).  Both ports-clang and libestdc++ got built by
> gcc -msvr4-struct-return.  --George

Yep.

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



sys/cdefs.h: visibility for old C++ standards

2020-02-11 Thread Jeremie Courreges-Anglas


IIUC we should limit the visibility of C features to ANSI C (instead of
C11) for old C++ standards.

I guess this is low priority so I didn't do much testing, but I can do
a base+xenocara build on amd64 and sparc64 if deemed useful.

Thoughts?


Index: cdefs.h
===
RCS file: /d/cvs/src/sys/sys/cdefs.h,v
retrieving revision 1.43
diff -u -p -p -u -r1.43 cdefs.h
--- cdefs.h 29 Oct 2018 17:10:40 -  1.43
+++ cdefs.h 30 Jan 2020 19:05:23 -
@@ -382,6 +382,9 @@
 (defined(__cplusplus) && __cplusplus >= 201103)
 # undef __ISO_C_VISIBLE
 # define __ISO_C_VISIBLE   1999
+#elif (defined(__cplusplus) && __cplusplus < 201103)
+#undef __ISO_C_VISIBLE
+#define __ISO_C_VISIBLE1990
 #endif
 
 /*

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



Re: macppc base-clang -msvr4-struct-return

2020-02-11 Thread Jeremie Courreges-Anglas
On Wed, Feb 05 2020, Todd Mortimer  wrote:
> Hi George,
>
> On Tue, Feb 04, 2020 at 08:39:12PM -0500, George Koehler wrote:
>> Hello tech list,
>> 
>> This is a diff for base-clang.  It would change the powerpc target to
>> return small structs in registers r3 and r4.  This would fix an
>> incompatibility with gcc in OpenBSD macppc.  I fear that if I commit
>> the diff, I might break snapshot builds.  I am looking for help.
>
> The diff looks sane to me. I don't see anything that would obviously
> break snaps, and the option is limited to only the ppc platform.
>
> I am running it through a release build here (amd64) and will know how
> that turned out tomorrow after work, so will be able to confirm that you
> won't break snaps on that arch. I don't expect there to be any problem
> there. I should be able to test it on macppc over the weekend, so if
> you're paranoid you can wait to see how that goes.

fwiw I'm already ok with the diff George sent for ports/devel/llvm.
I'm mostly ok with this one but it would be nice to know whether
base-clang can rebuild itself.  :)

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



Re: remove needless #ifdef

2020-02-09 Thread Jeremie Courreges-Anglas
On Sun, Feb 09 2020, Jan Stary  wrote:
> Currently, sys/net/pipex_local.h asks #ifdef __OpenBSD__
> and if so, defines "Static" to be nothing, to use it later.
> That can go away, right?

I believe that's something the IIJ folks want to keep, cc'ing Yasuoka.

>   Jan
>
>
> Index: sys/net/pipex_local.h
> ===
> RCS file: /cvs/src/sys/net/pipex_local.h,v
> retrieving revision 1.30
> diff -u -p -r1.30 pipex_local.h
> --- sys/net/pipex_local.h 31 Jan 2019 18:01:14 -  1.30
> +++ sys/net/pipex_local.h 9 Feb 2020 15:26:51 -
> @@ -26,12 +26,6 @@
>   * SUCH DAMAGE.
>   */
>  
> -#ifdef __OpenBSD__
> -#define Static
> -#else
> -#define Static static
> -#endif
> -
>  #define  PIPEX_PPTP  1
>  #define  PIPEX_L2TP  1
>  #define  PIPEX_PPPOE 1
> @@ -372,59 +366,56 @@ extern struct pipex_hash_head   pipex_id_h
>  #define PIPEX_TCP_OPTLEN 40
>  #define  PIPEX_L2TP_MINLEN   8
>  
> -/*
> - * static function prototypes
> - */
> -Static void  pipex_iface_start (struct pipex_iface_context 
> *);
> -Static void  pipex_iface_stop (struct pipex_iface_context *);
> -Static int   pipex_add_session (struct pipex_session_req *, 
> struct pipex_iface_context *);
> -Static int   pipex_close_session (struct 
> pipex_session_close_req *);
> -Static int   pipex_config_session (struct 
> pipex_session_config_req *);
> -Static int   pipex_get_stat (struct pipex_session_stat_req 
> *);
> -Static int   pipex_get_closed (struct pipex_session_list_req 
> *);
> -Static int   pipex_destroy_session (struct pipex_session *);
> -Static struct pipex_session  *pipex_lookup_by_ip_address (struct in_addr);
> -Static struct pipex_session  *pipex_lookup_by_session_id (int, int);
> -Static void  pipex_ip_output (struct mbuf *, struct 
> pipex_session *);
> -Static void  pipex_ppp_output (struct mbuf *, struct 
> pipex_session *, int);
> -Static int   pipex_ppp_proto (struct mbuf *, struct 
> pipex_session *, int, int *);
> -Static void  pipex_ppp_input (struct mbuf *, struct 
> pipex_session *, int);
> -Static void  pipex_ip_input (struct mbuf *, struct 
> pipex_session *);
> +void  pipex_iface_start (struct pipex_iface_context *);
> +void  pipex_iface_stop (struct pipex_iface_context *);
> +int   pipex_add_session (struct pipex_session_req *, struct 
> pipex_iface_context *);
> +int   pipex_close_session (struct pipex_session_close_req *);
> +int   pipex_config_session (struct pipex_session_config_req 
> *);
> +int   pipex_get_stat (struct pipex_session_stat_req *);
> +int   pipex_get_closed (struct pipex_session_list_req *);
> +int   pipex_destroy_session (struct pipex_session *);
> +struct pipex_session  *pipex_lookup_by_ip_address (struct in_addr);
> +struct pipex_session  *pipex_lookup_by_session_id (int, int);
> +void  pipex_ip_output (struct mbuf *, struct pipex_session 
> *);
> +void  pipex_ppp_output (struct mbuf *, struct pipex_session 
> *, int);
> +int   pipex_ppp_proto (struct mbuf *, struct pipex_session 
> *, int, int *);
> +void  pipex_ppp_input (struct mbuf *, struct pipex_session 
> *, int);
> +void  pipex_ip_input (struct mbuf *, struct pipex_session *);
>  #ifdef INET6
> -Static void  pipex_ip6_input (struct mbuf *, struct 
> pipex_session *);
> +void  pipex_ip6_input (struct mbuf *, struct pipex_session 
> *);
>  #endif
> -Static struct mbuf   *pipex_common_input(struct pipex_session *, 
> struct mbuf *, int, int, int);
> +struct mbuf   *pipex_common_input(struct pipex_session *, struct 
> mbuf *, int, int, int);
>  
>  #ifdef PIPEX_PPPOE
> -Static void  pipex_pppoe_output (struct mbuf *, struct 
> pipex_session *);
> +void  pipex_pppoe_output (struct mbuf *, struct 
> pipex_session *);
>  #endif
>  
>  #ifdef PIPEX_PPTP
> -Static void  pipex_pptp_output (struct mbuf *, struct 
> pipex_session *, int, int);
> -Static struct pipex_session  *pipex_pptp_userland_lookup_session(struct mbuf 
> *, struct sockaddr *);
> +void  pipex_pptp_output (struct mbuf *, struct pipex_session 
> *, int, int);
> +struct pipex_session  *pipex_pptp_userland_lookup_session(struct mbuf *, 
> struct sockaddr *);
>  #endif
>  
>  #ifdef PIPEX_L2TP
> -Static void  pipex_l2tp_output (struct mbuf *, struct 
> pipex_session *);
> +void  pipex_l2tp_output (struct mbuf *, struct pipex_session 
> *);
>  #endif
>  
>  #ifdef PIPEX_MPPE
> -Static void  pipex_mppe_init (struct pipex_mppe 

Re: acpiac(4): refresh status after resume (looking for tests)

2020-02-02 Thread Jeremie Courreges-Anglas
On Sat, Jan 25 2020, Jeremie Courreges-Anglas  wrote:
> So I have this diff for apmd -z/-Z that uses APM_POWER_CHANGE events to
> trigger autosuspend.  It works fine except for one glitch: if I plug the
> AC cable and then resume, apmd will receive another APM_POWER_CHANGE
> event and read the power info only to find that the AC is *unplugged*.
> So apmd happily suspends my system again.  If I resume once more, the AC
> status is correctly detected and apmd(8) doesn't suspend the system again.
>
> What happens is that acpibat_notify runs because of ACPIDEV_POLL, it
> grabs battery data and sends an APM_POWER_CHANGE event to apmd.  At this
> point apmd may see out of date data from acpiac(4).
>
> acpi_sleep_task later forces a refresh of (in order) acpiac(4),
> acpibat(4) and acpisbs(4), but in my use case it's too late, the window
> with stale acpiac(4) data is too long.
>
> The diff below refreshes AC status with notifications at DVACT_WAKEUP
> time.  Is is correct that DVACT_WAKEUP handlers will always run before
> the ACPI task queue is processed?  "No" seems unlikely but I'd rather
> ask...
>
> I hear we prefer running ACPI code in its dedicated thread but there are
> other drivers in dev/acpi that run ACPI code in DVACT_WAKEUP, so I'm
> assuming it is fine. :)
>
> There are probably many ways to tackle this problem, from having
> userland ignore stale data after a resume to tweaking acpibat(4) to
> stop/restart/skip the ACPIDEV_POLL loop, but all of them look
> hackish/undoable so far.
>
> Thoughts?  oks?

Still looking for feedback regarding the approach used, and also for
test reports.

Diff included again for convenience,


Index: acpiac.c
===
RCS file: /d/cvs/src/sys/dev/acpi/acpiac.c,v
retrieving revision 1.31
diff -u -p -r1.31 acpiac.c
--- acpiac.c1 Jul 2018 19:40:49 -   1.31
+++ acpiac.c21 Jan 2020 01:43:15 -
@@ -33,13 +33,18 @@
 
 int  acpiac_match(struct device *, void *, void *);
 void acpiac_attach(struct device *, struct device *, void *);
+int  acpiac_activate(struct device *, int);
 int  acpiac_notify(struct aml_node *, int, void *);
 
 void acpiac_refresh(void *);
 int acpiac_getsta(struct acpiac_softc *);
 
 struct cfattach acpiac_ca = {
-   sizeof(struct acpiac_softc), acpiac_match, acpiac_attach
+   sizeof(struct acpiac_softc),
+   acpiac_match,
+   acpiac_attach,
+   NULL,
+   acpiac_activate,
 };
 
 struct cfdriver acpiac_cd = {
@@ -92,6 +97,21 @@ acpiac_attach(struct device *parent, str
acpiac_notify, sc, ACPIDEV_NOPOLL);
 }
 
+int
+acpiac_activate(struct device *self, int act)
+{
+   struct acpiac_softc *sc = (struct acpiac_softc *)self;
+
+   switch (act) {
+   case DVACT_WAKEUP:
+   acpiac_refresh(sc);
+   dnprintf(10, "A/C status: %d\n", sc->sc_ac_stat);
+   break;
+   }
+
+   return (0);
+}
+
 void
 acpiac_refresh(void *arg)
 {
@@ -99,7 +119,6 @@ acpiac_refresh(void *arg)
 
acpiac_getsta(sc);
sc->sc_sens[0].value = sc->sc_ac_stat;
-   acpi_record_event(sc->sc_acpi, APM_POWER_CHANGE);
 }
 
 int
@@ -140,6 +159,7 @@ acpiac_notify(struct aml_node *node, int
/* FALLTHROUGH */
case 0x80:
acpiac_refresh(sc);
+   acpi_record_event(sc->sc_acpi, APM_POWER_CHANGE);
dnprintf(10, "A/C status: %d\n", sc->sc_ac_stat);
break;
}


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



Re: acme-client: prevent duplicate definitions of global variables

2020-02-01 Thread Jeremie Courreges-Anglas
On Fri, Jan 31 2020, Michael Forney  wrote:
> Every source file that includes extern.h will have its own definition
> of these variables. Since many compilers allocate the variables with
> .comm, they end up getting merged by the linker without error.
> However, ISO C requires exactly one definition of objects with
> external linkage.

LGTM, ok jca@.

I'll commit it if none of the usual suspects show up soon.

> gcc 10 will enable -fno-common by default, which will put
> zero-initialized data in .bss, causing linking errors when multiple
> definitions are present.

Good to know, thanks.

> ---
>  usr.sbin/acme-client/extern.h | 4 ++--
>  usr.sbin/acme-client/main.c   | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/usr.sbin/acme-client/extern.h b/usr.sbin/acme-client/extern.h
> index e6b7af0d05b..f280b3e279e 100644
> --- a/usr.sbin/acme-client/extern.h
> +++ b/usr.sbin/acme-client/extern.h
> @@ -277,12 +277,12 @@ char*json_fmt_signed(const char *, const 
> char *, const char *);
>  /*
>   * Should we print debugging messages?
>   */
> -int   verbose;
> +extern intverbose;
>  
>  /*
>   * What component is the process within (COMP__MAX for none)?
>   */
> -enum comp proccomp;
> +extern enum comp proccomp;
>  
>  __END_DECLS
>  
> diff --git a/usr.sbin/acme-client/main.c b/usr.sbin/acme-client/main.c
> index 7cbeeb7de03..1f59e6c755d 100644
> --- a/usr.sbin/acme-client/main.c
> +++ b/usr.sbin/acme-client/main.c
> @@ -32,6 +32,9 @@
>  #define WWW_DIR "/var/www/acme"
>  #define CONF_FILE "/etc/acme-client.conf"
>  
> +int   verbose;
> +enum comp proccomp;
> +
>  int
>  main(int argc, char *argv[])
>  {
> @@ -46,8 +49,6 @@ main(int argc, char *argv[])
>   int   c, rc, revocate = 0;
>   int   popts = 0;
>   pid_t pids[COMP__MAX];
> - extern intverbose;
> - extern enum comp  proccomp;
>   size_ti, altsz, ne;
>  
>   struct acme_conf*conf = NULL;

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



Re: rdomain.4: Use -rdomain to delete rdomain

2020-02-01 Thread Jeremie Courreges-Anglas
On Sat, Feb 01 2020, Klemens Nanni  wrote:
> Small nit but this properly reflects the "delete" semantic, much better
> than the implicit "reassign".
>
> ifconfig(8) also nicely mentions IPv4 and IPv6 addresses being purged
> in the `-rdomain' part, should the reader consult this manual afterwards.
>
> OK?

ok jca@

>
> Index: rdomain.4
> ===
> RCS file: /cvs/src/share/man/man4/rdomain.4,v
> retrieving revision 1.12
> diff -u -p -r1.12 rdomain.4
> --- rdomain.4 9 Sep 2018 10:13:21 -   1.12
> +++ rdomain.4 1 Feb 2020 00:13:17 -
> @@ -125,7 +125,7 @@ match out on rdomain 4 to !$internal_net
>  .Pp
>  Delete rdomain 4 again:
>  .Bd -literal -offset indent
> -# ifconfig em0 rdomain 0
> +# ifconfig em0 -rdomain
>  # ifconfig lo4 destroy
>  .Ed
>  .Sh SEE ALSO
>

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



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

2020-02-01 Thread Jeremie Courreges-Anglas
On Fri, Jan 31 2020, Ingo Schwarze  wrote:
> Hi,
>
> ngc...@gmail.com wrote on Fri, Jan 31, 2020 at 10:14:52PM +0900:
>
>> Reduce scope of a few variables.
>
> No, this contradicts OpenBSD coding style.
> We want local variables declared up front in functions
> such that you can see at one glance which variables exist.

Huh, this is your personal preference and I strongly disagree with
making it an "official" stance (again).  Reducing the scope of variables
*quite often* helps reasoning about them.

style(9) said something like that, and has been changed three years
ago (for good IMO).

  revision 1.68
  date: 2016/10/18 18:13:56;  author: millert;  state: Exp;  lines: +2 -4;  
commitid: aPPHgmmA4hseZUFx;
  Don't tell the programmer not to put variable declarations inside
  blocks.  OK guenther@ kettenis@

This being said, the patch doesn't apply and the proposed change doesn't
improve the current code much, so this feels like needless churn.
"ngc891", please find another itch to scratch. :)

>> While here, remove an extraneous space.
>
> While avoiding trailing whitespace is good, it's not worth
> a commit (nor sending patches around).

Seconded.

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



acpiac(4): refresh status after resume

2020-01-25 Thread Jeremie Courreges-Anglas


So I have this diff for apmd -z/-Z that uses APM_POWER_CHANGE events to
trigger autosuspend.  It works fine except for one glitch: if I plug the
AC cable and then resume, apmd will receive another APM_POWER_CHANGE
event and read the power info only to find that the AC is *unplugged*.
So apmd happily suspends my system again.  If I resume once more, the AC
status is correctly detected and apmd(8) doesn't suspend the system again.

What happens is that acpibat_notify runs because of ACPIDEV_POLL, it
grabs battery data and sends an APM_POWER_CHANGE event to apmd.  At this
point apmd may see out of date data from acpiac(4).

acpi_sleep_task later forces a refresh of (in order) acpiac(4),
acpibat(4) and acpisbs(4), but in my use case it's too late, the window
with stale acpiac(4) data is too long.

The diff below refreshes AC status with notifications at DVACT_WAKEUP
time.  Is is correct that DVACT_WAKEUP handlers will always run before
the ACPI task queue is processed?  "No" seems unlikely but I'd rather
ask...

I hear we prefer running ACPI code in its dedicated thread but there are
other drivers in dev/acpi that run ACPI code in DVACT_WAKEUP, so I'm
assuming it is fine. :)

There are probably many ways to tackle this problem, from having
userland ignore stale data after a resume to tweaking acpibat(4) to
stop/restart/skip the ACPIDEV_POLL loop, but all of them look
hackish/undoable so far.

Thoughts?  oks?


Index: acpiac.c
===
RCS file: /d/cvs/src/sys/dev/acpi/acpiac.c,v
retrieving revision 1.31
diff -u -p -p -u -r1.31 acpiac.c
--- acpiac.c1 Jul 2018 19:40:49 -   1.31
+++ acpiac.c25 Jan 2020 18:33:15 -
@@ -33,13 +33,18 @@
 
 int  acpiac_match(struct device *, void *, void *);
 void acpiac_attach(struct device *, struct device *, void *);
+int  acpiac_activate(struct device *, int);
 int  acpiac_notify(struct aml_node *, int, void *);
 
 void acpiac_refresh(void *);
 int acpiac_getsta(struct acpiac_softc *);
 
 struct cfattach acpiac_ca = {
-   sizeof(struct acpiac_softc), acpiac_match, acpiac_attach
+   sizeof(struct acpiac_softc),
+   acpiac_match,
+   acpiac_attach,
+   NULL,
+   acpiac_activate,
 };
 
 struct cfdriver acpiac_cd = {
@@ -92,6 +97,21 @@ acpiac_attach(struct device *parent, str
acpiac_notify, sc, ACPIDEV_NOPOLL);
 }
 
+int
+acpiac_activate(struct device *self, int act)
+{
+   struct acpiac_softc *sc = (struct acpiac_softc *)self;
+
+   switch (act) {
+   case DVACT_WAKEUP:
+   acpiac_refresh(sc);
+   dnprintf(10, "A/C status: %d\n", sc->sc_ac_stat);
+   break;
+   }
+
+   return (0);
+}
+
 void
 acpiac_refresh(void *arg)
 {
@@ -99,7 +119,6 @@ acpiac_refresh(void *arg)
 
acpiac_getsta(sc);
sc->sc_sens[0].value = sc->sc_ac_stat;
-   acpi_record_event(sc->sc_acpi, APM_POWER_CHANGE);
 }
 
 int
@@ -140,6 +159,7 @@ acpiac_notify(struct aml_node *node, int
/* FALLTHROUGH */
case 0x80:
acpiac_refresh(sc);
+   acpi_record_event(sc->sc_acpi, APM_POWER_CHANGE);
dnprintf(10, "A/C status: %d\n", sc->sc_ac_stat);
break;
}


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



apmd: fix autoaction timeout

2020-01-25 Thread Jeremie Courreges-Anglas


The diff below improves the way apmd -z/-Z may trigger.

I think the current behavior is bogus, incrementing and checking
apmtimeout like this doesn't make much sense.

Here's a proposal:
- on APM_POWER_CHANGE events, check the battery level and trigger
  autoaction if needed.  This should be enough to make autoaction just
  work with drivers like acpibat(4).
- on kevent timeout (10mn by default now, maybe too long), also check
  the battery level and suspend if needed.  This should be useful only
  if your battery driver doesn't send any APM_POWER_CHANGE event.

While here I also tweaked the warning.

Some more context:
- a subsequent diff would reorder the code to handle similarly the "!rv"
  and "ev->ident == ctl_fd" paths
- I think we want some throttling mechanism, like wait for 1mn after we
  resume before autosuspending again.  But I want to fix obvious
  problems first.
- if battery is still lower than your autoaction limit, your system
  might go back to sleep once if you attempt a resume after plugging
  your AC cable.  Another glitch, another mail to tech@.

Thoughts?  oks?


Index: apmd.c
===
RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.91
diff -u -p -r1.91 apmd.c
--- apmd.c  2 Nov 2019 00:41:36 -   1.91
+++ apmd.c  25 Jan 2020 18:05:08 -
@@ -380,7 +380,6 @@ main(int argc, char *argv[])
int noacsleep = 0;
struct timespec ts = {TIMO, 0}, sts = {0, 0};
struct apm_power_info pinfo;
-   time_t apmtimeout = 0;
const char *sockname = _PATH_APM_SOCKET;
const char *errstr;
int kq, nchanges;
@@ -502,13 +501,10 @@ main(int argc, char *argv[])
 
sts = ts;
 
-   apmtimeout += 1;
if ((rv = kevent(kq, NULL, 0, ev, 1, )) == -1)
break;
 
-   if (apmtimeout >= ts.tv_sec) {
-   apmtimeout = 0;
-
+   if (!rv) {
/* wakeup for timeout: take status */
powerbak = power_status(ctl_fd, 0, );
if (powerstatus != powerbak) {
@@ -519,8 +515,8 @@ main(int argc, char *argv[])
if (!powerstatus && autoaction &&
autolimit > (int)pinfo.battery_life) {
logmsg(LOG_NOTICE,
-   "estimated battery life %d%%, "
-   "autoaction limit set to %d%% .",
+   "estimated battery life %d%%"
+   " below configured limit %d%%",
pinfo.battery_life,
autolimit
);
@@ -530,10 +526,8 @@ main(int argc, char *argv[])
else
hibernate(ctl_fd);
}
-   }
-
-   if (!rv)
continue;
+   }
 
if (ev->ident == ctl_fd) {
suspends = standbys = hibernates = resumes = 0;
@@ -575,6 +569,19 @@ main(int argc, char *argv[])
if (powerstatus != powerbak) {
powerstatus = powerbak;
powerchange = 1;
+   }
+
+   if (!powerstatus && autoaction &&
+   autolimit > (int)pinfo.battery_life) {
+   logmsg(LOG_NOTICE,
+   "estimated battery life %d%%"
+   " below configured limit %d%%",
+   pinfo.battery_life, autolimit);
+
+   if (autoaction == AUTO_SUSPEND)
+   suspends++;
+   else
+   hibernates++;
}
break;
default:


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



acpisbs(4): send APM events

2020-01-25 Thread Jeremie Courreges-Anglas


acpisbs(4) should send events to userland (apmd) whenever it refreshes
its data, just like acpibat(4).

I have no hw to test this, I'd welcome a runtime check with apmd -d.
ok?


Index: acpisbs.c
===
RCS file: /d/cvs/src/sys/dev/acpi/acpisbs.c,v
retrieving revision 1.8
diff -u -p -p -u -r1.8 acpisbs.c
--- acpisbs.c   9 May 2019 18:29:25 -   1.8
+++ acpisbs.c   25 Jan 2020 17:28:10 -
@@ -28,6 +28,8 @@
 #include 
 #include 
 
+#include 
+
 #include 
 #include 
 #include 
@@ -377,6 +379,7 @@ acpisbs_notify(struct aml_node *node, in
if (diff.tv_sec > ACPISBS_POLL_FREQ) {
acpisbs_read(sc);
acpisbs_refresh_sensors(sc);
+   acpi_record_event(sc->sc_acpi, APM_POWER_CHANGE);
getmicrouptime(>sc_lastpoll);
}
break;

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



Re: apmd poll every minute

2020-01-25 Thread Jeremie Courreges-Anglas
On Fri, Jan 24 2020, "Ted Unangst"  wrote:
> Sometimes (particuarly if you are using apmd -z) the default polling interval
> of 10 minutes is a bit lazy and we fail to respond to low battery situations
> before they become no battery situations.
>
> Perhaps something smarter could be done, but the simplest change is to reduce
> the default polling interval to one minute. This is still rather slow, so as
> to not introduce unnecessary load on the system, but should make it more
> responsive to changing conditions.

Hah, I'm poking at this since yesterday.

Your change is only useful if you have a battery driver that doesn't
notify you often enough.  acpibat(4) uses ACPIDEV_POLL and sends apmd
a message every 10 seconds.  That's way more than enough for apmd to
act.

If you use acpisbs(4) or another battery driver that doesn't send any
event to userland, you might find the apmd poll timeout a tad long.  But
I'm not sure this is the way to solve it.

apmd -z/-Z is mostly broken if you don't use -t and a very short poll
timeout, but this can easily be improved.  Diffs are coming.

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



ftp(1): setjmp, volatile and errno fixes

2020-01-15 Thread Jeremie Courreges-Anglas


Some of those tweaks were mentioned first in

  https://marc.info/?l=openbsd-tech=157672366409171=2

* Fix setjmp vs volatile usage:
- "buf" shouldn't be modified after setjmp, else we break the setjmp
contract:
--8<--
 All accessible objects have values as of the time the longjmp() routine
 was called, except that the values of objects of automatic storage
 invocation duration that do not have the volatile type and have been
 changed between the setjmp() invocation and longjmp() call are
 indeterminate.
-->8--
  Looks like we're being lucky here.
- a bunch of local variables are marked as volatile even though they're
  not touched between setjmp and longjmp.  This leads to inefficient
  compiled code.

* Sync read/write loop with file_get():
- no need for temp variable "i", "wlen" is enough and appropriately typed
- make the loop condition/exit more similar (simpler)
- there's no point in checking for write(2) returning 0
- properly save and restore errno in case of fread(3) failure

size(1) says the resulting executables shrink on amd64.

Reviews and oks welcome.


Index: fetch.c
===
--- fetch.c.orig
+++ fetch.c
@@ -77,7 +77,7 @@ static char   hextochar(const char *);
 static char*urldecode(const char *);
 static char*recode_credentials(const char *_userinfo);
 static char*ftp_readline(FILE *, size_t *);
-static voidftp_close(FILE **, struct tls **, volatile int *);
+static voidftp_close(FILE **, struct tls **, int *);
 static const char *sockerror(struct tls *);
 #ifdef SMALL
 #defineftp_printf(fp, ...) fprintf(fp, __VA_ARGS__)
@@ -311,13 +311,13 @@ url_get(const char *origline, const char
char pbuf[NI_MAXSERV], hbuf[NI_MAXHOST], *cp, *portnum, *path, ststr[4];
char *hosttail, *cause = "unknown", *newline, *host, *port, *buf = NULL;
char *epath, *redirurl, *loctail, *h, *p, gerror[200];
-   int error, i, isftpurl = 0, isredirect = 0, rval = -1;
+   int error, isftpurl = 0, isredirect = 0, rval = -1;
int isunavail = 0, retryafter = -1;
struct addrinfo hints, *res0, *res;
-   const char * volatile savefile;
-   char * volatile proxyurl = NULL;
+   const char *savefile;
+   char *proxyurl = NULL;
char *credentials = NULL;
-   volatile int fd = -1, out = -1;
+   int fd = -1, out = -1;
volatile sig_t oldintr, oldinti;
FILE *fin = NULL;
off_t hashbytes;
@@ -1013,6 +1013,10 @@ noslash:
 #endif
}
 
+   free(buf);
+   if ((buf = malloc(buflen)) == NULL)
+   errx(1, "Can't allocate memory for transfer buffer");
+
/* Trap signals */
oldintr = NULL;
oldinti = NULL;
@@ -1029,11 +1033,7 @@ noslash:
hashbytes = mark;
progressmeter(-1, path);
 
-   free(buf);
-
/* Finally, suck down the file. */
-   if ((buf = malloc(buflen)) == NULL)
-   errx(1, "Can't allocate memory for transfer buffer");
oldinti = signal(SIGINFO, psummary);
if (chunked) {
error = save_chunked(fin, tls, out, buf, buflen);
@@ -1041,18 +1041,14 @@ noslash:
if (error == -1)
goto cleanup_url_get;
} else {
-   i = 0;
-   len = 1;
-   while (len > 0) {
-   len = fread(buf, 1, buflen, fin);
+   while ((len = fread(buf, 1, buflen, fin)) > 0) {
bytes += len;
-   for (cp = buf, wlen = len; wlen > 0; wlen -= i, cp += 
i) {
-   if ((i = write(out, cp, wlen)) == -1) {
+   for (cp = buf; len > 0; len -= wlen, cp += wlen) {
+   if ((wlen = write(out, cp, len)) == -1) {
warn("Writing %s", savefile);
signal(SIGINFO, oldinti);
goto cleanup_url_get;
-   } else if (i == 0)
-   break;
+   }
}
if (hash && !progress) {
while (bytes >= hashbytes) {
@@ -1062,6 +1058,7 @@ noslash:
(void)fflush(ttyout);
}
}
+   save_errno = errno;
signal(SIGINFO, oldinti);
if (hash && !progress && bytes > 0) {
if (bytes < mark)
@@ -1069,7 +1066,8 @@ noslash:
(void)putc('\n', ttyout);
(void)fflush(ttyout);
}
-   if (len != 0) {
+   if (len == 0 && ferror(fin)) {
+   errno = save_errno;
warnx("Reading from socket: %s", sockerror(tls));
goto 

Re: ksh: support "function name()"

2020-01-07 Thread Jeremie Courreges-Anglas
On Tue, Jan 07 2020, Klemens Nanni  wrote:
> On Tue, Jan 07, 2020 at 06:47:16PM +0100, Jeremie Courreges-Anglas wrote:
>> Bah, I think I understand why this was chosen.  bash functions declared
>> with "function name" or "function name()" aren't special.  Probably we
>> should do the same.

... the same as mksh.  Sorry, that was misleading.

>> I'm postponing this for now, thanks for the
>> feedback so far.
> I think we should keep differences between the two forms, having $0
> expand to the function name is a nice feature I do use.

I do not intend to change the existing behavior of "function name".

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



Re: ksh: support "function name()"

2020-01-07 Thread Jeremie Courreges-Anglas
On Sat, Dec 28 2019, Klemens Nanni  wrote:
> On Sat, Dec 28, 2019 at 04:07:02PM +0100, Mark Kettenis wrote:
>> Are there other ksh implementations that have this "feature"?
> MirBSD's ksh allows all three forms but treats `function name()' like
> `name()', that is $0 stays the same and will not be set to the funtion's
> name:

Bah, I think I understand why this was chosen.  bash functions declared
with "function name" or "function name()" aren't special.  Probably we
should do the same.  I'm postponing this for now, thanks for the
feedback so far.

>   $ echo $KSH_VERSION
>   @(#)MIRBSD KSH R57 2019/03/01
>   $ function f { echo $0; }
>   $ f2() { echo $0; }
>   $ function f3() { echo $0; }
>   $ typeset -f
>   function f {
>   \echo $0 
>   } 
>   f2() {
>   \echo $0 
>   } 
>   f3() {
>   \echo $0 
>   } 
>   $ f
>   f
>   $ f2
>   mksh
>   $ f3
>   mksh
>

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



ksh: support "function name()"

2019-12-28 Thread Jeremie Courreges-Anglas


We have a few ports (~12) patched because of shell script constructs
like

  function usage()
  {

which are rejected by our ksh.  Indeed ksh only supports either

  usage()
  {

or

  function usage
  {

Both bash and zsh also allow an optional "()".  The diff below
implements the missing bits.

Since the "reject = true;" mechanism bypasses yylex(), I have to
pass token() the same flags as with the musthave('{') call below.

ok?


Index: ksh.1
===
RCS file: /d/cvs/src/bin/ksh/ksh.1,v
retrieving revision 1.208
diff -u -p -r1.208 ksh.1
--- ksh.1   26 Nov 2019 22:49:01 -  1.208
+++ ksh.1   27 Dec 2019 12:27:37 -
@@ -679,7 +679,7 @@ The exit status of a
 statement is the last exit status of the
 .Ar list
 in the body of the loop; if the body is not executed, the exit status is zero.
-.It Xo Ic function Ar name
+.It Xo Ic function Ar name Op ()
 .No { Ar list ; No }
 .Xc
 Defines the function
Index: syn.c
===
RCS file: /d/cvs/src/bin/ksh/syn.c,v
retrieving revision 1.39
diff -u -p -r1.39 syn.c
--- syn.c   24 Apr 2018 08:25:16 -  1.39
+++ syn.c   27 Dec 2019 11:25:41 -
@@ -555,6 +555,12 @@ function_body(char *name,
 * an open-brace.
 */
if (ksh_func) {
+   /* allow optional () after function name */
+   if (token(CONTIN|KEYWORD|ALIAS) == '(')
+   musthave(')', 0);
+   else
+   reject = true;
+
musthave('{', CONTIN|KEYWORD|ALIAS); /* } */
reject = true;
}


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



Re: [UPDATE] xterm 351

2019-12-26 Thread Jeremie Courreges-Anglas
On Mon, Dec 23 2019, Matthieu Herrb  wrote:
> Hi,
>
> the diff below (also available as https://xenocara.org/xterm-351.diff)
> updates xterm to version 351 (from current version 344 in xenocara).
>
> For a detailed change log see
> https://invisible-island.net/xterm/xterm.log.html
>
> Please test and report. Oks welcome too.

Works for me, ok jca@

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



Re: ftp(1): separate file:/ URL handling

2019-12-26 Thread Jeremie Courreges-Anglas
On Thu, Dec 19 2019, Jeremie Courreges-Anglas  wrote:
> A bit late...
>
> Move file: URL handling into its own function.  This simplifies
> url_get() and would have prevented problems with bogus redirections.
>
> file_get() unrolls the code previously used in url_get(), except the
> #ifndef SMALL bits were stripped out.  file: support is mainly
> (only?) used in the installer which is built with SMALL defined.
> Resuming an incomplete file: transfer sounds nuts.
>
> I felt a bit guilty about copying dubious code, there are some changes
> that ought to be applied to url_get() too:
> - write(2) can't return 0, can it? (something old about non-blocking
>   sockets maybe?).  Anyway, no need to handle 0 explicitely.
> - allocate buf before setjmp instead of marking it volatile
> - save_errno/warnc dance if read(2) fails
>
> This survived make release on amd64 and a bsd.rd upgrade with sets
> on 'disk'.  The resulting ftp(1) binary size decreases.
>
> Comments/ok?

Still looking for feedback, else I'll resort to crowdtesting. ;)

>
> Index: fetch.c
> ===
> --- fetch.c.orig
> +++ fetch.c
> @@ -68,6 +68,7 @@ struct tls;
>  #include "ftp_var.h"
>  #include "cmds.h"
>  
> +static int   file_get(const char *, const char *);
>  static int   url_get(const char *, const char *, const char *, int);
>  static int   save_chunked(FILE *, struct tls *, int , char *, size_t);
>  static void  aborthttp(int);
> @@ -182,6 +183,125 @@ tooslow(int signo)
>  }
>  
>  /*
> + * Copy a local file (used by the OpenBSD installer).
> + * Returns -1 on failure, 0 on success
> + */
> +static int
> +file_get(const char *path, const char *outfile)
> +{
> + struct stat  st;
> + int  fd, out, rval = -1, save_errno;
> + volatile sig_t   oldintr, oldinti;
> + const char  *savefile;
> + char*buf = NULL, *cp;
> + const size_t buflen = 128 * 1024;
> + off_thashbytes;
> + ssize_t  len, wlen;
> +
> + direction = "received";
> +
> + fd = open(path, O_RDONLY);
> + if (fd == -1) {
> + warn("Can't open file %s", path);
> + return -1;
> + }
> +
> + if (fstat(fd, ) == -1)
> + filesize = -1;
> + else
> + filesize = st.st_size;
> +
> + if (outfile != NULL)
> + savefile = outfile;
> + else {
> + if (path[strlen(path) - 1] == '/')  /* Consider no file */
> + savefile = NULL;/* after dir invalid. */
> + else
> + savefile = basename(path);
> + }
> +
> + if (EMPTYSTRING(savefile)) {
> + warnx("No filename after directory (use -o): %s", path);
> + goto cleanup_copy;
> + }
> +
> + /* Open the output file.  */
> + if (!pipeout) {
> + out = open(savefile, O_CREAT | O_WRONLY | O_TRUNC, 0666);
> + if (out == -1) {
> + warn("Can't open %s", savefile);
> + goto cleanup_copy;
> + }
> + } else
> + out = fileno(stdout);
> +
> + if ((buf = malloc(buflen)) == NULL)
> + errx(1, "Can't allocate memory for transfer buffer");
> +
> + /* Trap signals */
> + oldintr = NULL;
> + oldinti = NULL;
> + if (setjmp(httpabort)) {
> + if (oldintr)
> + (void)signal(SIGINT, oldintr);
> + if (oldinti)
> + (void)signal(SIGINFO, oldinti);
> + goto cleanup_copy;
> + }
> + oldintr = signal(SIGINT, abortfile);
> +
> + bytes = 0;
> + hashbytes = mark;
> + progressmeter(-1, path);
> +
> + /* Finally, suck down the file. */
> + oldinti = signal(SIGINFO, psummary);
> + while ((len = read(fd, buf, buflen)) > 0) {
> + bytes += len;
> + for (cp = buf; len > 0; len -= wlen, cp += wlen) {
> + if ((wlen = write(out, cp, len)) == -1) {
> + warn("Writing %s", savefile);
> + signal(SIGINFO, oldinti);
> + goto cleanup_copy;
> + }
> + }
> + if (hash && !progress) {
> + while (bytes >= hashbytes) {
> + (void)putc('#', ttyout);
> + hashbytes += mark;
> + }
> + (void)fflus

ftp(1): separate file:/ URL handling

2019-12-18 Thread Jeremie Courreges-Anglas


A bit late...

Move file: URL handling into its own function.  This simplifies
url_get() and would have prevented problems with bogus redirections.

file_get() unrolls the code previously used in url_get(), except the
#ifndef SMALL bits were stripped out.  file: support is mainly
(only?) used in the installer which is built with SMALL defined.
Resuming an incomplete file: transfer sounds nuts.

I felt a bit guilty about copying dubious code, there are some changes
that ought to be applied to url_get() too:
- write(2) can't return 0, can it? (something old about non-blocking
  sockets maybe?).  Anyway, no need to handle 0 explicitely.
- allocate buf before setjmp instead of marking it volatile
- save_errno/warnc dance if read(2) fails

This survived make release on amd64 and a bsd.rd upgrade with sets
on 'disk'.  The resulting ftp(1) binary size decreases.

Comments/ok?


Index: fetch.c
===
--- fetch.c.orig
+++ fetch.c
@@ -68,6 +68,7 @@ struct tls;
 #include "ftp_var.h"
 #include "cmds.h"
 
+static int file_get(const char *, const char *);
 static int url_get(const char *, const char *, const char *, int);
 static int save_chunked(FILE *, struct tls *, int , char *, size_t);
 static voidaborthttp(int);
@@ -182,6 +183,125 @@ tooslow(int signo)
 }
 
 /*
+ * Copy a local file (used by the OpenBSD installer).
+ * Returns -1 on failure, 0 on success
+ */
+static int
+file_get(const char *path, const char *outfile)
+{
+   struct stat  st;
+   int  fd, out, rval = -1, save_errno;
+   volatile sig_t   oldintr, oldinti;
+   const char  *savefile;
+   char*buf = NULL, *cp;
+   const size_t buflen = 128 * 1024;
+   off_thashbytes;
+   ssize_t  len, wlen;
+
+   direction = "received";
+
+   fd = open(path, O_RDONLY);
+   if (fd == -1) {
+   warn("Can't open file %s", path);
+   return -1;
+   }
+
+   if (fstat(fd, ) == -1)
+   filesize = -1;
+   else
+   filesize = st.st_size;
+
+   if (outfile != NULL)
+   savefile = outfile;
+   else {
+   if (path[strlen(path) - 1] == '/')  /* Consider no file */
+   savefile = NULL;/* after dir invalid. */
+   else
+   savefile = basename(path);
+   }
+
+   if (EMPTYSTRING(savefile)) {
+   warnx("No filename after directory (use -o): %s", path);
+   goto cleanup_copy;
+   }
+
+   /* Open the output file.  */
+   if (!pipeout) {
+   out = open(savefile, O_CREAT | O_WRONLY | O_TRUNC, 0666);
+   if (out == -1) {
+   warn("Can't open %s", savefile);
+   goto cleanup_copy;
+   }
+   } else
+   out = fileno(stdout);
+
+   if ((buf = malloc(buflen)) == NULL)
+   errx(1, "Can't allocate memory for transfer buffer");
+
+   /* Trap signals */
+   oldintr = NULL;
+   oldinti = NULL;
+   if (setjmp(httpabort)) {
+   if (oldintr)
+   (void)signal(SIGINT, oldintr);
+   if (oldinti)
+   (void)signal(SIGINFO, oldinti);
+   goto cleanup_copy;
+   }
+   oldintr = signal(SIGINT, abortfile);
+
+   bytes = 0;
+   hashbytes = mark;
+   progressmeter(-1, path);
+
+   /* Finally, suck down the file. */
+   oldinti = signal(SIGINFO, psummary);
+   while ((len = read(fd, buf, buflen)) > 0) {
+   bytes += len;
+   for (cp = buf; len > 0; len -= wlen, cp += wlen) {
+   if ((wlen = write(out, cp, len)) == -1) {
+   warn("Writing %s", savefile);
+   signal(SIGINFO, oldinti);
+   goto cleanup_copy;
+   }
+   }
+   if (hash && !progress) {
+   while (bytes >= hashbytes) {
+   (void)putc('#', ttyout);
+   hashbytes += mark;
+   }
+   (void)fflush(ttyout);
+   }
+   }
+   save_errno = errno;
+   signal(SIGINFO, oldinti);
+   if (hash && !progress && bytes > 0) {
+   if (bytes < mark)
+   (void)putc('#', ttyout);
+   (void)putc('\n', ttyout);
+   (void)fflush(ttyout);
+   }
+   if (len == -1) {
+   warnc(save_errno, "Reading from file");
+   goto cleanup_copy;
+   }
+   progressmeter(1, NULL);
+   if (verbose)
+   ptransfer(0);
+   (void)signal(SIGINT, oldintr);
+
+   rval = 0;
+
+cleanup_copy:
+   free(buf);
+   if (out >= 0 && out != fileno(stdout))
+   

Re: /usr/bin/ftp: 308 Permanent Redirect

2019-12-14 Thread Jeremie Courreges-Anglas
On Sat, Dec 14 2019, Christopher Zimmermann  wrote:
> Hi,

Hi,

> to me it seems that since our ftp does only support GET requests anyway,
> a 308 Redirect response can be handled identical to a 301 Redirect. OK?

ok jca@

Do you need this for particular websites / implementations?

> Christopher
>
>
> Index: fetch.c
> ===
> RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
> retrieving revision 1.184
> diff -u -p -r1.184 fetch.c
> --- fetch.c   9 Dec 2019 19:05:06 -   1.184
> +++ fetch.c   14 Dec 2019 11:10:09 -
> @@ -834,6 +834,7 @@ noslash:
>   case 302:   /* Found */
>   case 303:   /* See Other */
>   case 307:   /* Temporary Redirect */
> + case 308:   /* Moved Permanently */
>   isredirect++;
>   if (redirect_loop++ > 10) {
>   warnx("Too many redirections requested");

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



Re: rpki-client: improve the distclean target

2019-12-10 Thread Jeremie Courreges-Anglas
On Mon, Dec 09 2019, "Marco d'Itri"  wrote:
> Without this patch distclean may leave around some *.old files generated 
> by configure.

The portable build system is not used or maintained within the OpenBSD
source tree*.  You'll be better off reporting this to Kristaps at

  https://github.com/kristapsdz/rpki-client/

* 
https://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/src/usr.sbin/rpki-client/Makefile?rev=1.14=text/plain

> --- a/Makefile
> +++ b/Makefile
> @@ -73,7 +73,7 @@ clean:
>   rm -f $(BINS) $(ALLOBJS) rpki-client.install.8
>  
>  distclean: clean
> - rm -f config.h config.log Makefile.configure
> + rm -f config.h config.log config.h.old config.log.old Makefile.configure
>  
>  $(ALLOBJS): extern.h config.h

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



ftp(1) fetch.c: print sent headers with -d

2019-12-09 Thread Jeremie Courreges-Anglas


Since rev 1.176 TLS connections are also handled with stdio.  When
removing the ftp_printf wrapper I also removed the optional printing of
headers sent to the server.  The diff below reinstates ftp_printf
for !SMALL builds.  For ramdisks, ftp_printf is just a #define so size
doesn't change.

ok?


Index: fetch.c
===
--- fetch.c.orig
+++ fetch.c
@@ -78,6 +78,11 @@ static char  *recode_credentials(const ch
 static char*ftp_readline(FILE *, size_t *);
 static voidftp_close(FILE **, struct tls **, volatile int *);
 static const char *sockerror(struct tls *);
+#ifdef SMALL
+#defineftp_printf(fp, ...) fprintf(fp, __VA_ARGS__)
+#else
+static int ftp_printf(FILE *, const char *, ...);
+#endif /* SMALL */
 #ifndef NOSSL
 static int proxy_connect(int, char *, char *);
 static int stdio_tls_write_wrapper(void *, const char *, int);
@@ -695,14 +700,14 @@ noslash:
 * the original URI (path).
 */
if (credentials)
-   fprintf(fin, "GET %s HTTP/1.1\r\n"
+   ftp_printf(fin, "GET %s HTTP/1.1\r\n"
"Connection: close\r\n"
"Proxy-Authorization: Basic %s\r\n"
"Host: %s\r\n%s%s\r\n\r\n",
epath, credentials,
proxyhost, buf ? buf : "", httpuseragent);
else
-   fprintf(fin, "GET %s HTTP/1.1\r\n"
+   ftp_printf(fin, "GET %s HTTP/1.1\r\n"
"Connection: close\r\n"
"Host: %s\r\n%s%s\r\n\r\n",
epath, proxyhost, buf ? buf : "", httpuseragent);
@@ -721,7 +726,7 @@ noslash:
 #endif /* SMALL */
 #ifndef NOSSL
if (credentials) {
-   fprintf(fin,
+   ftp_printf(fin,
"GET /%s HTTP/1.1\r\n"
"Connection: close\r\n"
"Authorization: Basic %s\r\n"
@@ -730,12 +735,12 @@ noslash:
credentials = NULL;
} else
 #endif /* NOSSL */
-   fprintf(fin,
+   ftp_printf(fin,
"GET /%s HTTP/1.1\r\n"
"Connection: close\r\n"
"Host: ", epath);
if (proxyhost) {
-   fprintf(fin, "%s", proxyhost);
+   ftp_printf(fin, "%s", proxyhost);
port = NULL;
} else if (strchr(host, ':')) {
/*
@@ -747,10 +752,10 @@ noslash:
errx(1, "Can't allocate memory.");
if ((p = strchr(h, '%')) != NULL)
*p = '\0';
-   fprintf(fin, "[%s]", h);
+   ftp_printf(fin, "[%s]", h);
free(h);
} else
-   fprintf(fin, "%s", host);
+   ftp_printf(fin, "%s", host);
 
/*
 * Send port number only if it's specified and does not equal
@@ -759,15 +764,15 @@ noslash:
 */
 #ifndef NOSSL
if (port && strcmp(port, (ishttpsurl ? "443" : "80")) != 0)
-   fprintf(fin, ":%s", port);
+   ftp_printf(fin, ":%s", port);
if (restart_point)
-   fprintf(fin, "\r\nRange: bytes=%lld-",
+   ftp_printf(fin, "\r\nRange: bytes=%lld-",
(long long)restart_point);
 #else /* !NOSSL */
if (port && strcmp(port, "80") != 0)
-   fprintf(fin, ":%s", port);
+   ftp_printf(fin, ":%s", port);
 #endif /* !NOSSL */
-   fprintf(fin, "\r\n%s%s\r\n\r\n",
+   ftp_printf(fin, "\r\n%s%s\r\n\r\n",
buf ? buf : "", httpuseragent);
}
free(epath);
@@ -1614,6 +1619,27 @@ ftp_readline(FILE *fp, size_t *lenp)
return fparseln(fp, lenp, NULL, "\0\0\0", 0);
 }
 
+#ifndef SMALL
+static int
+ftp_printf(FILE *fp, const char *fmt, ...)
+{
+   va_list ap;
+   int ret;
+
+   va_start(ap, fmt);
+   ret = vfprintf(fp, fmt, ap);
+   va_end(ap);
+
+   if (debug) {
+   va_start(ap, fmt);
+   vfprintf(ttyout, fmt, ap);
+   va_end(ap);
+   }
+
+   return ret;
+}
+#endif /* !SMALL */
+
 static void
 ftp_close(FILE **fin, struct tls **tls, volatile int *fd)
 {


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



Re: Infinite sleeps in kern/vfs_*

2019-12-08 Thread Jeremie Courreges-Anglas
On Thu, Dec 05 2019, Martin Pieuchot  wrote:
> Convert them to tsleep_nsec(9), ok? 

Pretty mechanical, no regression spotted on my laptop or on my amd64 and
sparc64 builders.  ok jca@

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



Re: Infinite sleeps in sys/uvm

2019-12-08 Thread Jeremie Courreges-Anglas
On Thu, Dec 05 2019, Martin Pieuchot  wrote:
> Convert them to {m,t}sleep_nsec(9), ok?

Pretty mechanical, no regression spotted on my laptop or on my amd64 and
sparc64 builders.  ok jca@

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



Re: C11 visibility in libc++

2019-12-08 Thread Jeremie Courreges-Anglas
On Sat, Dec 07 2019, Jonathan Gray  wrote:
> While we don't have C11's quick_exit() we do have timespec_get() and
> struct timespec/aligned_alloc().

LGTM, ok jca@

For other readers, no shared_libs bump is needed, this only affects
the headers.


> Index: include/__config
> ===
> RCS file: /cvs/src/lib/libcxx/include/__config,v
> retrieving revision 1.6
> diff -u -p -r1.6 __config
> --- include/__config  17 Jun 2019 22:28:51 -  1.6
> +++ include/__config  7 Dec 2019 03:10:51 -
> @@ -341,6 +341,9 @@
>  #  if defined(__FreeBSD__)
>  #define _LIBCPP_HAS_QUICK_EXIT
>  #define _LIBCPP_HAS_C11_FEATURES
> +#  elif defined(__OpenBSD__)
> +#define _LIBCPP_HAS_TIMESPEC_GET
> +#define _LIBCPP_HAS_C11_FEATURES
>  #  elif defined(__Fuchsia__)
>  #define _LIBCPP_HAS_QUICK_EXIT
>  #define _LIBCPP_HAS_TIMESPEC_GET
>

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



Re: ftp(1): https:// handling with NOSSL

2019-12-05 Thread Jeremie Courreges-Anglas
On Wed, Dec 04 2019, Jeremie Courreges-Anglas  wrote:
> ftp(1) built with no TLS support is confused (confusing?) when handled
> an https url.  I have noticed this during tests with
> /usr/src/distrib/special/ftp.
>
> Now:
> --8<--
> ritchie /usr/src/distrib/special/ftp$ obj/ftp -o/dev/null 
> https://www.openbsd.org/
> ftp: https: no address associated with name
> ftp: Can't connect or login to host `https'
> -->8--
>
> The fix is easy: let url_get() decide if it can handle the URL, as
> suggested by the comment which has been there since TLS support has been
> added.  The diff below addresses this and also kills the ineffective
> comment.
>
> With the diff:
> --8<--
> ritchie /usr/src/distrib/special/ftp$ obj/ftp -o/dev/null 
> https://www.openbsd.org/
> ftp: url_get: Invalid URL 'https://www.openbsd.org/'
> -->8--
>
> --8<--
> ritchie /usr/src/distrib/special/ftp$ size fetch.o obj/fetch.o ftp obj/ftp
> textdatabss dec hex
> 10927   0   104 11031   2b17fetch.o
> 11002   0   104 11106   2b62obj/fetch.o
> 407983  12904   43328   464215  71557   ftp
> 408079  12904   43328   464311  715b7   obj/ftp
> -->8--
>
> ok?  (assuming it fits on i386 floppies)

Rebased on top of cvs HEAD, I noticed a bunch of #ifdef inconsistencies
that were just too ugly.

So after feedback from Theo: print an explicit message if ftp(1) was
built without HTTPS support.  Printing a helpful message in this case
makes a lot of sense.

No need to show the function name in that case, but I'm keeping the
existing "url_get: Invalid URL '%s'" error as is; it shouldn't happen.

ok?


Index: fetch.c
===
RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.181
diff -u -p -r1.181 fetch.c
--- fetch.c 5 Dec 2019 10:26:25 -   1.181
+++ fetch.c 5 Dec 2019 10:41:14 -
@@ -240,14 +240,16 @@ url_get(const char *origline, const char
 #ifndef SMALL
scheme = FILE_URL;
 #endif /* !SMALL */
-#ifndef NOSSL
} else if (strncasecmp(newline, HTTPS_URL, sizeof(HTTPS_URL) - 1) == 0) 
{
+#ifndef NOSSL
host = newline + sizeof(HTTPS_URL) - 1;
ishttpsurl = 1;
+#else
+   errx(1, "%s: No HTTPS support", newline);
+#endif /* !NOSSL */
 #ifndef SMALL
scheme = HTTPS_URL;
 #endif /* !SMALL */
-#endif /* !NOSSL */
} else
errx(1, "url_get: Invalid URL '%s'", newline);
 
@@ -1266,10 +1268,7 @@ auto_fetch(int argc, char *argv[], char 
 * Try HTTP URL-style arguments first.
 */
if (strncasecmp(url, HTTP_URL, sizeof(HTTP_URL) - 1) == 0 ||
-#ifndef NOSSL
-   /* even if we compiled without SSL, url_get will check */
strncasecmp(url, HTTPS_URL, sizeof(HTTPS_URL) -1) == 0 ||
-#endif /* !NOSSL */
strncasecmp(url, FILE_URL, sizeof(FILE_URL) - 1) == 0) {
redirect_loop = 0;
retried = 0;

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



Re: [patch] ftp: improve SMALL and NOSSL #ifdefs

2019-12-05 Thread Jeremie Courreges-Anglas
On Wed, Nov 06 2019, Hiltjo Posthuma  wrote:

[...]

> Thanks for reviewing the patch. Sadly I noticed and made a stupid mistake. 
> When
> NOSSL is set, but SMALL is not set.  It will set scheme = HTTPS_URL for the
> file handler.
>
> Below is the full updated patch:

I think we don't want to maintain tons of #ifdef combinations, but
applying your diff just made sense after I tried to modify nearby code
and I noticed the inconsistencies.

Committed, thanks!

> diff --git usr.bin/ftp/fetch.c usr.bin/ftp/fetch.c
> index 4c7e14b04bd..4511fb29fa1 100644
> --- usr.bin/ftp/fetch.c
> +++ usr.bin/ftp/fetch.c
> @@ -201,14 +201,14 @@ url_get(const char *origline, const char *proxyenv, 
> const char *outfile, int las
>   char *proxyhost = NULL;
>  #ifndef NOSSL
>   char *sslpath = NULL, *sslhost = NULL;
> - char *full_host = NULL;
> - const char *scheme;
>   int ishttpurl = 0, ishttpsurl = 0;
>  #endif /* !NOSSL */
>  #ifndef SMALL
> + char *full_host = NULL;
> + const char *scheme;
>   char *locbase;
>   struct addrinfo *ares = NULL;
> -#endif
> +#endif /* !SMALL */
>   struct tls *tls = NULL;
>   int status;
>   int save_errno;
> @@ -221,8 +221,10 @@ url_get(const char *origline, const char *proxyenv, 
> const char *outfile, int las
>   errx(1, "Can't allocate memory to parse URL");
>   if (strncasecmp(newline, HTTP_URL, sizeof(HTTP_URL) - 1) == 0) {
>   host = newline + sizeof(HTTP_URL) - 1;
> -#ifndef SMALL
> +#ifndef NOSSL
>   ishttpurl = 1;
> +#endif /* !NOSSL */
> +#ifndef SMALL
>   scheme = HTTP_URL;
>  #endif /* !SMALL */
>   } else if (strncasecmp(newline, FTP_URL, sizeof(FTP_URL) - 1) == 0) {
> @@ -234,12 +236,16 @@ url_get(const char *origline, const char *proxyenv, 
> const char *outfile, int las
>   } else if (strncasecmp(newline, FILE_URL, sizeof(FILE_URL) - 1) == 0) {
>   host = newline + sizeof(FILE_URL) - 1;
>   isfileurl = 1;
> -#ifndef NOSSL
> +#ifndef SMALL
>   scheme = FILE_URL;
> +#endif /* !SMALL */
> +#ifndef NOSSL
>   } else if (strncasecmp(newline, HTTPS_URL, sizeof(HTTPS_URL) - 1) == 0) 
> {
>   host = newline + sizeof(HTTPS_URL) - 1;
>   ishttpsurl = 1;
> +#ifndef SMALL
>   scheme = HTTPS_URL;
> +#endif /* !SMALL */
>  #endif /* !NOSSL */
>   } else
>   errx(1, "url_get: Invalid URL '%s'", newline);
> @@ -1066,8 +1072,10 @@ improper:
>   warnx("Improper response from %s", host);
>  
>  cleanup_url_get:
> -#ifndef NOSSL
> +#ifndef SMALL
>   free(full_host);
> +#endif /* !SMALL */
> +#ifndef NOSSL
>   free(sslhost);
>  #endif /* !NOSSL */
>   ftp_close(, , );

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



ftp(1): https:// handling with NOSSL

2019-12-04 Thread Jeremie Courreges-Anglas


ftp(1) built with no TLS support is confused (confusing?) when handled
an https url.  I have noticed this during tests with
/usr/src/distrib/special/ftp.

Now:
--8<--
ritchie /usr/src/distrib/special/ftp$ obj/ftp -o/dev/null 
https://www.openbsd.org/
ftp: https: no address associated with name
ftp: Can't connect or login to host `https'
-->8--

The fix is easy: let url_get() decide if it can handle the URL, as
suggested by the comment which has been there since TLS support has been
added.  The diff below addresses this and also kills the ineffective
comment.

With the diff:
--8<--
ritchie /usr/src/distrib/special/ftp$ obj/ftp -o/dev/null 
https://www.openbsd.org/
ftp: url_get: Invalid URL 'https://www.openbsd.org/'
-->8--

--8<--
ritchie /usr/src/distrib/special/ftp$ size fetch.o obj/fetch.o ftp obj/ftp
textdatabss dec hex
10927   0   104 11031   2b17fetch.o
11002   0   104 11106   2b62obj/fetch.o
407983  12904   43328   464215  71557   ftp
408079  12904   43328   464311  715b7   obj/ftp
-->8--

ok?  (assuming it fits on i386 floppies)


Index: fetch.c
===
RCS file: /d/cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.180
diff -u -p -r1.180 fetch.c
--- fetch.c 2 Dec 2019 22:32:18 -   1.180
+++ fetch.c 4 Dec 2019 17:07:21 -
@@ -1258,10 +1258,7 @@ auto_fetch(int argc, char *argv[], char 
 * Try HTTP URL-style arguments first.
 */
if (strncasecmp(url, HTTP_URL, sizeof(HTTP_URL) - 1) == 0 ||
-#ifndef NOSSL
-   /* even if we compiled without SSL, url_get will check */
-   strncasecmp(url, HTTPS_URL, sizeof(HTTPS_URL) -1) == 0 ||
-#endif /* !NOSSL */
+   strncasecmp(url, HTTPS_URL, sizeof(HTTPS_URL) - 1) == 0 ||
strncasecmp(url, FILE_URL, sizeof(FILE_URL) - 1) == 0) {
redirect_loop = 0;
retried = 0;


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



Re: grep -R with no path

2019-12-03 Thread Jeremie Courreges-Anglas
On Tue, Dec 03 2019, Klemens Nanni  wrote:
> On Mon, Dec 02, 2019 at 08:31:02AM +, Miod Vallat wrote:
>> grep(1), when invoked with the -R option but no path, displays a
>> "recursive search of stdin" warning and acts as if -R had not been
>> specified.
>> 
>> GNU grep, in that case, will perform a recursive search in the current
>> directory, i.e. uses an implicit "." path if none is given.
>> 
>> This is IMO a much better behaviour. What about the following diff?
> Document this.
>
> OK?

yep, ok jca@

> Index: grep.1
> ===
> RCS file: /cvs/src/usr.bin/grep/grep.1,v
> retrieving revision 1.49
> diff -u -p -r1.49 grep.1
> --- grep.17 Oct 2019 20:51:34 -   1.49
> +++ grep.12 Dec 2019 23:58:56 -
> @@ -246,6 +246,11 @@ will only search a file until a match ha
>  making searches potentially less expensive.
>  .It Fl R
>  Recursively search subdirectories listed.
> +If no
> +.Ar file
> +is given,
> +.Nm
> +searches the working directory.
>  .It Fl s
>  Silent mode.
>  Nonexistent and unreadable files are ignored
>

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



Re: grep -R with no path

2019-12-02 Thread Jeremie Courreges-Anglas
On Mon, Dec 02 2019, Jeremie Courreges-Anglas  wrote:

[...]

> I have had a similar diff for some time but didn't push for it because
> nitpicking: GNU grep doesn't prepend "./" to file names in this case.
> I have a diff to do this but let's keep the nitpicking for later.

Current:
--8<--
ritchie /usr/src/usr.bin/grep$ grep -R grep_tree
./grep.c:   c = grep_tree(argv);
./grep.h:int grep_tree(char **argv);
./util.c:grep_tree(char **argv)
Binary file ./grep matches
./grep.c.orig:  c = grep_tree(argv);
./util.c.orig:grep_tree(char **argv)
-->8--

With the diff below (matches GNU grep)
--8<--
ritchie /usr/src/usr.bin/grep$ obj/grep -R grep_tree
grep.c: c = grep_tree(argv);
grep.h:int   grep_tree(char **argv);
util.c:grep_tree(char **argv)
Binary file grep matches
grep.c.orig:c = grep_tree(argv);
util.c.orig:grep_tree(char **argv)
-->88

Explicit "."
--8<--
ritchie /usr/src/usr.bin/grep$ obj/grep -R grep_tree .
./grep.c:   c = grep_tree(argv);
./grep.h:int grep_tree(char **argv);
./util.c:grep_tree(char **argv)
Binary file ./grep matches
./grep.c.orig:  c = grep_tree(argv);
./util.c.orig:grep_tree(char **argv)
-->8--

This diff moves the fake argv logic moves into grep_tree() where we can
opt for pretty-printing.  ok?


Index: grep.c
===
RCS file: /cvs/src/usr.bin/grep/grep.c,v
retrieving revision 1.63
diff -u -p -r1.63 grep.c
--- grep.c  2 Dec 2019 21:50:11 -   1.63
+++ grep.c  2 Dec 2019 21:53:14 -
@@ -473,12 +473,6 @@ main(int argc, char *argv[])
++argv;
}
 
-   if (Rflag && argc == 0) {
-   /* default to . if no path given */
-   static char *dot_argv[] = { ".", NULL };
-   argv = dot_argv;
-   argc = 1;
-   }
if (Eflag)
cflags |= REG_EXTENDED;
if (Fflag)
@@ -516,7 +510,7 @@ main(int argc, char *argv[])
if ((argc == 0 || argc == 1) && !Rflag && !Hflag)
hflag = 1;
 
-   if (argc == 0)
+   if (argc == 0 && !Rflag)
exit(!procfile(NULL));
 
if (Rflag)
Index: util.c
===
RCS file: /cvs/src/usr.bin/grep/util.c,v
retrieving revision 1.61
diff -u -p -r1.61 util.c
--- util.c  7 Oct 2019 17:47:32 -   1.61
+++ util.c  2 Dec 2019 21:53:15 -
@@ -61,6 +61,12 @@ grep_tree(char **argv)
FTS *fts;
FTSENT  *p;
int c, fts_flags;
+   char*dot_argv[] = { ".", NULL };
+   char*path;
+
+   /* default to . if no path given */
+   if (argv[0] == NULL)
+   argv = dot_argv;
 
c = 0;
 
@@ -81,7 +87,11 @@ grep_tree(char **argv)
case FTS_DP:
break;
default:
-   c += procfile(p->fts_path);
+   path = p->fts_path;
+   /* skip "./" if implied */
+   if (argv == dot_argv && p->fts_pathlen >= 2)
+   path += 2;
+   c += procfile(path);
break;
}
}

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



Re: grep -R with no path

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

Same here, even if muscle memory helps this is just a better default IMO.

I have had a similar diff for some time but didn't push for it because
nitpicking: GNU grep doesn't prepend "./" to file names in this case.
I have a diff to do this but let's keep the nitpicking for later.

I'd like us to go with Miod's diff.  Any objection?  oks?
ok jca@ if Marc or someone else wants to commit it.

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



Re: kill VNDIOCGET60

2019-11-18 Thread Jeremie Courreges-Anglas
On Mon, Nov 18 2019, Benjamin Baier  wrote:
> Hi, found this.

No idea why I commented this out instead of just deleting it.  We're not
keeping old ioctls in other places.  Committed, thanks.

>  -- Ben
>
> Index: vndioctl.h
> ===
> RCS file: /cvs/src/sys/dev/vndioctl.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 vndioctl.h
> --- vndioctl.h14 Dec 2016 18:59:12 -  1.10
> +++ vndioctl.h18 Nov 2019 20:11:50 -
> @@ -75,8 +75,6 @@ struct vnd_user {
>   */
>  #define VNDIOCSET_IOWR('F', 0, struct vnd_ioctl) /* enable disk */
>  #define VNDIOCCLR_IOW('F', 1, struct vnd_ioctl)  /* disable disk */
> -/* XXX kill after 6.1 */
> -/* #define VNDIOCGET60   _IOWR('F', 2, struct vnd_user60) */
>  #define VNDIOCGET_IOWR('F', 3, struct vnd_user)  /* get disk info */
>  
>  #endif /* !_SYS_VNDIOCTL_H_ */
>

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



  1   2   3   4   5   6   7   >