Jason King writes:
> On Thu, Apr 30, 2009 at 10:52 AM, James Carlson <james.d.carl...@sun.com>
> wrote:
> >> Looking for reviewers for http://cr.opensolaris.org/~jbk/ls -- This
[...]
> > 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.
Yes. Variables with static storage duration (that is, things not
declared as either function arguments or automatics) are required by
the ANSI C standard to be automatically initialized to zero (or NULL
if a pointer). See section 6.1.2.4 and 6.5.7 of ANSI/ISO 9899-1990.
On UNIX systems, this is accomplished by putting them into the ".bss"
("block started by symbol" -- don't ask; it's an old IBM thing)
section. This area is automatically bzero'd before main() starts
running.
By initializing, you force the variable into the ".data" section.
Unlike .bss, the .data section takes up space in the actual binary.
It's a bit of a small nit, but if you grow up on embedded systems, you
remember these things ...
> > 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.
OK.
> > 374: why declare this table inside main()? Suggest moving out.
>
> I followed the example in the man page :) Will move it outside of main().
Ah, ok. I don't think the man page example is nicely coded. :-/
> > 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).
It's mostly the removal of the needless failure mode (strdup could
possibly return NULL) that I'm after.
> > 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,
OK.
> but not sure the proper process to amend things slightly to support
> it. Guidance here would be helpful.
As far as the ARC case goes; it's trivial. You just drop a line to
psarc-...@sac.sfbay with the case number and title in the subject line
("2009/228 ls enhancements") and say something like this:
One of the code reviewers noticed that the suffixes supported
by "--block-size=NN" are not accurately reflected in the case
materials. The actual syntax (as an RE) is:
[kKmMgGtTpPeE]B?
The optional "B" (upper case only) signifies powers of 10
rather than 2 (e.g., KB is 1000 and K is 1024). This is
obviously not the same as the -h/-H that were added to du and
df, nor is it compatible with the standard SI notations. It
is compatible with GNU ls, though.
As this is just a correction to bring the case materials back
into agreement with the GNU ls behavior, I believe that it
doesn't affect the vote that was taken, nor does it require a
separate case to discuss. If anyone disagrees, please speak
up now.
> > 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.
OK.
> > 593: comment is confusing; what's actually being replaced is the
> > 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).
OK.
> > 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.
As above; a message to the case should do it. At some point, you
might cross the line between "trivial updates" and "amendments." You
might want to talk to your case sponsor (Garrett) about that.
> > 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).
The tool doesn't catch everything, unfortunately.
> > 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.
OK; sounds good.
> > 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.
Ah, ok. I was just going by the fact that the original code tested
"eflg" first, and checked "Eflg" only in the else clause.
> > 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
OK; I get it. I was actually confused by the 'diff' output, which
ended up getting tortured by the bit changes in this area. I see now
what you're doing.
> > 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.
That doesn't sound right to me. The man page for tparm() says:
Routines that return pointers always return NULL on error.
None of them can return ERR (which is an 'int' anyway). I don't think
this check is right. (I don't think it's _harmful_, but I don't think
it's correct.)
If there is some case in which tparm() returns "(char *)-1", then we
need to have a bug filed against libcurses; that's not right.
> > 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.
I'm confused. 2861 doesn't check for a NULL pointer at all. In fact,
it'll crash on a NULL pointer. It calls strlen() which returns the
length of the pointed-to string.
Can 'colorstr' be NULL on input? If so, then you need an explicit
check for that. NULL and zero-length string are not the same thing.
Try running this small program:
#include <stdio.h>
#include <string.h>
int
main(void)
{
(void) printf("%d\n", strlen(NULL));
return (0);
}
> > 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.
It actually works fine to have multiple strtok_r's on the same string;
that's what the third parameter gives you. But ok; I can live with
this.
> > 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.
OK; that seems fine.
> > 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.
I was asking about the reason. Modifying the string in the
environment is safe (UNIX doesn't do the old MS-DOS trick of a
double-NUL terminated list and instead uses an actual array), so
changing this is ok, but I wanted to check why the strdup was there.
--
James Carlson, Solaris Networking <james.d.carl...@sun.com>
Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
_______________________________________________
opensolaris-code mailing list
opensolaris-code@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
