On Sat, Feb 7, 2015 at 6:02 PM, Rob Landley <r...@landley.net> 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 Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net