Re: mktemp: Clarify error message

[Theo de Raadt]

>On 12/25/17, Otto Moerbeek  wrote:
>> On Tue, Dec 26, 2017 at 12:31:02AM +0100, Klemens Nanni wrote:
>>
>>> On Mon, Dec 25, 2017 at 03:57:00PM -0700, Theo de Raadt wrote:
>>> > I think this is a silly solution, and the documentation is clear
>>> > enough.
>>> The manual page certainly is clear enough but the current error message
>>> is logically wrong, as there are sufficient Xs *in* `XXs' but just
>>> not at the end of it, call it nitpicking if you will.
>>>
>>> > How did this happen to you?  Show the place where it happened to you.
>>> > Would the text you propose actually have saved you 1 second of time
>>> > to help you realize what was wrong?  I don't think so.
>>> Just a typo really making me think "this could be clearer". So yes, I
>>> find telling this way actually saves time understanding the error, even
>>> if so little.
>>>
>>> > If you weren't familiar that the template has to be minimum 6 XX at
>>> > end of the string, then you hadn't achieved familiarity of the
>>> > subject matter yet.
>>> I agree that knowing one from the manual implies knowing the other as
>>> well, but it doesn't seem reason enough to keep the error message as is,
>>> hence the diff.
>>
>> I disagree. An error message does not need to document everything, An
>> erro message should short and clear enough together with the doumentation.
>
>all well and good, but let's not drop words and letters in pursuit of brevity.

this isn't a competition to see if your opinion matters

Re: mktemp: Clarify error message

[Theo de Raadt]

>On Mon, Dec 25, 2017 at 03:57:00PM -0700, Theo de Raadt wrote:
>> I think this is a silly solution, and the documentation is clear
>> enough.
>The manual page certainly is clear enough but the current error message
>is logically wrong, as there are sufficient Xs *in* `XXs' but just
>not at the end of it, call it nitpicking if you will.

Disagree.

It is logically correct according to the definition of a mktemp
template.  If you put X elsewhere it is not template material.

If the *programmer* didn't understand what a template was and screwed
it up, providing the *application user* with detailed information
isn't useful.  If the programmer is that user, the terse message is
enough to indicate "hey moron, go study"

And now, you want to come up with a 40 character snippet of text
which will inform the user of the software that the programmer
didn't follow the rules..

And let me guess, soon you'll find another fundamental concept easily
learned in the manual page, and want to sneak it into the error
message also?

Sorry, that's not how it works.  Short firm error messages
provide a strong hint that further study is needed.

Unix is a terse operating system.  It is considered a strength.

Re: mktemp: Clarify error message

[patrick keshishian]

On 12/25/17, Otto Moerbeek  wrote:
> On Tue, Dec 26, 2017 at 12:31:02AM +0100, Klemens Nanni wrote:
>
>> On Mon, Dec 25, 2017 at 03:57:00PM -0700, Theo de Raadt wrote:
>> > I think this is a silly solution, and the documentation is clear
>> > enough.
>> The manual page certainly is clear enough but the current error message
>> is logically wrong, as there are sufficient Xs *in* `XXs' but just
>> not at the end of it, call it nitpicking if you will.
>>
>> > How did this happen to you?  Show the place where it happened to you.
>> > Would the text you propose actually have saved you 1 second of time
>> > to help you realize what was wrong?  I don't think so.
>> Just a typo really making me think "this could be clearer". So yes, I
>> find telling this way actually saves time understanding the error, even
>> if so little.
>>
>> > If you weren't familiar that the template has to be minimum 6 XX at
>> > end of the string, then you hadn't achieved familiarity of the
>> > subject matter yet.
>> I agree that knowing one from the manual implies knowing the other as
>> well, but it doesn't seem reason enough to keep the error message as is,
>> hence the diff.
>
> I disagree. An error message does not need to document everything, An
> erro message should short and clear enough together with the doumentation.

all well and good, but let's not drop words and letters in pursuit of brevity.

-pk

> This reminds me of the old IRIX compiler, that would cite complete
> parapgraphs of the C standard in error mesasges. Of course logically
> it was all correct, but it lead to long and unreadable error messages
> that filled up disks with build logs.
>
>   -Otto
>
>
>
>

Re: mktemp: Clarify error message

[Otto Moerbeek]

On Tue, Dec 26, 2017 at 12:31:02AM +0100, Klemens Nanni wrote:

> On Mon, Dec 25, 2017 at 03:57:00PM -0700, Theo de Raadt wrote:
> > I think this is a silly solution, and the documentation is clear
> > enough.
> The manual page certainly is clear enough but the current error message
> is logically wrong, as there are sufficient Xs *in* `XXs' but just
> not at the end of it, call it nitpicking if you will.
> 
> > How did this happen to you?  Show the place where it happened to you.
> > Would the text you propose actually have saved you 1 second of time
> > to help you realize what was wrong?  I don't think so.
> Just a typo really making me think "this could be clearer". So yes, I
> find telling this way actually saves time understanding the error, even
> if so little.
> 
> > If you weren't familiar that the template has to be minimum 6 XX at
> > end of the string, then you hadn't achieved familiarity of the
> > subject matter yet.
> I agree that knowing one from the manual implies knowing the other as
> well, but it doesn't seem reason enough to keep the error message as is,
> hence the diff.

