Hi Paul, thank you for code review. I have updated webrev. Please see in line for more info.
Paul Cunningham wrote: > Petr, > > See below for my quick skip through comments ... > > Paul > > Petr Sumbera wrote: >> >> I have prepared webrev for Wireshark integration to Solaris Nevada >> (PSARC PSARC/2007/334). Can someone do code review for me? >> >> http://cr.opensolaris.org/~xsumbe00/6567201-wireshark/ > > === Start of Comments ==== > > 1. CDDL HEADER and top of files > Cosmetic: you might want to update the tops of > the files so that they conform to that in ... > "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/"; > Most have just got a missing line-space, but > there are a few that need more work. Done. > 2. usr/src/cmd/wireshark/METADATA > Make the NAME: field more descriptive Done. I also changed SUPPORT filed to Integrated. > 3. usr/src/cmd/wireshark/Makefile.sfw > Do you need to run 'protofix' on the stuff installed > by 'make install' to correct permissions/ownership? > (you probably will go after fixing (7)) Added. I also added to CFLAGS "-I/usr/include/pcre" so that libpcre is detected and used. Plus enabled kerberos support (--with-krb5 configure option, also autocnf is run to get changes from one patch to configure script). Per Marcel's note I'm passing environment after configure (and the same for make): ./configure $(TARGET_ENV) > 4. usr/src/cmd/wireshark/sunman-stability > Is "Interface Stability Unstable" correct? Changed to Uncommitted. > 5. usr/src/pkgdefs/SUNWwireshark/copyright > & usr/src/pkgdefs/SUNWwiresharkr/copyright > Do you need to add any owner copyright lines near > the top of the file, eg. the copyrights from src files > in the unpacked src tarball? Added. > 6. usr/src/pkgdefs/SUNWwireshark/depend > Move Copyright lines to after the 'CDDL HEADER END' > header. Done. > 7. usr/src/pkgdefs/SUNWwireshark/prototype_com > Remove the write permissions from the files installed > into /usr Done. > 8. usr/src/pkgdefs/SUNWwireshark/prototype_i386 > & usr/src/pkgdefs/SUNWwireshark/prototype_sparc > & usr/src/pkgdefs/SUNWwiresharkr/prototype_i386 > & usr/src/pkgdefs/SUNWwiresharkr/prototype_sparc > Cosmetic: add the SUNW pkg name as comment, just for > consistency with other pkg files, example see ... > "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/prototype_i386"; > Done. > 9. usr/src/pkgdefs/SUNWwiresharkr/pkginfo.tmpl > Add pkg version to the end of the DESC= line, eg. > DESC=""....... (n.n.n)" This package actually don't deliver Wireshark at all. Therefore it's better not to mention version. This will save us some time during upgrade to new version. > === End of comments ====== I also renamed applied patches to reflect Wireshark bugzilla numbers. Regards, Petr
