[Toybox] [PATCH] Fix several bugs in date.

[Rob Landley]

On 08/11/2015 02:10 PM, enh wrote:
> On Mon, Aug 10, 2015 at 11:58 PM, Rob Landley <rob at landley.net> wrote:
>> On 08/10/2015 10:39 PM, enh wrote:
>>> Fix several bugs in date.
>>
>> Sigh. I honestly did test it, although clearly not thoroughly enough.
>> (That's how I set my clock to the year 2157, which networkmangler did
>> _not_ like...)
> 
> yeah, that's the advantage of having real unit tests. -d lets us test
> the parsing reasonably well though (even if toybox's -d is
> incompatible with coreutils').

If you mean "doesn't support multiple different fallback formats via
some magic /etc file they only tell you about in the info pages", yes.
If you mean something else, tell me more and maybe I can fix it?

I admit that "date -D %s -d 1234567890 +%s" isn't something gnu date
supports, although it can do date -d @1234567890 %s

(Hmmm, mine assumes utc on @unix input but then displays in current
timezone. Grrr. Not quite sure design-wise how to fix that. "Unix time
is in utc" seemed like the right thing to do, but "time is displayed in
current timezone unless you give it a flag" is existing behavior, and it
should be consistent...)

>> Ow. I see I checked in debug code. (I put my debug code flush with the
>> left edge so it's easy to spot and yank again.) Sorry about that.
>>
>>> We were exiting before actually setting the date, chkmktime was always
>>> failing, and we would segfault in glibc's strftime for non-Unix dates.
>>> Plus one of the tests was mangled.
>>
>> Regression test suite. This is another one where I need to run it in a
>> VM with root access...
> 
> the date tests run fine without. (well, in the version below where
> they don't assume the system timezone is UTC.) the trade-off, of
> course, is that they don't actually test setting the date. but they
> still catch all the errors we've had so far...

First time I ran said test suite it complained because it wasn't root,
then set the time to 2158. Admittedly that was because I had the test
wrong... :)

>>> Also fix existing tests and add some more, and stop using the overly
>>> vague "bad" in errors.
>>
>> I intentionally use "bad" because I'm trying to restrict the error
>> message vocabulary so non-native english speakers have a fighting chance
>> to follow along without a dictionary. See "Error messages and
>> internationalization" in http://landley.net/toybox/design.html
>>
>> It's likely localized man pages are available, or posix standards, or a
>> translation of toybox's help -ah output web page. But annotating a bunch
>> of strings with _("thingy") in each command is something I'd prefer to
>> avoid because it's intrusive and waaaay too easy to introduce printf
>> bugs. (And half the command names are english words anyway so a certain
>> amount of english is probably unavoidable, but I would like to minimize
>> the requirement for the 2/3 of the planet it's inconvenient for.)
> 
> the problem with this is that the resulting error messages suck
> because they're so vague,

That is the most common failure mode of this approach, yes. :(

> and you can't even tell what the problem was
> by looking at the source (because it's one error for every possible
> point of failure). i agree that "invalid" isn't great, but i couldn't
> come up with a short way to say what it actually means (other than
> "non-normal", which is accurate but not helpful even for 99.9% of
> native English speakers).

Which is why I try to show the thing I couldn't parse, preferably with
the location at which I couldn't parse it. User supplied input doesn't
need to be translated. :)

Not really an option here since the library's doing the parsing for us
and not providing this granularity of information. Recreating the output
(twice) was the best I could come up with. (Error handling is hard, even
when we essentially abort() in response to almost everything.)

> (your latest patch actually makes things worse because now you're not
> even showing the string you failed to parse! fixed in the patch
> below.)

Sorry, quick 2am "fix the obvious breakage, look closer in the morning"
thing. Wound up spending the morning trying to get cpio -p to work on
nommu instead, and starting the skeleton of a tar.c rewrite.

Oh, side note: cpio doesn't store xattrs. It also has a 32 bit date
(y2038) and 32 bit file lengths. This means initramfs is kinda limited.
Do you care strongly about this?

There was a thread on linux-kernel a while back about possibly extending
the format to fix htis, but it petered out. Since I'm looking at cpio
anyway, I'm thinking of doing a 070702 format with some extra data
fields, plus an initramfs patch so the kernel can read 'em to populate
initramfs...

>> I had two messages this time because failure to handle input and failure
>> to handle output are two distinct error conditions. (Failure to parse
>> date is because it's a bad date. Failure to _set_ the date is usually a
>> permissions issue, you'll note the "cannot set date" and "bad format"
>> messages are perror_exit() and the libc message probably _is_
>> internationalized.)
>>
>> Generally when I want to provide more details about failure to parse a
>> string I say "bad stringname" and then give the string and the offset in
>> it, ala sed's error_exit("bad pattern '%s'@%ld (%c)", errstart,
>> line-errstart+1L, *line);
>>
>> Other times I use "bad $ADJECTIVE date" where you can treat $ADJECTIVE
>> as a swear word and still get the general idea. You're distinguishing
>> failure to parse ("bad date format", and since format is already a word
>> in the help text it's not _entirely_ new) from the date itself being
>> invalid ("bad date").
>>
>> Rather than asking non english speakers to distinguish "invalid" from
>> "bad", I'd rather try to print the input and output dates to show why
>> they don't match, ala error_msg("%s != %s", unfiltered, filtered);
> 
> yeah, that's a good idea. done in the patch below. this also turned up
> another couple of bugs (also fixed in the patch below).
> 
>> I checked in about 1/3 of your patch (most obvious bugfixes), I need to
>> look at the rest in the morning. (I wanted to turn chmktime() into
>> xmktime() and stick it in lib/xwrap.c but it didn't error_exit().
>> Changing the semantics of that makes life easier there. But not
>> something to fiddle with at 2am... :)
> 
> what you did actually made date worse (with the exception of removing
> the exit; at least what's currently checked in should work if you
> supply a Unix time with @). if you didn't have such an urge to win the
> obfuscated C contest, toybox would be much better off...

Sorry, it was 2am I and I was trying _not_ to fiddle, while not leaving
the tree in a broken state.

Applied this one as is.

>> Thanks,
>>
>> Rob
> 
> the above-promised patch follows...

Your email client still wraps long lines, FYI. (Fixed 'em up, but it can
lose multiple spaces in the breaks and tests can care..)

Thanks,

Rob

 1439327357.0