[sfwnv-discuss] code review request for integrating sg3 utilities to opensolaris

[xiao li - Sun Microsystems - Beijing China]

Hi Amanda,
Please see my comments below.

Amanda Waite wrote:
> 1. usr/src/cmd/sg3_utils/Makefile.sfw
>
> - I suggest renaming install-sg3 to install-sfw to keep with the 
> general convention
Done.
>
> - test without the find $(VER) ... statements as they probably aren't 
> needed and just add
>   overhead to the build
Not quite understand this.
>
> - Why would you want to find and remove core files at the end of the 
> 'all' target
Removed, done.
>
> - don't hard code the path to the shell used to run the script. Use 
> $(SH) for /usr/bin/sh or
>    $(SHELL) for /usr/bin/ksh93
Done.
>
> - remove the 'test' target if you aren't going to use it
Done.
>
> - Do you need to specify the dependency on $(VER)/src with the 
> 'install' target?
No, removed.
>
> - Get $(VER) from METADATA file as suggested by Muktha
Done.
>
> 2. usr/src/cmd/sg3_utils/install-sg3
>
> - rename to install-sfw
Done.
>
> - Use ksh93 as described here::
>
>    Roland Mainz wrote:
>    > use /usr/bin/ksh93 or /usr/bin/bash for install-sfw*
>    > and add a
>    > $ set -o errexit # at the beginning and replace
>    > ". > ${SRC}/tools/install.subr" with
>    > "source ${SRC}/tools/install.subr" (the idea is to
>    > catch failures in the script and abort it at that
>    > point, right now the script will just continue)
>
Done.
> - '# /* Portions Copyright 2007 ShivaKumar GN */' <------ Should this 
> be there?
Removed.
>
> 3. usr/src/pkgdefs/SUNWsg3utilsr/Makefile
>
> - with 'DATAFILES = depend' your asking the build environment to 
> supply the depend file
>    which means you don't need to supply your own
Removed depend file for SUNWsg3utilsr
>
> 4. usr/src/pkgdefs/SUNWsg3utilsr/depend
>
> - This looks like the default depend file so you can remove it as long 
> as the Makefile specifies
>    'DATAFILES = depend' (which it does)
Removed.
>
> 5. usr/src/pkgdefs/SUNWsg3utilsr/prototype_com
>
> - I've seen that other packages are already doing this, but why would 
> you include
>   /etc/security/prof_attr and exec_attr as part of the package? Maybe 
> it's something I've missed
>   but even if it is allowed the way it's being done will just clobber 
> the existing files along
>   with any changes made by the user. With Lighttpd we added our 
> entries to the common files
>   as you have done, and these updates appear in the build in which our 
> packages appear. No
>   need to include them with the package.
According to PSARC 2008/683, sg3 utilities have to be RBAC compatible, 
so two new RBAC profiles introduced here.
Since I need to touch /etc/security/prof_attr and 
/etc/security/exec_attr, I have to separate the package into 2 parts, 
one for
"root"(for things under /) another is for "usr"(for things under /usr).
I've tested the package SUNWsg3utilsr SUNWsg3utilsu, and seems that the 
new profiles will be merged with the existing profiles.
So I think it will not "clobber" the existing files.
Thanks a lot for your comments,
-Xiao
>
> I'm new to doing in depth code reviews, but Paul's right we in Sun 
> need to do more of them so as to move the process along.
>
> Amanda
>
>
> xiao li - Sun Microsystems - Beijing China wrote:
>> Hi Experts,
>> I'm responsible for integrating sg3 utilities into opensolaris.
>> My code change is available at:
>> http://cr.opensolaris.org/~xl222276/sg3utils
>>
>> You comments will be highly appreciated.
>>
>> Thanks and regards,
>> -Xiao
>>
>>
>>
>> _______________________________________________
>> sfwnv-discuss mailing list
>> sfwnv-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/sfwnv-discuss
>>   
>