On Thu, 2006-04-20 at 22:43, Dave Miner wrote: > [ request-sponsor removed from Cc list for remainder of discussion ] > > Consider the comments below my code/design review. One thing that I've > asked the internal team to do is to conduct design reviews for RFE's in > this community. We will also be doing code reviews here once the web > site support is available (yes, I know there are some options out there > now, but they're too manual IMHO; I'm trying to balance the effort vs. > the benefit for the short-term).
Thanks for the comments. More involvement would be good - especially as I've got more changes (across the whole of the tools) in the pipeline. > Peter Tribble wrote: > > I would like a sponsor for several pkgchk related issues. > > > > See http://www.petertribble.co.uk/Solaris/fixes/1/ for details > > (given below) and diffs. > > > > > > 1. 6218542 (also the core reported in 5035606) > > > > Removed the call to selpath. The problem is that the first branch of > > the preceding if block has already taken ppathlist[n] off the end of > > the array - replacing n with 0 stops the crash, but in fact checkmap > > also has exactly the same code, so that it's better to remove this > > call completely. > > > > (I've also removed the forward declaration of selpath from main.c as > > it's no longer necessary. And selpkg too.) > > > > This also solves the problem of duplicate output such as the > > following: > > > > pkgchk -p hjzxv > > NOTE: Couldn't lock the package database. > > WARNING: no information associated with pathname <hjzxv> > > WARNING: no information associated with pathname <hjzxv> > > > > No issues with this part. > > > > > 2. 6412140 > > > > Rewritten the ELF check for the file referenced by the -i flag. I > > presume that this feature is to guard against supplying a binary file > > as input, but it also catches genuine cases. So I explicitly check just > > the first 4 characters of the file for the ELF signature. > > > > Fair enough. I'm not sure how useful this check really is, as there are > a lot of other types of files which would be somewhat toxic as input, > but I guess it doesn't hurt. I'm not sure either. Presumably there would be some record of why it was added. Without the history, I don't know enough about what it's meant to do to feel safe in taking it out. Besides, I can see 'pkgchk -l -i /bin/ls' as a plausible typo. > > > > 3. 6412749 > > > > Enhanced to allow -i input file to be specified on stdin. > > > > Check and generate an error on empty input file. > > > > This last check adds an extra error message. > > > > Fine by me. > > > > > 4. 6412765 > > > > I guard against overly long input strings (doesn't do anything > > smart, but doesn't crash). > > > > I do this by adding a length specifier to fscanf. (Ideally this would > > use PATH_MAX rather than being hardcoded to 1024, but I'm not sure how > > to get a numeric define in a format specifier past the preprocessor.) > > > > OK with this part. I don't have a better idea. It's a tricky one. I did see some two-layer substitution trick, but it looked pretty ugly. > > > > 5. 6322837 > > > > I've added a simple check that the input file specified is indeed a > > regular file. > > > > Note that this actually constitutes a change of behaviour, and will > > cause anything that explicitly uses /dev/stdin (rather than the more > > conventional notation - see point 3 above) to fail. I have identified 2 > > places where this occurs > > > > /sfw/usr/src/pkgdefs/common_files/checkinstall.initd > > /on/usr/src/pkgdefs/common_files/checkinstall.initd > > > > Ouch; synchronizing fixing those two would be painful, we might also > need an ARC fasttrack to allow this change. How about adding an S_ISCHR > as well, which would continue to allow /dev/stdin to work? That idea falls over. You actually need S_ISFIFO for stdin, and that doesn't work very well either. My preferred solution (after trying a number of variations that didn't work) is simply to check for the "/dev/stdin" string explicitly. Diff from my original: *** main.c.firstpass Tue Apr 18 19:11:32 2006 --- main.c Sun Apr 23 20:26:14 2006 *************** *** 530,535 **** --- 530,537 ---- if (strcmp(file, "-") == 0) { fplist = stdin; + } else if (strcmp(file, "/dev/stdin") == 0) { + fplist = stdin; } else { if ((fd = open(file, O_RDONLY)) == -1) { progerr(gettext(ERR_IOPEN), file); This captures the exact usage, and doesn't allow other peculiar file to get through the trap. > > 6. 6413788 > > > > Fixed various typos and removed unused defines. > > > > Thanks for cleaning up the trash! > > I've built and tested the submitted code and it does work as billed. That's good to hear. I mean, I hoped that would be the case, but it's nice to have it confiremd that I haven't goofed. -- -Peter Tribble L.I.S., University of Hertfordshire - http://www.herts.ac.uk/ http://www.petertribble.co.uk/ - http://ptribble.blogspot.com/
