[Toybox] [Patch] - touch, kill and umount fixes
Rob Landley
rob at landley.net
Mon Sep 22 05:23:59 PDT 2014
I spent the weekend off in my corner doing various fixes to the code, so
I'm a bit behind on email...
On 09/19/14 00:40, Ashwini Sharma wrote:
> Attached is the updated patch.
This doesn't put back the "Z" case for timezones, so I thought it
applied on top of the other patch, but when I did that 3 of the 4 hunks
failed...?
I'm confused.
I took out the 'Z' case because the comment led me to think it was a
workaround for a libc bug, in which case it was probably just uClibc and
musl/glibc probably didn't have that issue. But when I went to test it
over the weekend I confirmed that musl doesn't do it either, and the
spec doesn't actually _require_ it of strptime, so yeah the code needs
to be put back in. My bad.
Ideally this is what the test suite is for, but making a test that
properly checks every interesting case is approximately as fiddly as
designing the command right in the first place (and requires just as
close a reading of the spec). Really I should have a "pending" directory
for test files, but there's no infrastructure for that. (I did move the
test files around over the weekend so they're easier to update in
future, but they haven't got categories yet like the command code does.)
> On Fri, Sep 19, 2014 at 12:33 AM, Rob Landley <rob at landley.net
> <mailto:rob at landley.net>> wrote:
>
> On 09/18/14 05:38, Ashwini Sharma wrote:
> > Hi Rob,
>
> Hi. Your last round of fixes (which I've applied a little over half of)
> touched some of the files needing cleanup outside of the pending
> directory. (I.E. command contributions that predated "pending".) I
> updated the toys/pedning/README file listing them (so it's not just my
> private list), and have been looking at a few. (Yesterday it was "touch"
> and "cut" I think.)
I think I've applied all ofthem except touch now...
For touch I applied the patch from this email and put the "Z" case back
as well, but now...
date = TT.time;
+ i = ((s = strchr(date, '.'))) ? s-date : strlen(date);
+ if (i < 8 || i%2) error_exit("bad '%s'", date);
for (i=0;i<3;i++) {
s = strptime(date, toybuf+(i&2), &tm);
if (s) break;
toybuf[1]='y';
}
strptime() advances s to the "first character not processed in this
function call", or 0 if it couldn't match the whole pattern. That's why
I was checking for '.' afterwards, it should naturally advance (and not
get confused if a timezone had a period in it).
Then down at the bottom there's already a perror_exit("bad '%s')
although that's not right because the errno would be 0 for the !s case
and thus it would print "bad 'name': no error" which is just silly...
So yeah, the code needs fixing but this is a bit awkward. Lemme look at
your actual bug reports...
Hang on: why did you remove the tv_usec setting ability? (Did it not
work?) The point of the code removed by:
- if (s && *s=='.') {
- int count = sscanf(s, ".%2d%u%n", &(tm.tm_sec), &i, &len);
-
- if (count==2) tv->tv_usec = i;
+ if (s && *s=='.' && sscanf(s, ".%2u%n", &(tm.tm_sec), &len) == 1)
s += len;
- }
Was to set tv_usec, I.E. fractions of a second. (Looking at it, I can
see it needs padding with the appropriate number of zeroes since .123
should be 3 tenths of a second and 3 usec is 3 millionths of a second.
So yes it needs fixing, but why remove it? Ah, I see you talk about that
later on...)
> > Attached are the patches. This has fixes for
> >
> > touch:
> > 1. while setting access or modify times of a file, this was causing a
> > hang due to non-increment of ss.
>
> I actually have more fixes to touch pending: the logic is wrong for file
> create. I think if you combine "touch -a -d" when creating a file, it
> should set the access time to what you specified with -d but leave the
> modification time at the current (creation) time? I need to read the
> spec and test what the ubuntu version does.
>
> Query: this patch replaces the stanza I just took _out_ that tries to
> work around a libc bug. Which libc is having the problem?
>
> Does the for loop not work?
>
> 2 use cases here.
>
> 1. update access or modify time one at a time.
> __fetch()__ was checking for __TT.file__ instead of input parameter
> __file__. Failure
> here as TT.file = NULL, causes a jump to open statement in loop. Success
> here results in endless loop.
Indeed, TT.file is passed in as the parameter for the first call but not
subsequent calls. Looks like I moved code out to a file and then didn't
completely functionize it.
(Again, I need to do a proper test file for this...)
So ok: that part's a pure bugfix. And putting 'Z' support back is a pure
bugfix.
> 2. file to be touched is existing and owned by other user.
> In this open(O_CREAT) did succeed, but utimes() fail returning
> EPERM. Again the endless loop.
Ah, good catch. But... I don't see how adding access() helps here? F_OK
just tests for the existence of the file?
>
> > 2. handling the time format for __-d__ option as per the spec
> > (http://pubs.opengroup.org/onlinepubs/9699919799/utilities/touch.html).
>
> The majority of this patch affects the -t case, not the -d case?
>
>
> for -d case, it is taking into account the UTC time zone.
As mentioned above, I retested this and you're right, we do need to
special case it here. Comment was a bit misleading, possibly I should
rephrase it...
>
> - strcpy(toybuf, "%Y%m%d%H%M");
> date = TT.time;
> - for (i=0;i<3;i++) {
> - s = strptime(date, toybuf+(i&2), &tm);
> - if (s) break;
> - toybuf[1]='y';
> - }
> + i = ((s = strchr(date, '.'))) ? s-date : strlen(date);
> + if (i == 8) strcpy(toybuf, "%m%d%H%M");
> + else if (i == 10) strcpy(toybuf, "%y%m%d%H%M");
> + else if (i == 12) strcpy(toybuf, "%Y%m%d%H%M");
> + else perror_exit("bad '%s'", date);
> +
> if (s && *s=='.') {
> - int count = sscanf(s, ".%2d%u%n", &(tm.tm_sec), &i, &len);
> -
> - if (count==2) tv->tv_usec = i;
> - s += len;
> + if ((sscanf(s+1, "%2u%n", &(tm.tm_sec), &i) != 1) || *(s+i+1))
> + error_exit("bad '%s'", date);
> + s += (i+1);
>
> Where did the call to strptime go? This new code seems to set up toybuf
> (using three separate strings instead of three cases on one string), and
> then... not call strptime? I'm confused...
>
> my bad, attached is the updated patch. I preferred if..else over loop
> calling strptime() 3 times. Though the loop is kept intact now.
>
> __for__ loop works fine, but as per spec it should accept
> [[CC]YY]mmddMMHH[.SS], all the fields in pairs of two digits.
> strptime() doesn't care for this two digit format, leading '0' may or
> may not be there.
> Finally [.SS] can be in the range [00, 60] and no further fractions.
> sscanf(".%2d%u%n") would fail when input is like "1910200240.24" or
> "1910200240." (Segfault)
> as len is not updated properly and s is advanced by the same.
I need to put together a decent tests/touch.test that includes all these
issues. (The "file belongs to another user" one is hard to do without
root access. There are some tests that only run as root, but I need to
work out a proper setup to run the test suite as root without
potentially hosing my system if commands being tested malfunction. Way
back when I used a chroot, these days it might involve qemu... Have to
think about it.)
In the meantime:
$ VERBOSE=1 scripts/test.sh touch
...
FAIL: touch -d
echo '' | touch -d 2009-02-13T23:31:30.12Z walrus && date -r walrus +%s.%N
--- expected 2014-09-22 06:54:39.709277379 -0500
+++ actual 2014-09-22 06:54:39.717277378 -0500
@@ -1 +1 @@
-1234567890.120000000
+1234567890.000012000
I'll see what I can do.
Thanks,
Rob
1411388639.0
More information about the Toybox
mailing list