On Sat, Feb 7, 2015 at 6:02 PM, Rob Landley <[email protected]> wrote:
> On 02/07/2015 12:04 PM, enh wrote:
>> Use $TMPDIR if set (necessary on Android, where there is no /tmp).
>> Include full template in error messages.
>> Don't report success on failure with -q.
>> Avoid unnecessary allocation.
>> Fix "xxxxxx" versus "XXXXXX" confusion.
>
> Apparently I'm not capable of consistently spelling your name in commit
> -u arguments.
i blame my parents. (and have been using enh as my username since 1993
for pretty much this reason.)
>> diff --git a/toys/lsb/mktemp.c b/toys/lsb/mktemp.c
>> index c1175fe..52e53ee 100644
>> --- a/toys/lsb/mktemp.c
>> +++ b/toys/lsb/mktemp.c
>> @@ -12,8 +12,8 @@ config MKTEMP
>> help
>> usage: mktemp [-dq] [-p DIR] [TEMPLATE]
>>
>> - Safely create new file and print its name. Default TEMPLATE is
>> - /tmp/tmp.XXXXXX and each trailing X is replaced with random char.
>> + Safely create a new file and print its name. The default TEMPLATE is
>> + tmp.XXXXXX. The default DIR is $TMPDIR, or /tmp if $TMPDIR is not set.
>
> I redid the help text a bit.
>
>> -d, --directory Create directory instead of file
>> -p DIR, --tmpdir=DIR Put new file in DIR
>> @@ -29,24 +29,27 @@ GLOBALS(
>>
>> void mktemp_main(void)
>> {
>> - int d_flag = toys.optflags & FLAG_d;
>> - char *tmp;
>> + int d_flag = toys.optflags & FLAG_d;
>> + char *template = *toys.optargs;
>> + int success;
>
> "success" isn't used and that's two int declaration lines anyway.
>
>> - tmp = *toys.optargs;
>> -
>> - if (!tmp) {
>> - if (!TT.tmpdir) TT.tmpdir = "/tmp";
>> - tmp = "tmp.xxxxxx";
>> + if (!template) {
>> + template = "tmp.XXXXXX";
>> }
>
> I tend to avoid curly brackets around single lines unless there's
> if/else confusion or similar. (Code style thing.)
>
>> - if (TT.tmpdir) tmp = xmprintf("%s/%s", TT.tmpdir ? TT.tmpdir : "/tmp",
>> - *toys.optargs ? *toys.optargs : "tmp.XXXXXX");
>>
>> - if (d_flag ? mkdtemp(tmp) == NULL : mkstemp(tmp) == -1)
>> - if (toys.optflags & FLAG_q)
>> - perror_exit("Failed to create temporary %s",
>> - d_flag ? "directory" : "file");
>> + if (!TT.tmpdir) TT.tmpdir = getenv("TMPDIR");
>> + if (!TT.tmpdir) TT.tmpdir = "/tmp";
>> +
>> + snprintf(toybuf, sizeof(toybuf), "%s/%s", TT.tmpdir, template);
>
> So if we _do_ have tmpdir+template combining to be bigger than the old
> PATH_MAX, we silently truncate. That seems more like a "throw an error"
> situation...
true. i'm usually the one arguing against fixed-length buffers, so i
should be ashamed of myself.
sort-of speaking of which... i didn't include this before since it
wasn't really a bug fix but do you think we should use more
randomness? 6 Xes is the minimum you're allowed to pass to the C
library, and the desktop mktemp(1) defaults to 10.
diff --git a/toys/lsb/mktemp.c b/toys/lsb/mktemp.c
index 498f9f1..2c0adf2 100644
--- a/toys/lsb/mktemp.c
+++ b/toys/lsb/mktemp.c
@@ -19,7 +19,7 @@ config MKTEMP
-q Quiet, no error messages
Each X in TEMPLATE is replaced with a random printable character. The
- default TEMPLATE is tmp.XXXXXX, and the default DIR is $TMPDIR if set,
+ default TEMPLATE is tmp.XXXXXXXXXX, and the default DIR is $TMPDIR if set,
else "/tmp".
*/
@@ -35,7 +35,7 @@ void mktemp_main(void)
int d_flag = toys.optflags & FLAG_d;
char *template = *toys.optargs;
- if (!template) template = "tmp.XXXXXX";
+ if (!template) template = "tmp.XXXXXXXXXX";
if (!TT.tmpdir) TT.tmpdir = getenv("TMPDIR");
if (!TT.tmpdir) TT.tmpdir = "/tmp";
>> - xputs(tmp);
>> + if (d_flag ? mkdtemp(toybuf) == NULL : mkstemp(toybuf) == -1) {
>
> I try to avoid == 0 comparisons and such, because aero is special in C.
> A != 0 test ia a NOP, and for X == 0 we have have !X.
>
> And I've just about stopped using "NULL" entirely. 0 gets typecast to
> everything, is shorter, and I actually had something get _confused_ by
> being fed a NULL where it wanted a 0 because there was a typecast on it.
> (In musl there was a whole argument where they came to the conclusion
> NULL had to be 0L so it padded right in printf() without causing the
> unpleasant side effects of pointer typecasts, or something like that.)
>
> Neither is a big deal, just an "I tend to wander through after and clean
> those up for consistency" sort of heads up.
>
>> + if (toys.optflags & FLAG_q) {
>> + toys.exitval = 1;
>> + } else {
>> + perror_exit("Failed to create temporary %s with template %s/%s",
>> + d_flag ? "directory" : "file", TT.tmpdir, template);
>> + }
>> + }
>>
>> - if (CFG_TOYBOX_FREE && TT.tmpdir) free(tmp);
>> + xputs(toybuf);
>
> Your comment at the top said not to report failure as success, but
> you're doing an xputs(toybuf) in the -q failure case anyway? (I added an
> else, I assume that's right?)
yes, what you committed is correct. i broke that when i fixed
something else because i wasn't writing tests.
thanks for fixing up touch too; i've killed the toolbox touch now.
i really must get into the habit of writing actual tests for toybox...
_______________________________________________
Toybox mailing list
[email protected]
http://lists.landley.net/listinfo.cgi/toybox-landley.net
