Re: CVS commit: src/sys/dev/usb

2020-03-13 Thread Valery Ushakov
On Fri, Mar 13, 2020 at 22:26:05 -0400, Christos Zoulas wrote:

> > On Mar 13, 2020, at 10:25 PM, Valery Ushakov  wrote:
> > 
> > As I wrote in a follow up email, it changes formatting b/c you didn't
> > change field widths and IMO using %# with a field width is mostly
> > trouble to begin with.  It's not the first time someone tries to do
> > this without actually understanding the consequences of the change.
> > Please, can we assume that when people write either 0x%x or %#x they
> > most likely actaully mean it for whatever reason and that they want
> > that specific output format, and it's just rude to change that,
> > especially when you do so incorrectly.
> 
> I am reverting the fixed width ones, hold on.

Even for the ones without the widths specified.  E.g. I personally
prefer zero printed as 0x0, not as 0, so I assume that when people
choose either one that reflects their preference.  Why mess with it?
It's all so unnecessary.

-uwe


Re: CVS commit: src/sys/dev/usb

2020-03-13 Thread Christos Zoulas


> On Mar 13, 2020, at 10:25 PM, Valery Ushakov  wrote:
> 
> As I wrote in a follow up email, it changes formatting b/c you didn't
> change field widths and IMO using %# with a field width is mostly
> trouble to begin with.  It's not the first time someone tries to do
> this without actually understanding the consequences of the change.
> Please, can we assume that when people write either 0x%x or %#x they
> most likely actaully mean it for whatever reason and that they want
> that specific output format, and it's just rude to change that,
> especially when you do so incorrectly.

I am reverting the fixed width ones, hold on.

christos



signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sys/dev/usb

2020-03-13 Thread Valery Ushakov
On Fri, Mar 13, 2020 at 22:15:31 -0400, Christos Zoulas wrote:

