On Fri, May 1, 2009 at 8:42 AM, James Carlson <james.d.carl...@sun.com> wrote: > 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.
I have an updated webrev http://cr.opensolaris.org/~jbk/ls-2 (in case anyone wants to compare to the original version). Hopefully, I've addressed all the points, but of course please let me know if I have not. I will send an email to psarc-ext a bit later with the two clarifications. _______________________________________________ opensolaris-code mailing list opensolaris-code@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
