Paul Cunningham wrote: > Hi Sagun, > > See additional comments inline below ... > > Otherwise it looks okay to me > > Paul > > sagun shakya wrote: >> >> I've address your comments and generated a new webrev. Please see my >> responses to your comments inline. >> >> Webrev for Paul's comments: >> http://cr.opensolaris.org/~sagun/libpcap-postreview/ >> >> Webrev against sfw gate: >> http://cr.opensolaris.org/~sagun/libpcap-sfw/ >> > > ... cut ... > >> Paul Cunningham wrote: >> >>> === Start of Comments === >>> >>> 1. METADATA >>> Your don't seem to have a METADATA file for your new package. >>> >> >> I hadn't seen this in existing libraries in SFW but I had noticed the >> METADATA file in the current code reviews. However, I missed >> including the file. I've added it. I wasn't sure what exactly needed >> to go into the METADATA file so based it on what was in libdvdnav. >> >>> 2. usr/src/cmd/nmap/Makefile.sfw >>> Is there a reason why you have removed the explicit >>> '--prefix=' from 'configure'? >>> >> >> Restored bask as it was before. >> >>> 3. usr/src/lib/libpcap/Makefile.sfw >>> You don't seem to have explicitly defined '--prefix=' on >>> 'configure' (I think it's normal SFW practice to do so). >>> Also you could do this using the predefined stuff in >>> Makefile.master, see example at >>> http://cr.opensolaris.org/~rayx/erlang/webrev/usr/src/cmd/erlang/Makefile.sfw.html >>> >>> >>> >> >> Fixed to used predefined stuff. >> >>> Why are you using '-I/usr/local/include' ? >>> >> >> This is an error. I've fixed it. >> >>> You have copied various .c and .h files into the src, >>> could these have been patches to the original (in the >>> tarball) instead? >>> >> >> Do you mean a patch to the libpcap0.9.8 tarball upstream? If so, no. >> Elaborating a bit more - all the changes patch/new files have been >> integrated upstream in the main libpcap gate and the libpcap1.0 tree. >> Since we do not have a confirmed date on when libpcap1.0 is coming >> out I'm patching libpcap0.9.8. Does that answer your question? > > What I was try to say was: > > You have copied the following files ... > 81 cp libdlpi.patches/dlpisubs.c $(VER)/. > 82 cp libdlpi.patches/dlpisubs.h $(VER)/. > 83 cp libdlpi.patches/pcap-libdlpi.c $(VER)/. > so if these are in the tarball why have you copied them rather than > patched them. But from your comment above it sounds as though they are > 'new' files - so that's okay then. It may be clearer to add a comment > to say that though. > Yes, these are 'new' files. I"ll add a comment in Makefile.sfw. > > Also in Makefile.sfw, you have run 'autoconf', is there a reason for > running this - it's not normal (I think) to have to do this - if you > need to maybe you need to add comment to say why. There are changes made to configure.in so new configure scripts need to be generated. I don't think a anything else will generate a new configure script. So I'll add a comment there as well. > > And just as an after thought, you might want to consider using > compiler flag ... > > Roland Mainz wrote: > > > > the "-xstrconst" puts all string literals into > > read-only memory (e.g. it's shared between processes and > > won't waste memory then). > OK.
Thanks, Sagun -- Sagun Shakya 781.442.7344/ X27344 sagun.shakya at sun.com
