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 <[email protected] > <mailto:[email protected]>> 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 _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