> > This was not a part of the PR and is completely cosmetic (surely it
> > supports plain %x if it does support %#x).  Why was this necessary?
> > (I know I would be quite miffed if someone made a change like that to
> > my code).
> 
> Yes, that %x formatting change was not part of the PR, but I only
> changed 0x%x not plain %x.  I did it because as I was fixing the
> 0x%x in the log, I started changing them to %#jx so I did it
> globally in that directory for consistency.  It found two formats
> that were 0x%hu...
>
> So one can view it as a format consistency checker(not just cosmetic).

As I wrote in a follow up email, it changes formatting b/c you didn't
change field widths and IMO using %# with a field width is mostly
trouble to begin with.  It's not the first time someone tries to do
this without actually understanding the consequences of the change.
Please, can we assume that when people write either 0x%x or %#x they
most likely actaully mean it for whatever reason and that they want
that specific output format, and it's just rude to change that,
especially when you do so incorrectly.

-uwe


Re: CVS commit: src/sys/dev/usb

2020-03-13 Thread Christos Zoulas

> This was not a part of the PR and is completely cosmetic (surely it
> supports plain %x if it does support %#x).  Why was this necessary?
> (I know I would be quite miffed if someone made a change like that to
> my code).

Yes, that %x formatting change was not part of the PR, but I only changed 0x%x 
not plain %x.
I did it because as I was fixing the 0x%x in the log, I started changing them 
to %#jx so I did it
globally in that directory for consistency. It found two formats that were 
0x%hu...
 So one can view it as a format consistency checker(not just cosmetic).

christos



signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sys/dev/usb

2020-03-13 Thread Valery Ushakov
On Fri, Mar 13, 2020 at 17:09:14 -0700, Paul Goyette wrote:

> On Sat, 14 Mar 2020, Valery Ushakov wrote:
> 
> > On Fri, Mar 13, 2020 at 14:17:42 -0400, Christos Zoulas wrote:
> > 
> > > Log Message:
> > > PR/55068: sc.dying: Fix printf formats:
> > [...]
> > > - 0x% -> %#
> > 
> > This was not a part of the PR and is completely cosmetic (surely it
> > supports plain %x if it does support %#x).  Why was this necessary?
> > (I know I would be quite miffed if someone made a change like that to
> > my code).
> 
> Plain %x - no  :(
> 
> In order to enable sysctl-transport to userland, all the args need to
> be promoted to %jx, and the format strings need to ensure that they
> consume that size.

Random sample from the diff:

-   printf("%s: expect 0xe6!! (0x%x)\n", device_xname(sc->sc_dev),
+   printf("%s: expect 0xe6!! (%#x)\n", device_xname(sc->sc_dev),


Actually, looking close I see diffs like 

-   DPRINTFN(MD_ROOT, "wValue=0x%04jx", value, 0, 0, 0);
+   DPRINTFN(MD_ROOT, "wValue=%#04jx", value, 0, 0, 0);

that are plain wrong as %#x counts the 0x prefix towards the field
width.

$ printf '0x%02x %#02x\n' 1 1
0x01 0x1
$ printf '0x%08x 0x%08x\n%#08x %#08x\n' 0 1 0 1
0x 0x0001
 0x01


So, as far as I can tell, these %# changes don't fix all the argument
size issues, break some output formatting and violate preference of
the original author.  Did I miss something else?

-uwe


Re: CVS commit: src/sys/dev/usb

2020-03-13 Thread Paul Goyette

On Sat, 14 Mar 2020, Valery Ushakov wrote:


On Fri, Mar 13, 2020 at 14:17:42 -0400, Christos Zoulas wrote:


Log Message:
PR/55068: sc.dying: Fix printf formats:

[...]

- 0x% -> %#


This was not a part of the PR and is completely cosmetic (surely it
supports plain %x if it does support %#x).  Why was this necessary?
(I know I would be quite miffed if someone made a change like that to
my code).


Plain %x - no  :(

In order to enable sysctl-transport to userland, all the args need to
be promoted to %jx, and the format strings need to ensure that they
consume that size.


++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+


Re: CVS commit: src/sys/dev/usb

2020-03-13 Thread Valery Ushakov
On Fri, Mar 13, 2020 at 14:17:42 -0400, Christos Zoulas wrote:

> Log Message:
> PR/55068: sc.dying: Fix printf formats:
[...]
> - 0x% -> %#

This was not a part of the PR and is completely cosmetic (surely it
supports plain %x if it does support %#x).  Why was this necessary?
(I know I would be quite miffed if someone made a change like that to
my code).

-uwe


Re: CVS commit: src/sys/arch/mips/mips

2020-03-13 Thread Christos Zoulas

> On Mar 13, 2020, at 12:25 PM, Jason Thorpe  wrote:
> 
> 
>> On Mar 13, 2020, at 9:11 AM, Christos Zoulas  wrote:
>> 
>> I think this is better done in the driver, as other ports
>> do the same check and it catches bugs.
> 
> x86 *explcitly* checks for 0 to skip work.  If you want to find bugs, change 
> the most-often-used implementation maybe?
> 
> -- thorpej

I remembered wrong then.

christos



signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sys/arch/mips/mips

2020-03-13 Thread Jason Thorpe


> On Mar 13, 2020, at 9:11 AM, Christos Zoulas  wrote:
> 
> I think this is better done in the driver, as other ports
> do the same check and it catches bugs.

x86 *explcitly* checks for 0 to skip work.  If you want to find bugs, change 
the most-often-used implementation maybe?

-- thorpej



Re: CVS commit: src/sys/arch/mips/mips

2020-03-13 Thread Christos Zoulas
In article <20200313034939.553d5f...@cvs.netbsd.org>,
Jason R Thorpe  wrote:
>-=-=-=-=-=-
>
>Module Name:   src
>Committed By:  thorpej
>Date:  Fri Mar 13 03:49:39 UTC 2020
>
>Modified Files:
>   src/sys/arch/mips/mips: bus_dma.c
>
>Log Message:
>Allow len == 0 in bus_dmamap_sync().

I think this is better done in the driver, as other ports
do the same check and it catches bugs.

christos



Re: CVS commit: src/lib/libcurses

2020-03-13 Thread Christos Zoulas
Thanks for explaining!

christos




signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/lib/libcurses

2020-03-13 Thread Martin Husemann
On Fri, Mar 13, 2020 at 04:09:25PM -, Christos Zoulas wrote:
> Sorry I don't understand this change? How is that different than using
> 
>   err(EXIT_FAILURE, "initscr");

For crunched install media the older variant seems to pull in lots of libc
(libhack has a simple perror but no err/errx) and all the locale support.

Martin


Re: CVS commit: src/lib/libcurses

2020-03-13 Thread Christos Zoulas
In article <20200312155012.1ce50f...@cvs.netbsd.org>,
Roy Marples  wrote:
>-=-=-=-=-=-
>
>Module Name:   src
>Committed By:  roy
>Date:  Thu Mar 12 15:50:12 UTC 2020
>
>Modified Files:
>   src/lib/libcurses: initscr.c
>
>Log Message:
>curses: use perror rather than err in initscr
>
>Index: src/lib/libcurses/initscr.c
>diff -u src/lib/libcurses/initscr.c:1.34 src/lib/libcurses/initscr.c:1.35
>--- src/lib/libcurses/initscr.c:1.34   Wed Mar 11 21:33:38 2020
>+++ src/lib/libcurses/initscr.cThu Mar 12 15:50:11 2020
>@@ -1,4 +1,4 @@
>-/*$NetBSD: initscr.c,v 1.34 2020/03/11 21:33:38 roy Exp $ */
>+/*$NetBSD: initscr.c,v 1.35 2020/03/12 15:50:11 roy Exp $ */
> 
> /*
>  * Copyright (c) 1981, 1993, 1994
>@@ -34,11 +34,10 @@
> #if 0
> static char sccsid[] = "@(#)initscr.c 8.2 (Berkeley) 5/4/94";
> #else
>-__RCSID("$NetBSD: initscr.c,v 1.34 2020/03/11 21:33:38 roy Exp $");
>+__RCSID("$NetBSD: initscr.c,v 1.35 2020/03/12 15:50:11 roy Exp $");
> #endif
> #endif/* not lint */
> 
>-#include 
> #include 
> 
> #include "curses.h"
>@@ -66,8 +65,15 @@ initscr(void)
>   sp = Def_term;
> 
>   /* LINTED const castaway; newterm does not modify sp! */
>-  if ((_cursesi_screen = newterm((char *) sp, stdout, stdin)) == NULL)
>-  errx(EXIT_FAILURE, "initscr"); /* POSIX says exit on failure */
>+  if ((_cursesi_screen = newterm((char *) sp, stdout, stdin)) == NULL) {
>+  /*
>+   * POSIX says we should write a diagnostic and exit on error.
>+   * As such some applications don't bother checking the return
>+   * value at all.
>+   */
>+  perror("initscr");
>+  exit(EXIT_FAILURE);
>+  }
> 
>   set_term(_cursesi_screen);
>   wrefresh(curscr);
>

Sorry I don't understand this change? How is that different than using

err(EXIT_FAILURE, "initscr");

christos



Re: CVS commit: src/external/cddl/osnet/dist/uts/common/fs/zfs

2020-03-13 Thread Warner Losh
On Wed, Mar 11, 2020 at 12:13 AM J. Hannken-Illjes 
wrote:

> On 10. Mar 2020, at 13:37, Santhosh Raju  wrote:
>
> On Tue, Mar 10, 2020 at 7:25 AM Santhosh Raju  wrote:
>
>
> Module Name:src
> Committed By:   fox
> Date:   Mon Mar  9 15:40:50 UTC 2020
>
> Modified Files:
>src/external/cddl/osnet/dist/uts/common/fs/zfs: metaslab.c
>
> Log Message:
> external/cddl/osnet: Fix possible null pointer access.
>
> Detected by UBSan and fixed upstream, pick only the fix from the commit.
>
> Cherry-pick:
> From 928e8ad47d3478a3d5d01f0dd6ae74a9371af65e Mon Sep 17 00:00:00 2001
> From: Serapheim Dimitropoulos 
> Date: Wed, 20 Feb 2019 09:59:57 -0800
> Subject: [PATCH] Introduce auxiliary metaslab histograms
>
> Reviewed by: Paul Dagnelie 
> Reviewed-by: Brian Behlendorf 
> Reviewed by: Matt Ahrens 
> Signed-off-by: Serapheim Dimitropoulos 
> Closes #8358
>
> Reviewed by: kamil@
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.1.1.3 -r1.2 \
>src/external/cddl/osnet/dist/uts/common/fs/zfs/metaslab.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
>
> Forgot to add in the commit log, the changes have been pulled in from
> upstream openzfs.
>
>
> https://github.com/openzfs/zfs/commit/928e8ad47d3478a3d5d01f0dd6ae74a9371af65e#diff-9fd6b453f8153161abe0728c449e6505R4386
>
> --
> Santhosh
>
>
> This is NOT our upstream --
>

Regardless of NetBSD's "official" upstream, this is the new OpenZFS
upstream. It has moved off illumos earlier in the year to ZoL which was
restructured to be cross platform. It includes FreeBSD support, though that
flavor of ZFS hasn't been completely merged back into FreeBSD just yet
(there's work in progress to make that happen). Since it's the only real
upstream today, you may need to do some updates to cope with the new ZFS
playing field.

Warner


Re: CVS commit: src/sys/arch/mips/mips

2020-03-13 Thread Nick Hudson

On 13/03/2020 03:49, Jason R Thorpe wrote:

Module Name:src
Committed By:   thorpej
Date:   Fri Mar 13 03:49:39 UTC 2020

Modified Files:
src/sys/arch/mips/mips: bus_dma.c

Log Message:
Allow len == 0 in bus_dmamap_sync().

XXX pullup-9


The assertion that len is not 0 in arm bus_dma.c has found several bugs
over the years.

Just saying.

Nick