=====================================================
CODE REVIEWER: [EMAIL PROTECTED]
WEBREV: http://cr.grommit.com/~meem/wifi-0926
FILES: GLDv3/WiFi build and packaging
NOTES: Description of feedbacks:
ACCEPT Request accepted
REJECT Request rejected
EXPLAIN Explanation given
DISCUSS Request requires further discussion to resolve
DEFER Request deferred (e.g. because work is out-of-scope)
=====================================================
Venu, thanks for the review. Our actions and rationale follow; please let
us know if you have any additional questions.
usr/src/cmd/dladm/Makefile
L32: Does this mean that aggregation.conf is now going to live in
/etc/dladm. Does it currently reside in /etc? If so would
this cause any issues on existing S10 updates that already
has aggregation.conf in /etc? I know this won't/shouldn't
be hand-edited, but could there be any scripts etc. that
reads this file?
| EXPLAIN. The file is project-private and has always contained notes
| to that effect. The move has been approved by PSARC/2006/517, which
| has Patch binding. However, this code review is only for Nevada.
L34: It is taken out because POFILE is in Makefile.master, right?
| EXPLAIN. Yes.
L40: Owner is dladm?
| EXPLAIN. Yes, the dladm user is needed to keep keys secure in a way
| that minimizes risk of privilege escalation; see sections 2.6-2.8 of
| dladm-wifi.pdf.
L61: Just curious is $(ROOTCFGDIR) needed? Should
$(ROOTCFGDIR)/%: $(ROOTCFGDIR) %
$(INS.file)
work? (just checking since I seen something similar in another
Makefile).
| DISCUSS. How is that different from what's there today? If you're
| asking whether the INS.dir rule for $(ROOTCFGDIR) is needed: it either
| has to be here or in $(SRC)/Targetdirs (or both). Will ACCEPT the
| request to move it to $(SRC)/Targetdirs, if that's being requested.
usr/src/lib/Makefile
L454: I think this is fine, i.e. explicitly stating the dependency,
but given L485, do we still have to mention libdevinfo here?
| REJECT. No; make(1) handles recursive dependencies automatically.
usr/src/lib/libwladm/Makefile.com
L38: Do we need to list libsocket and librt in the dependency list
in usr/src/lib/Makefile?
| ACCEPT. Not strictly necessary given the current .WAIT's, but still
| a good idea.
usr/src/pkgdefs/SUNWckr/prototype_i386
[Haven't read any docs] do we deliver anything for SPARC?
| EXPLAIN. Nope; see question 6 in PSARC/2006/406's 20 questions.
usr/src/pkgdefs/SUNWcnetr/postinstall
Same question as L32 in usr/src/cmd/dladm/Makefile
| EXPLAIN. Same answer :-)
L29 Can we just remove all the comments? Could we miss some
specific comments because of this?
| ACCEPT. There shouldn't be comments in this file other than the
| copyright because it's not user-editable. We didn't use mv(1) because
| we'd have to chown/chmod afterwards -- however, cat(1) will preserve
| both comments and owner/permissions, so we will change L29 to:
|
| cat $ORIG > $BASEDIR/etc/dladm/aggregation.conf
usr/src/pkgdefs/SUNWcnetr/prototype_com
L50-53 : Not sure why 'dladm' and not 'root' (for owner).
| REJECT. See section 2.8 of dladm-wifi.pdf for why dladm is used.
Shouldn't postinstall be listed as a packaging file "i postinstall"?
| ACCEPT. Good catch.
usr/src/pkgdefs/SUNWcslr/prototype_com
no comments (don't we deliver 64-bit?)
| EXPLAIN. Correct; libwladm (like libdladm) is Consolidation Private
| and ON currently has no 64-bit applications that need it. If the
| need arises, it will be added.
usr/src/pkgdefs/common_files/i.ftpusers
I am not sure what is being done here (perhaps I should read the
document 8)).
| EXPLAIN. See sections 2.6-2.8 of dladm-wifi.pdf.
usr/src/pkgdefs/common_files/i.group
not sure what has changed here.
| ACCEPT. We had applied a change here but it was rendered unnecessary.
| Will remove this file from our list prior to integration.
usr/src/pkgdefs/common_files/i.minorperm_i386
L119 Just looking at the changes (and L243) do we need an entry for
"clone:e1000g"
| REJECT. This is intentional; make_chattr_list() applies permission
| changes to entries that are not currently in /etc/minor_perm.
| However, clone:e1000g is already in $SRC/uts/intel/os/minor_perm.
usr/src/pkgdefs/common_files/i.minorperm_sparc
Similar questions as for i.minorperm_i386.
| ACCEPT. Here, we're adding e1000g to $SRC/uts/sparc/os/minor_perm, so
| we need to list clone:e1000g in make_chattr_list() so that any incorrect
| existing permissions get fixed up.
|
| In addition, we will also remove the erroneous addition of `ath:*'
| that we noticed on subsequent review of this file.
usr/src/pkgdefs/common_files/i.passwd
L116 : Do "15" & "3" have any significance?
| EXPLAIN. The `3' is the GID for `sys', which is the group for the
| dladm user. The `15' is not inherently significant; any unused UID
| less than 100 (in the system space) would do as long as we used it
| consistently.
L124-124 : This results in adding
"dladm:x:15:3:Datalink Admin:/:" follwoing the enrty
for nuucp? or if nuucp is not present the line gets
appended at the end?
Just curious why nuucp?
| EXPLAIN. This is the (lame) way the existing algorithm works -- it
| inserts each entry and the entry that precedes it. Note that webservd
| inserts gdm and webservd, and gdm inserts listen and gdm. In our case,
| nuucp is the entry in the passwd file before dladm, based on UID.
usr/src/tools/scripts/bfu.sh
L2135 Please add some comments here. So, I think you determine
if this is new dladm (for aggregation.conf) by checking
for /etc/dladm dir.
| ACCEPT. Will add the following comment:
|
| # Check whether the archives have an /etc/dladm directory; this is
| # later used to determine if aggregation.conf needs to be moved.
L6259 Again, should we just ignore all comments?
| ACCEPT. Will change to use cat(1), as per the discussion of
| pkgdefs/SUNWcnetr/postinstall above.
L6264 Here we don't bother about comments.
| EXPLAIN. As above, the comments were not a consideration; we were just
| trying to avoid needless chmod/chown calls. Here, the cp will cause the
| file permissions to be preserved and the owner to be set to `root'.
| However, we still need to set the group to sys to match the packaging
| for /etc/aggregation.conf. As such, we ACCEPT a change to add
|
| chgrp sys $rootprefix/etc/aggregation.conf
usr/src/uts/common/Makefile.files
usr/src/uts/common/Makefile.rules
I suppose your comment about some WiFi drivers not part of the
initial putback affects the entries here, right? I mean do your
comments relate to pcan, pcwl, ipw2100 etc.?
| EXPLAIN. Correct. The other drivers will be removed from these files
| prior to integration.
usr/src/uts/intel/ath/Makefile
just curious why was this dependent on drv/ip earlier?
| EXPLAIN. Previously, WiFi ioctl handling was done by ath, which needed
| mi_mpprintf_putc() (which resides in drv/ip). It's now done by the
| net80211 kernel module.
--
meem
_______________________________________________
networking-discuss mailing list
[email protected]
