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. 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. 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). -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Computer Products