I disagree. An error message does not need to document everything, An
erro message should short and clear enough together with the doumentation.

This reminds me of the old IRIX compiler, that would cite complete
parapgraphs of the C standard in error mesasges. Of course logically
it was all correct, but it lead to long and unreadable error messages
that filled up disks with build logs.

-Otto

Re: armv7 / A33 clock and pinctrl bindings

[Jonathan Gray]

On Tue, Dec 26, 2017 at 01:15:35PM +0800, Kevin Lo wrote:
> The diff below adds support for the "next-generation" clock and pinctrl
> bindings for the Allwinner A33.
> 
> Tested on my Banana Pi M2 Magic.  dmesg: http://ix.io/DoB
> ok?
> 
> Index: sys/dev/fdt/ehci_fdt.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/ehci_fdt.c,v
> retrieving revision 1.2
> diff -u -p -u -p -r1.2 ehci_fdt.c
> --- sys/dev/fdt/ehci_fdt.c17 Dec 2017 13:23:03 -  1.2
> +++ sys/dev/fdt/ehci_fdt.c25 Dec 2017 09:43:48 -
> @@ -273,9 +273,10 @@ sun4i_phy_init(struct ehci_fdt_softc *sc
>  
>   /*
>* We need to poke an undocumented register to make the PHY
> -  * work on Allwinner H3/H5/A64.
> +  * work on Allwinner H3/H5/A33/A64.
>*/
> - if (OF_is_compatible(node, "allwinner,sun8i-h3-usb-phy") ||
> + if (OF_is_compatible(node, "allwinner,sun8i-a33-usb-phy") ||
> + OF_is_compatible(node, "allwinner,sun8i-h3-usb-phy") ||
>   OF_is_compatible(node, "allwinner,sun50i-a64-usb-phy")) {
>   val = bus_space_read_4(sc->sc.iot, sc->sc.ioh, 0x810);
>   val &= ~(1 << 1);

Is this actually required?  U-Boot only does it for h3/h5.

If so should "allwinner,sun8i-a23-usb-phy" and
"allwinner,sun8i-v3s-usb-phy" also be added?

> Index: sys/dev/fdt/sxiccmu.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/sxiccmu.c,v
> retrieving revision 1.12
> diff -u -p -u -p -r1.12 sxiccmu.c
> --- sys/dev/fdt/sxiccmu.c 24 Dec 2017 18:24:06 -  1.12
> +++ sys/dev/fdt/sxiccmu.c 25 Dec 2017 09:43:48 -
> @@ -88,6 +88,8 @@ voidsxiccmu_ccu_reset(void *, uint32_t 
>  
>  uint32_t sxiccmu_a10_get_frequency(struct sxiccmu_softc *, uint32_t);
>  int  sxiccmu_a10_set_frequency(struct sxiccmu_softc *, uint32_t, uint32_t);
> +uint32_t sxiccmu_a23_get_frequency(struct sxiccmu_softc *, uint32_t);
> +int  sxiccmu_a23_set_frequency(struct sxiccmu_softc *, uint32_t, uint32_t);
>  uint32_t sxiccmu_a64_get_frequency(struct sxiccmu_softc *, uint32_t);
>  int  sxiccmu_a64_set_frequency(struct sxiccmu_softc *, uint32_t, uint32_t);
>  uint32_t sxiccmu_a80_get_frequency(struct sxiccmu_softc *, uint32_t);
> @@ -110,6 +112,8 @@ sxiccmu_match(struct device *parent, voi
>   OF_is_compatible(node, "allwinner,sun5i-a10s") ||
>   OF_is_compatible(node, "allwinner,sun5i-r8") ||
>   OF_is_compatible(node, "allwinner,sun7i-a20") ||
> + OF_is_compatible(node, "allwinner,sun8i-a23") ||
> + OF_is_compatible(node, "allwinner,sun8i-a33") ||
>   OF_is_compatible(node, "allwinner,sun8i-h3") ||
>   OF_is_compatible(node, "allwinner,sun9i-a80") ||
>   OF_is_compatible(node, "allwinner,sun50i-a64") ||
> @@ -118,6 +122,8 @@ sxiccmu_match(struct device *parent, voi
>  
>   return (OF_is_compatible(node, "allwinner,sun4i-a10-ccu") ||
>   OF_is_compatible(node, "allwinner,sun7i-a20-ccu") ||
> + OF_is_compatible(node, "allwinner,sun8i-a23-ccu") ||
> + OF_is_compatible(node, "allwinner,sun8i-a33-ccu") ||
>   OF_is_compatible(node, "allwinner,sun8i-h3-ccu") ||
>   OF_is_compatible(node, "allwinner,sun9i-a80-ccu") ||
>   OF_is_compatible(node, "allwinner,sun9i-a80-usb-clks") ||
> @@ -150,6 +156,15 @@ sxiccmu_attach(struct device *parent, st
>   sc->sc_nresets = nitems(sun4i_a10_resets);
>   sc->sc_get_frequency = sxiccmu_a10_get_frequency;
>   sc->sc_set_frequency = sxiccmu_a10_set_frequency;
> + } else if (OF_is_compatible(node, "allwinner,sun8i-a23-ccu") ||
> + OF_is_compatible(node, "allwinner,sun8i-a33-ccu")) {
> + KASSERT(faa->fa_nreg > 0);
> + sc->sc_gates = sun8i_a23_gates;
> + sc->sc_ngates = nitems(sun8i_a23_gates);
> + sc->sc_resets = sun8i_a23_resets;
> + sc->sc_nresets = nitems(sun8i_a23_resets);
> + sc->sc_get_frequency = sxiccmu_a23_get_frequency;
> + sc->sc_set_frequency = sxiccmu_a23_set_frequency;
>   } else if (OF_is_compatible(node, "allwinner,sun8i-h3-ccu") ||
>   OF_is_compatible(node, "allwinner,sun50i-h5-ccu")) {
>   KASSERT(faa->fa_nreg > 0);
> @@ -357,6 +372,32 @@ struct sxiccmu_device sxiccmu_devices[] 
>   .get_frequency = sxiccmu_apbs_get_frequency
>   },
>   {
> + .compat = "allwinner,sun8i-a23-ahb1-gates-clk",
> + .get_frequency = sxiccmu_gen_get_frequency,
> + .enable = sxiccmu_gate_enable
> + },
> + {
> + .compat = "allwinner,sun8i-a23-apb0-gates-clk",
> + .get_frequency = sxiccmu_gen_get_frequency,
> + .enable = sxiccmu_gate_enable
> + },
> + {
> + .compat = "allwinner,sun8i-a23-apb1-gates-clk",
> +  

armv7 / A33 clock and pinctrl bindings

[Kevin Lo]

The diff below adds support for the "next-generation" clock and pinctrl
bindings for the Allwinner A33.

Tested on my Banana Pi M2 Magic.  dmesg: http://ix.io/DoB
ok?

Index: sys/dev/fdt/ehci_fdt.c
===
RCS file: /cvs/src/sys/dev/fdt/ehci_fdt.c,v
retrieving revision 1.2
diff -u -p -u -p -r1.2 ehci_fdt.c
--- sys/dev/fdt/ehci_fdt.c  17 Dec 2017 13:23:03 -  1.2
+++ sys/dev/fdt/ehci_fdt.c  25 Dec 2017 09:43:48 -
@@ -273,9 +273,10 @@ sun4i_phy_init(struct ehci_fdt_softc *sc
 
/*
 * We need to poke an undocumented register to make the PHY
-* work on Allwinner H3/H5/A64.
+* work on Allwinner H3/H5/A33/A64.
 */
-   if (OF_is_compatible(node, "allwinner,sun8i-h3-usb-phy") ||
+   if (OF_is_compatible(node, "allwinner,sun8i-a33-usb-phy") ||
+   OF_is_compatible(node, "allwinner,sun8i-h3-usb-phy") ||
OF_is_compatible(node, "allwinner,sun50i-a64-usb-phy")) {
val = bus_space_read_4(sc->sc.iot, sc->sc.ioh, 0x810);
val &= ~(1 << 1);
Index: sys/dev/fdt/sxiccmu.c
===
RCS file: /cvs/src/sys/dev/fdt/sxiccmu.c,v
retrieving revision 1.12
diff -u -p -u -p -r1.12 sxiccmu.c
--- sys/dev/fdt/sxiccmu.c   24 Dec 2017 18:24:06 -  1.12
+++ sys/dev/fdt/sxiccmu.c   25 Dec 2017 09:43:48 -
@@ -88,6 +88,8 @@ void  sxiccmu_ccu_reset(void *, uint32_t 
 
 uint32_t sxiccmu_a10_get_frequency(struct sxiccmu_softc *, uint32_t);
 intsxiccmu_a10_set_frequency(struct sxiccmu_softc *, uint32_t, uint32_t);
+uint32_t sxiccmu_a23_get_frequency(struct sxiccmu_softc *, uint32_t);
+intsxiccmu_a23_set_frequency(struct sxiccmu_softc *, uint32_t, uint32_t);
 uint32_t sxiccmu_a64_get_frequency(struct sxiccmu_softc *, uint32_t);
 intsxiccmu_a64_set_frequency(struct sxiccmu_softc *, uint32_t, uint32_t);
 uint32_t sxiccmu_a80_get_frequency(struct sxiccmu_softc *, uint32_t);
@@ -110,6 +112,8 @@ sxiccmu_match(struct device *parent, voi
OF_is_compatible(node, "allwinner,sun5i-a10s") ||
OF_is_compatible(node, "allwinner,sun5i-r8") ||
OF_is_compatible(node, "allwinner,sun7i-a20") ||
+   OF_is_compatible(node, "allwinner,sun8i-a23") ||
+   OF_is_compatible(node, "allwinner,sun8i-a33") ||
OF_is_compatible(node, "allwinner,sun8i-h3") ||
OF_is_compatible(node, "allwinner,sun9i-a80") ||
OF_is_compatible(node, "allwinner,sun50i-a64") ||
@@ -118,6 +122,8 @@ sxiccmu_match(struct device *parent, voi
 
return (OF_is_compatible(node, "allwinner,sun4i-a10-ccu") ||
OF_is_compatible(node, "allwinner,sun7i-a20-ccu") ||
+   OF_is_compatible(node, "allwinner,sun8i-a23-ccu") ||
+   OF_is_compatible(node, "allwinner,sun8i-a33-ccu") ||
OF_is_compatible(node, "allwinner,sun8i-h3-ccu") ||
OF_is_compatible(node, "allwinner,sun9i-a80-ccu") ||
OF_is_compatible(node, "allwinner,sun9i-a80-usb-clks") ||
@@ -150,6 +156,15 @@ sxiccmu_attach(struct device *parent, st
sc->sc_nresets = nitems(sun4i_a10_resets);
sc->sc_get_frequency = sxiccmu_a10_get_frequency;
sc->sc_set_frequency = sxiccmu_a10_set_frequency;
+   } else if (OF_is_compatible(node, "allwinner,sun8i-a23-ccu") ||
+   OF_is_compatible(node, "allwinner,sun8i-a33-ccu")) {
+   KASSERT(faa->fa_nreg > 0);
+   sc->sc_gates = sun8i_a23_gates;
+   sc->sc_ngates = nitems(sun8i_a23_gates);
+   sc->sc_resets = sun8i_a23_resets;
+   sc->sc_nresets = nitems(sun8i_a23_resets);
+   sc->sc_get_frequency = sxiccmu_a23_get_frequency;
+   sc->sc_set_frequency = sxiccmu_a23_set_frequency;
} else if (OF_is_compatible(node, "allwinner,sun8i-h3-ccu") ||
OF_is_compatible(node, "allwinner,sun50i-h5-ccu")) {
KASSERT(faa->fa_nreg > 0);
@@ -357,6 +372,32 @@ struct sxiccmu_device sxiccmu_devices[] 
.get_frequency = sxiccmu_apbs_get_frequency
},
{
+   .compat = "allwinner,sun8i-a23-ahb1-gates-clk",
+   .get_frequency = sxiccmu_gen_get_frequency,
+   .enable = sxiccmu_gate_enable
+   },
+   {
+   .compat = "allwinner,sun8i-a23-apb0-gates-clk",
+   .get_frequency = sxiccmu_gen_get_frequency,
+   .enable = sxiccmu_gate_enable
+   },
+   {
+   .compat = "allwinner,sun8i-a23-apb1-gates-clk",
+   .get_frequency = sxiccmu_gen_get_frequency,
+   .enable = sxiccmu_gate_enable
+   },
+   {
+   .compat = "allwinner,sun8i-a23-apb2-gates-clk",
+   .get_frequency = sxiccmu_gen_get_frequency,
+   .enable = 

Re: mktemp: Clarify error message

[Klemens Nanni]

On Mon, Dec 25, 2017 at 03:57:00PM -0700, Theo de Raadt wrote:
> I think this is a silly solution, and the documentation is clear
> enough.
The manual page certainly is clear enough but the current error message
is logically wrong, as there are sufficient Xs *in* `XXs' but just
not at the end of it, call it nitpicking if you will.

> How did this happen to you?  Show the place where it happened to you.
> Would the text you propose actually have saved you 1 second of time
> to help you realize what was wrong?  I don't think so.
Just a typo really making me think "this could be clearer". So yes, I
find telling this way actually saves time understanding the error, even
if so little.

> If you weren't familiar that the template has to be minimum 6 XX at
> end of the string, then you hadn't achieved familiarity of the
> subject matter yet.
I agree that knowing one from the manual implies knowing the other as
well, but it doesn't seem reason enough to keep the error message as is,
hence the diff.

Re: mktemp: Clarify error message

[Theo de Raadt]

I think this is a silly solution, and the documentation is clear
enough.

How did this happen to you?  Show the place where it happened to you.
Would the text you propose actually have saved you 1 second of time
to help you realize what was wrong?  I don't think so.  If you
weren't familiar that the template has to be minimum 6 XX at
end of the string, then you hadn't achieved familiarity of the
subject matter yet.

>On Mon, Dec 25, 2017 at 08:36:07PM +, Stuart Henderson wrote:
>> On 2017/12/25 20:52, Klemens Nanni wrote:
>> > from mktemp(1):
>> > 
>> >The template may be any filename with at least six ‘Xs’ appended
>> >to it, for example /tmp/tfile.XX.
>> > 
>> > Now when a template contains but does not end in six Xs, the error
>> > message may imply errornous behaviour instead of bad usage:
>> > 
>> >$ mktemp XX
>> >oAQnQ5
>> >$ mktemp XXs
>> >mktemp: insufficient number of Xs in template `XXs'
>> > 
>> > I'd like to see a more precise error message here.
>> > 
>> > Feedback?
>> > 
>> > diff --git a/usr.bin/mktemp/mktemp.c b/usr.bin/mktemp/mktemp.c
>> > index 713b67fd105..c080d1d6474 100644
>> > --- a/usr.bin/mktemp/mktemp.c
>> > +++ b/usr.bin/mktemp/mktemp.c
>> > @@ -77,10 +77,9 @@ main(int argc, char *argv[])
>> >}
>> >  
>> >len = strlen(template);
>> > -  if (len < 6 || strcmp([len - 6], "XX")) {
>> > -  fatalx("insufficient number of Xs in template `%s'",
>> > -  template);
>> > -  }
>> > +  if (len < 6 || strcmp([len - 6], "XX"))
>> > +  fatalx("template must end in six or more Xs");
>> > +
>> >if (tflag) {
>> >if (strchr(template, '/')) {
>> >fatalx("template must not contain directory "
>> > 
>> 
>> Printing the actual template used makes it easier to track down
>> the problematic call.
>Fair enough, how about this?
>
>diff --git a/usr.bin/mktemp/mktemp.c b/usr.bin/mktemp/mktemp.c
>index 713b67fd105..96b6731ca90 100644
>--- a/usr.bin/mktemp/mktemp.c
>+++ b/usr.bin/mktemp/mktemp.c
>@@ -78,7 +78,7 @@ main(int argc, char *argv[])
> 
>   len = strlen(template);
>   if (len < 6 || strcmp([len - 6], "XX")) {
>-  fatalx("insufficient number of Xs in template `%s'",
>+  fatalx("insufficient number of Xs at end of template `%s'",
>   template);
>   }
>   if (tflag) {
>
>

ifstated.conf.5 try to cover a use-case w/android phone

[Artturi Alm]

Hi,

in my use-case urndis is likely never there at boot, and while i've
been aware of ifstated(8) all the time, the relevant man pages have
felt dry w/regards giving something trivial/!carp, to make me skip
setting this up again for far too long:)

/etc/examples/ifstated.conf also supported my previous view,
that this wasn't the tool i was looking for.

english, and all that + manpage diff, so i'll show everything necessary
for the simple use-case, instead of trying to add bits here and there..

originally i intended to just make it clearer that the "For example:"
under "STATE DEFINITIONS" was not about:
"It is also possible to write multiple nested if blocks.",
and iirc. in my quick testing i found out i actually needed the nesting
below in the auto state, to allow auto->running when starting ifstated
while urndis0 is already set up

-Artturi


diff --git a/usr.sbin/ifstated/ifstated.conf.5 
b/usr.sbin/ifstated/ifstated.conf.5
index 2721871926c..554ea043c5f 100644
--- a/usr.sbin/ifstated/ifstated.conf.5
+++ b/usr.sbin/ifstated/ifstated.conf.5
@@ -136,20 +136,49 @@ blocks.
 .Pp
 For example:
 .Bd -literal -offset indent
-state one {
+urndis_up = "urndis0.link.up"
+_avail_fast = '( "ifconfig urndis0 -debug 2> /dev/null" every 5)'
+_avail_init = '( "ifconfig urndis0 -debug 2> /dev/null" every 15)'
+_avail_slow = '( "ifconfig urndis0 -debug 2> /dev/null" every 30)'
+
+# The ip address below is the address of the android peer
+peer = '( "ping -q -c 1 -w 1 192.168.42.129 > /dev/null" every 5)'
+
+state auto {
+   if $urndis_up
+   set-state running
+
+   if ! $urndis_up
+   if $_avail_init
+   set-state bringup
+}
+
+state absent {
+   if $_avail_fast
+   set-state bringup
+}
+
+state bringup {
init {
-   run "ifconfig carp0 advskew 10"
-   run "ifconfig carp1 advskew 10"
+   run "ksh /etc/netstart urndis0"
+   run "sleep 2"
}
+   if $peer
+   set-state running
+}
 
-   if ! $net
-   set-state two
+state lost {
+   if $urndis_up && $peer
+   set-state running
+   if ! $_avail_fast
+   set-state absent
+}
 
-   if ! $carp_up {
-   run "ifconfig carp0 advskew 254"
-   run "ifconfig carp1 advskew 254"
-   set-state three
-   }
+state running {
+   if ! $urndis_up
+   set-state lost
+   if ! $_avail_slow
+   set-state absent
 }
 .Ed
 .Sh GRAMMAR

Re: mktemp: Clarify error message

[Klemens Nanni]

On Mon, Dec 25, 2017 at 08:36:07PM +, Stuart Henderson wrote:
> On 2017/12/25 20:52, Klemens Nanni wrote:
> > from mktemp(1):
> > 
> > The template may be any filename with at least six ‘Xs’ appended
> > to it, for example /tmp/tfile.XX.
> > 
> > Now when a template contains but does not end in six Xs, the error
> > message may imply errornous behaviour instead of bad usage:
> > 
> > $ mktemp XX
> > oAQnQ5
> > $ mktemp XXs
> > mktemp: insufficient number of Xs in template `XXs'
> > 
> > I'd like to see a more precise error message here.
> > 
> > Feedback?
> > 
> > diff --git a/usr.bin/mktemp/mktemp.c b/usr.bin/mktemp/mktemp.c
> > index 713b67fd105..c080d1d6474 100644
> > --- a/usr.bin/mktemp/mktemp.c
> > +++ b/usr.bin/mktemp/mktemp.c
> > @@ -77,10 +77,9 @@ main(int argc, char *argv[])
> > }
> >  
> > len = strlen(template);
> > -   if (len < 6 || strcmp([len - 6], "XX")) {
> > -   fatalx("insufficient number of Xs in template `%s'",
> > -   template);
> > -   }
> > +   if (len < 6 || strcmp([len - 6], "XX"))
> > +   fatalx("template must end in six or more Xs");
> > +
> > if (tflag) {
> > if (strchr(template, '/')) {
> > fatalx("template must not contain directory "
> > 
> 
> Printing the actual template used makes it easier to track down
> the problematic call.
Fair enough, how about this?

diff --git a/usr.bin/mktemp/mktemp.c b/usr.bin/mktemp/mktemp.c
index 713b67fd105..96b6731ca90 100644
--- a/usr.bin/mktemp/mktemp.c
+++ b/usr.bin/mktemp/mktemp.c
@@ -78,7 +78,7 @@ main(int argc, char *argv[])
 
len = strlen(template);
if (len < 6 || strcmp([len - 6], "XX")) {
-   fatalx("insufficient number of Xs in template `%s'",
+   fatalx("insufficient number of Xs at end of template `%s'",
template);
}
if (tflag) {

Re: mktemp: Clarify error message

[Stuart Henderson]

On 2017/12/25 20:52, Klemens Nanni wrote:
> from mktemp(1):
> 
>   The template may be any filename with at least six ‘Xs’ appended
>   to it, for example /tmp/tfile.XX.
> 
> Now when a template contains but does not end in six Xs, the error
> message may imply errornous behaviour instead of bad usage:
> 
>   $ mktemp XX
>   oAQnQ5
>   $ mktemp XXs
>   mktemp: insufficient number of Xs in template `XXs'
> 
> I'd like to see a more precise error message here.
> 
> Feedback?
> 
> diff --git a/usr.bin/mktemp/mktemp.c b/usr.bin/mktemp/mktemp.c
> index 713b67fd105..c080d1d6474 100644
> --- a/usr.bin/mktemp/mktemp.c
> +++ b/usr.bin/mktemp/mktemp.c
> @@ -77,10 +77,9 @@ main(int argc, char *argv[])
>   }
>  
>   len = strlen(template);
> - if (len < 6 || strcmp([len - 6], "XX")) {
> - fatalx("insufficient number of Xs in template `%s'",
> - template);
> - }
> + if (len < 6 || strcmp([len - 6], "XX"))
> + fatalx("template must end in six or more Xs");
> +
>   if (tflag) {
>   if (strchr(template, '/')) {
>   fatalx("template must not contain directory "
> 

Printing the actual template used makes it easier to track down
the problematic call.

mktemp: Clarify error message

[Klemens Nanni]

from mktemp(1):

The template may be any filename with at least six ‘Xs’ appended
to it, for example /tmp/tfile.XX.

Now when a template contains but does not end in six Xs, the error
message may imply errornous behaviour instead of bad usage:

$ mktemp XX
oAQnQ5
$ mktemp XXs
mktemp: insufficient number of Xs in template `XXs'

I'd like to see a more precise error message here.

Feedback?

diff --git a/usr.bin/mktemp/mktemp.c b/usr.bin/mktemp/mktemp.c
index 713b67fd105..c080d1d6474 100644
--- a/usr.bin/mktemp/mktemp.c
+++ b/usr.bin/mktemp/mktemp.c
@@ -77,10 +77,9 @@ main(int argc, char *argv[])
}
 
len = strlen(template);
-   if (len < 6 || strcmp([len - 6], "XX")) {
-   fatalx("insufficient number of Xs in template `%s'",
-   template);
-   }
+   if (len < 6 || strcmp([len - 6], "XX"))
+   fatalx("template must end in six or more Xs");
+
if (tflag) {
if (strchr(template, '/')) {
fatalx("template must not contain directory "

Re: Add "-c command" option to script(1)

[Paul de Weerd]

Hi all,

Sorry to keep harping on this script stuff, but I'd really like to see
this committed.  I've just upgraded my laptop while doing some
vlan-bridging debugging and suddenly script(1) lost my new favorite
feature.

The manpage bits are OK jmc@; job@ and ian@ (off-list) OK'd the diff.
Is anyone willing to commit it with those?

Thanks,

Paul

On Sat, Dec 16, 2017 at 09:45:02AM +0100, Paul de Weerd wrote:
| On Fri, Dec 15, 2017 at 12:24:45PM +0100, Paul de Weerd wrote:
| | I've updated the diff to add this example as per jmc's suggestion.  It
| | now has:
| | 
| | - add the `-c command` feature
| | - updates usage
| | - removes /* ARGSUSED */ lint comments
| | - documents the -c feature
| | - adds an example to the manpage
| 
| jmc@ pointed out a missing colon at the end of the example.  Apologies
| for the extra noise.  Updated diff (still covers the above five
| changes) included.
| 
| Cheers,
| 
| Paul
| 
| 
| Index: script.1
| ===
| RCS file: /cvs/src/usr.bin/script/script.1,v
| retrieving revision 1.14
| diff -u -p -r1.14 script.1
| --- script.1  15 Jan 2012 20:06:40 -  1.14
| +++ script.1  16 Dec 2017 08:42:24 -
| @@ -39,6 +39,7 @@
|  .Sh SYNOPSIS
|  .Nm script
|  .Op Fl a
| +.Op Fl c Ar command
|  .Op Ar file
|  .Sh DESCRIPTION
|  .Nm
| @@ -65,9 +66,14 @@ Append the output to
|  or
|  .Pa typescript ,
|  retaining the prior contents.
| +.It Fl c Ar command
| +Run
| +.Ar command
| +instead of an interactive shell.
| +To run a command with arguments, enclose both in quotes.
|  .El
|  .Pp
| -The script ends when the forked shell exits (a control-D
| +The script ends when the forked program exits (a control-D
|  .Pq Ql ^D
|  to exit
|  the Bourne shell
| @@ -102,6 +108,11 @@ Name of the shell to be forked by
|  If not set, the Bourne shell is assumed.
|  (Most shells set this variable automatically.)
|  .El
| +.Sh EXAMPLES
| +Start a virtual machine and log all console output to a file:
| +.Bd -literal -offset indent
| +$ script -c "vmctl start myvm -c" myvm.typescript
| +.Ed
|  .Sh HISTORY
|  A predecessor called
|  .Nm dribble
| Index: script.c
| ===
| RCS file: /cvs/src/usr.bin/script/script.c,v
| retrieving revision 1.33
| diff -u -p -r1.33 script.c
| --- script.c  12 Apr 2017 14:49:05 -  1.33
| +++ script.c  14 Dec 2017 07:34:10 -
| @@ -89,7 +89,7 @@ int istty;
|  
|  __dead void done(int);
|  void dooutput(void);
| -void doshell(void);
| +void doshell(char *);
|  void fail(void);
|  void finish(int);
|  void scriptflush(int);
| @@ -102,17 +102,23 @@ main(int argc, char *argv[])
|   struct sigaction sa;
|   struct winsize win;
|   char ibuf[BUFSIZ];
| + char *cmd;
|   ssize_t cc, off;
|   int aflg, ch;
|  
| + cmd = NULL;
|   aflg = 0;
| - while ((ch = getopt(argc, argv, "a")) != -1)
| + while ((ch = getopt(argc, argv, "ac:")) != -1)
|   switch(ch) {
|   case 'a':
|   aflg = 1;
|   break;
| + case 'c':
| + cmd = optarg;
| + break;
|   default:
| - fprintf(stderr, "usage: %s [-a] [file]\n", __progname);
| + fprintf(stderr, "usage: %s [-a] [-c command] [file]\n",
| + __progname);
|   exit(1);
|   }
|   argc -= optind;
| @@ -163,7 +169,7 @@ main(int argc, char *argv[])
|   if (child)
|   dooutput();
|   else
| - doshell();
| + doshell(cmd);
|   }
|  
|   bzero(, sizeof sa);
| @@ -196,7 +202,6 @@ main(int argc, char *argv[])
|   done(sigdeadstatus);
|  }
|  
| -/* ARGSUSED */
|  void
|  finish(int signo)
|  {
| @@ -215,7 +220,6 @@ finish(int signo)
|   errno = save_errno;
|  }
|  
| -/* ARGSUSED */
|  void
|  handlesigwinch(int signo)
|  {
| @@ -294,7 +298,6 @@ dooutput(void)
|   done(0);
|  }
|  
| -/* ARGSUSED */
|  void
|  scriptflush(int signo)
|  {
| @@ -302,9 +305,10 @@ scriptflush(int signo)
|  }
|  
|  void
| -doshell(void)
| +doshell(char *cmd)
|  {
|   char *shell;
| + char *argp[] = {"sh", "-c", NULL, NULL};
|  
|   shell = getenv("SHELL");
|   if (shell == NULL)
| @@ -313,8 +317,15 @@ doshell(void)
|   (void)close(master);
|   (void)fclose(fscript);
|   login_tty(slave);
| - execl(shell, shell, "-i", (char *)NULL);
| - warn("%s", shell);
| +
| + if (cmd != NULL) {
| + argp[2] = cmd;
| + execv(_PATH_BSHELL, argp);
| + warn("unable to execute %s", _PATH_BSHELL);
| + } else {
| + execl(shell, shell, "-i", (char *)NULL);
| + warn("%s", shell);
| + }
|   fail();
|  }
|  
| 
| -- 
| >[<++>-]<+++.>+++[<-->-]<.>+++[<+