mktemp: Clarify error message

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

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

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

        $ mktemp XXXXXX
        oAQnQ5
        $ mktemp XXXXXXs
        mktemp: insufficient number of Xs in template `XXXXXXs'

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(&template[len - 6], "XXXXXX")) {
- fatalx("insufficient number of Xs in template `%s'",
-    template);
- }
+ if (len < 6 || strcmp(&template[len - 6], "XXXXXX"))
+ fatalx("template must end in six or more Xs");
+
  if (tflag) {
  if (strchr(template, '/')) {
  fatalx("template must not contain directory "

Reply | Threaded
Open this post in threaded view
|

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.XXXXXXXXXX.
>
> Now when a template contains but does not end in six Xs, the error
> message may imply errornous behaviour instead of bad usage:
>
> $ mktemp XXXXXX
> oAQnQ5
> $ mktemp XXXXXXs
> mktemp: insufficient number of Xs in template `XXXXXXs'
>
> 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(&template[len - 6], "XXXXXX")) {
> - fatalx("insufficient number of Xs in template `%s'",
> -    template);
> - }
> + if (len < 6 || strcmp(&template[len - 6], "XXXXXX"))
> + 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.

Reply | Threaded
Open this post in threaded view
|

Re: mktemp: Clarify error message

Klemens Nanni
On Mon, Dec 25, 2017 at 08:36:07PM +0000, 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.XXXXXXXXXX.
> >
> > Now when a template contains but does not end in six Xs, the error
> > message may imply errornous behaviour instead of bad usage:
> >
> > $ mktemp XXXXXX
> > oAQnQ5
> > $ mktemp XXXXXXs
> > mktemp: insufficient number of Xs in template `XXXXXXs'
> >
> > 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(&template[len - 6], "XXXXXX")) {
> > - fatalx("insufficient number of Xs in template `%s'",
> > -    template);
> > - }
> > + if (len < 6 || strcmp(&template[len - 6], "XXXXXX"))
> > + 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(&template[len - 6], "XXXXXX")) {
- fatalx("insufficient number of Xs in template `%s'",
+ fatalx("insufficient number of Xs at end of template `%s'",
     template);
  }
  if (tflag) {

Reply | Threaded
Open this post in threaded view
|

Re: mktemp: Clarify error message

Theo de Raadt-2
In reply to this post by Klemens Nanni
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 XXXXXX 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 +0000, 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.XXXXXXXXXX.
>> >
>> > Now when a template contains but does not end in six Xs, the error
>> > message may imply errornous behaviour instead of bad usage:
>> >
>> > $ mktemp XXXXXX
>> > oAQnQ5
>> > $ mktemp XXXXXXs
>> > mktemp: insufficient number of Xs in template `XXXXXXs'
>> >
>> > 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(&template[len - 6], "XXXXXX")) {
>> > - fatalx("insufficient number of Xs in template `%s'",
>> > -    template);
>> > - }
>> > + if (len < 6 || strcmp(&template[len - 6], "XXXXXX"))
>> > + 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(&template[len - 6], "XXXXXX")) {
>- fatalx("insufficient number of Xs in template `%s'",
>+ fatalx("insufficient number of Xs at end of template `%s'",
>    template);
> }
> if (tflag) {
>
>

Reply | Threaded
Open this post in threaded view
|

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* `XXXXXXs' 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 XXXXXX 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.

Reply | Threaded
Open this post in threaded view
|

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* `XXXXXXs' 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 XXXXXX 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



Reply | Threaded
Open this post in threaded view
|

Re: mktemp: Clarify error message

patrick keshishian
On 12/25/17, Otto Moerbeek <[hidden email]> 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* `XXXXXXs' 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 XXXXXX 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
>
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: mktemp: Clarify error message

Theo de Raadt-2
In reply to this post by 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* `XXXXXXs' 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 XXXXX 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.

Reply | Threaded
Open this post in threaded view
|

Re: mktemp: Clarify error message

Theo de Raadt-2
In reply to this post by Klemens Nanni
>On 12/25/17, Otto Moerbeek <[hidden email]> 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* `XXXXXXs' 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 XXXXXX 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

Reply | Threaded
Open this post in threaded view
|

Re: mktemp: Clarify error message

Marc Espie-2
In reply to this post by Otto Moerbeek
On Tue, Dec 26, 2017 at 08:25:50AM +0100, Otto Moerbeek wrote:
>
> 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.

Actually, having at least the paragraph number in the standard would
often help a lot with the more arcane messages.

Reply | Threaded
Open this post in threaded view
|

Re: mktemp: Clarify error message

Scott Cheloha
In reply to this post by Klemens Nanni
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* `XXXXXXs' 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.

I think your error message is too complex.  It explains two possible
error cases in a single go.  This necessitates more thinking on the part
of the user.

Why not just acknowledge the two errors differently?

$ mktemp test.XXXXX
mktemp: insufficient number of Xs in template: test.XXXXX
$ mktemp test.XXXXXXC
mktemp: invalid template suffix: test.XXXXXXC
# mktemp test.XXXXXX
test.KJ9TG4

--
Scott Cheloha

Index: mktemp.1
===================================================================
RCS file: /cvs/src/usr.bin/mktemp/mktemp.1,v
retrieving revision 1.28
diff -u -p -r1.28 mktemp.1
--- mktemp.1 7 Aug 2013 06:19:36 -0000 1.28
+++ mktemp.1 26 Dec 2017 13:53:18 -0000
@@ -228,12 +228,17 @@ does not succeed and the
 .Fl q
 option was not specified:
 .Bl -tag -width indent
+.It Li "invalid template suffix"
+The specified
+.Ar template
+was suffixed with something other than
+.Ql X Ns s.
 .It Li "insufficient number of Xs in template"
 The specified
 .Ar template
 contained fewer than six
 .Ql X Ns s
-at the end.
+in its suffix.
 .It Li "template must not contain directory separators in -t mode"
 The
 .Ar template
Index: mktemp.c
===================================================================
RCS file: /cvs/src/usr.bin/mktemp/mktemp.c,v
retrieving revision 1.22
diff -u -p -r1.22 mktemp.c
--- mktemp.c 9 Oct 2015 01:37:08 -0000 1.22
+++ mktemp.c 26 Dec 2017 13:53:18 -0000
@@ -77,10 +77,10 @@ main(int argc, char *argv[])
  }
 
  len = strlen(template);
- if (len < 6 || strcmp(&template[len - 6], "XXXXXX")) {
- fatalx("insufficient number of Xs in template `%s'",
-    template);
- }
+ if (template[len - 1] != 'X')
+ fatalx("invalid template suffix: %s", template);
+ if (len < 6 || strcmp(&template[len - 6], "XXXXXX"))
+ fatalx("insufficient number of Xs in template: %s", template);
  if (tflag) {
  if (strchr(template, '/')) {
  fatalx("template must not contain directory "