Responses inline...
On Thu, Apr 30, 2009 at 10:52 AM, James Carlson <james.d.carl...@sun.com> wrote:
> Jason King writes:
>> Looking for reviewers for http://cr.opensolaris.org/~jbk/ls -- This
>> adds a number of GNU compatibility options (including color) to
>> Solaris ls.
>
> ls.c
>
> 253,255,336,338,2954: nit: suggest using ANSI '(void)' here.
Acccepted.
>
> 277,304,305,333,340-351: nit: initialization to zero isn't needed.
Is that done automatically due to the section in the binary they are
placed? I don't want them to have unknown values.
>
> 330: it'd be nice to avoid having to check for NULL (and just set
> the value of time_fmt_new to be equal to time_fmt_old in those
> cases), but it looks like that might be hard.
I believe that should work. I will try this out separate from the
rest of the fixes though to make sure it doesn't present any problems
before I will say definitively.
>
> 336-338: these seem misplaced; do they belong near line 255?
Yes. I did move them, but apparently forgot to remove the old ones,
so they're actually duplicated. The duplicates on 336-338 will be
removed :(.
> 374: why declare this table inside main()? Suggest moving out.
I followed the example in the man page :) Will move it outside of main().
> 484,582: is duplicating this string necessary? I thought that
> optarg pointed to modifiable storage.
I couldn't find anything that said one way or another, so I opted for
caution and assume it doesn't (or at least that it could not be relied
upon). If that isn't the case, I'll have it modify the string in
place (and simplify things a bit).
> 488-513: this doesn't look like the set supported by GNU ls (kB, K,
> MB, M, GB, G, ...), nor does it look like the -h/-H used by df and
> du. (We probably should have caught this in ARC review, but I think
> the assumption of those reading the case was that it would conform
> to GNU's behavior for the options that were implemented.)
Actually it is -- ls --help mentions it at the bottom. I thought I
mentioned it, but in case I didn't, I explicitly did NOT (nor have I
ever that I can recall) look at the GNU ls source code to hopefully
preclude any issues (yes, it's probably overkill), so everything was
done either from the GNU ls manpage, the ls --help output, or by
running tests. In this case, it appears it accepts the [Kk][Mm][Gg]
form. Though looking at it closer, it does look like it also supports
a 'B' (only uppercase, the other modifiers seem to accept upper or
lower case) suffix to indicate powers of 10 instead of 2. I didn't
catch that originally. It'd probably be good to support that as well,
but not sure the proper process to amend things slightly to support
it. Guidance here would be helpful.
>
> 518: should probably protect against negative and zero here.
Accepted.
>
> 542,2552: stray XXX comments need to be removed before integration.
> (Design issues need to be resolved.)
They will be removed (should have been).
>
> 560,568-569: should these new formats also have #defines like the
> others?
The can be. In addition, I'm going to rename them to FORMAT_OLD,
FORMAT_ISO_OLD, etc. so it's a bit more obvious which format is which.
>
> 584: these 'continue' statements look more like 'exit(1)' to me.
See below.
>
> 590,598: why does this need to be allocated, and if it does need to
> be allocated, why is calloc required?
Was just to make it 0 filled, but will rewrite the +CUSTOM_FORMAT
processing block to only require a single allocation (the current code
requires leading and trailing spaces around the format to line up
right, so a slightly larger string than optarg needs to be allocated
to hold them.
> 593: comment is confusing; what's actually being replaced is the
> first character in the array, which happens to be '\0'.
See above
>
> 608: I doubt that this code works; nothing ever copies 'tmp' (or
> optarg) into 'told'. Thus, the 'told' string is always exact " "
> (two spaces), set by lines 594 and 595.
Hmm.. I know I tested this, but somehow managed to screw it up later.
I will correct the comment and fix this. What is happening, is for
whatever reason, the existing format strings include both a leading
and trailing space, instead of letting the output code pad it, thus
the specified custom format needs the leading and trailing space
added, as well as possibly broken up into 2 strings if there's an
embedded newline (for old vs. new format).
> 612: does getting here represent an error in "time-style"?
Hmm yes, an error should be printed out and exit. Will correct.
> 627-629: This was cited as an existing Solaris ls flag in the ARC
> case. Why is it being added? (I'm guessing that was just a minor
> mistake in the ARC materials ...)
Yes. It is a new flag. Let me know the proper way to correct the case.
>
> 782: range checks? (Perhaps lines 922-924 should just be taken
> outside of the 'if' block starting at line 912, so that num_cols is
> always repaired if in error, no matter whether it comes from the
> user or from the system.)
Good idea. I will move the check to have it apply regardless of source.
> 880: 'U' wasn't included here.
Accepted.
> 1217: this will drop core if block_size isn't validated (at least
> non-zero) when set by users.
Accepted.
> 1219: not your code, but this test does nothing. Just using "%7lld"
> all the time would work fine.
I will change it.
> 1460: C Coding Standards: '==' operator goes at end of previous
> line.
Accepted. (I thought cstyle caught that -- and I know I ran the pbchk
on my workspace before generating the webrev).
> 1717,1732: what are these changes about? They don't seem related to
> this project. (Do they fix a bug ... ?)
Yes -- I thought someone else was planning to file a bug on this (I'll
ping them to see if they still plan to) -- basically the OpenSolaris
ls (bin/xpg4/xpg6) fail the SUS test suite (and have since launch --
even rev 0 in the hg ON repo on os.o fails). Those are to fix that.
> 2495: I think this should be '!eflg && Eflg' to capture the original
> logic.
It actually should -- the -E time format doesn't get localized, thus
has to go through snprintf before going through strftime.
format_etime in the existing code was only called when -E was invoked.
Also see line 660 -> Eflg implies !eflg always.
>
> 2496: can 'fstr' potentially be NULL here due to the convention used
> for time_fmt_new?
Hmm yes, this will be changed -- an earlier revision was such that it
couldn't, but looking at it, fixing that now meant that's no longer
the case.
> 2553: this looks like a functional change to me. Why would we print
> these things at all if Eflg isn't set? The old code didn't do that.
The function (from the existing code) is poorly named (I can change it
if desired). print_time() is only called when - '% all' is specified
-- see lines 859, 1312
-E vs. -e, etc. is a bit more interesting, as best as I can figure out
(the current code doesn't make it obvious imo), the current behavior
is:
(hopefully this lines up)
Flag Timestamp format besides filename timestamp: lines
-l FORMAT1/FORMAT2 (old/new) n/a
-le FORMAT3 n/a
-E FORMAT4 FORMAT4
-l% all FORMAT1/FORMAT2 (old/new) FORMAT3
-le% all FORMAT3 FORMAT3
-lE% all FORMAT4 FORMAT4
Since GNU ls has no equivalent to -% all, and since I couldn't find
anything that documented this actual behavior as such, I opted to stay
with FORMAT3 for -% all (the timestamp: lines) unless -E is used.
Hopefully, the new version is a bit clearer.
> 2537,2577: nit: save is unconditional, so restore can be as well;
> 'if' not needed here, and code is safer without.
Accepted.
> 2583: could use a comment or two (at least a block comment).
Accepted.
> 2584,2589,2683,2767,2793,2795,2934, et al: C Coding Standard:
> consider using better variable names.
Accepted.
>
> 2774: not sure what this comment means. If not now, when?
Old comment that needs to be rewritten (ls_color_init does sort the color data):
/*
* Colors are sorted from most general lsc_colors[0] to most specific
* lsc_colors[lsc_ncolors - 1] by ls_color_init(). Start search with
* most specific color rule and work towards most general.
*/
> 2776: this skips over (does not visit) lsc_colors[0]. Is that
> intentional?
No. Will change to >= 0
> 2788: nit: missing blank line between variable declarations and
> executable code.
Accepted.
> 2788: when does tparm return (char *)-1?
I ran into this early on in trying to figure out how to get the proper
output sequence via terminfo for manipulating color, but I now forget
the exact circumstance. Will change to ERR per curs_terminfo(3CURSES)
manpage.
> 2833: nit: could be const.
Accepted.
> 2861-2684: unnecessary; check at line 2867 will catch this.
The manpage wasn't clear on the behavior of strtok_r when called for
the first time with a NULL pointer, I erred on the side of caution,
but will remove those lines.
> 2875: why strdup?
Unsure of guaranteed behavior with multiple strtok_r's processing the
same string, so I tend to take the conservative approach about
modifying strings in place.
> 2904-2919: this doesn't appear to work. The value computed is
> overwritten at line 2932. Did you mean to use 'attr' in each of
> these assignments?
Yes.
> 2899: consider using strtol(p, NULL, 10) here to avoid accidental
> octal interpretation.
Accepted.
> 2918: this value doesn't seem to be used in the 'default_colorstr'
> string. It's a good thing, because atoi would likely have trouble
> with "08" (which is an illegal octal value).
GNU ls is very vague as to what is actually acceptable. Based on
anecdotal evidence, it merely does the equivalent of printf("\e[%s",
<stuff after = from color rule>), but doesn't actually document what
codes are actually valid. Since it is strongly implied that it's
using the ANSI specification, from the references I could find at the
time, 8 seemed to be valid (if somewhat rare) that had a corresponding
terminfo equivalent, so I included it.
>
> 2980: why? (Concern about modifying LS_COLORS in the environment
> ... ?)
Not sure if that was permitted or not. If ok to do, I'll get rid of
the allocation.
_______________________________________________
opensolaris-code mailing list
opensolaris-code@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
