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

[enh]

On Tue, Aug 11, 2015 at 2:09 PM, Rob Landley <rob at landley.net> wrote:
> 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?

no, i think toybox is saner than GNU here. i was just obliquely
pointing out that these tests will fail when run against the host
tools.

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

yeah, it's the "date -d 14081805" format that toybox supports (yay!)
but GNU doesn't (boo!).

> (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...)

the toybox behavior of "use what i told you to use" was unsurprising to me :-)

>>> 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.)

and the real kicker is that date accepts such a bewildering variety of
formats, all via the same option. correct date inputs don't look much
different from incorrect ones. and there's no way in hell you'd ever
work out that GNU's -d takes input like "+2 days". (about the only
useful thing i found that GNU supports that toybox doesn't is ISO date
format. but YAGNI.)

>> (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?

the ramdisk is on its way out. i think some of this work is already
visible in AOSP. there are certainly people internally running devices
that boot from a squashfs system image and although they do still have
a ramdisk, they only use it for the recovery image. (with the longer
term plan killing that too.) so "no, i don't think so".

> 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..)

argh. i'll try to remember to use the basic HTML UI when sending patches...

> Thanks,
>
> Rob



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.

 1439614514.0