[ 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).
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. > > 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. > > 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? > 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. Dave
