re: CVS commit: src/sys/dev/usb
> 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've come to agree that %# is dangerous in general to save one character. not only does it have the width issue you've mentioned, but it also emits "0" instead of "0x0" for the zero case, which i find surprising. christos, thanks for the backout. .mrg.
Re: CVS commit: src/sys/dev/usb
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
> 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
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
> 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
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
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
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
> 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
> 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
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
Thanks for explaining! christos signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/lib/libcurses
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
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
