Ed, Thanks for your comments. My responses are in-line.
Edward Pilatowicz wrote: > On Tue, May 13, 2008 at 09:59:22AM -0600, Jerry Jelinek wrote: >> I have posted a webrev at: >> >> http://cr.opensolaris.org/~gjelinek/webrev/ >> >> for bug: >> >> 6553514 native zone svr4 pkg code should be moved into zone callbacks >> >> This is a refactoring of the code so that it is >> easier to support different brands going forward. >> In particular this will help with the IPS support >> for the Indiana release, although it will also >> benefit any distro that wants to use brands to provide >> zone support. >> > > some initial comments starting with all the easy stuff. > ed > > ---------- > usr/src/Makefile.lint > usr/src/cmd/zoneadm/Makefile > usr/src/cmd/zoneadm/zoneadm.h > usr/src/lib/libbrand/common/libbrand.h > usr/src/lib/libbrand/common/mapfile-vers > usr/src/lib/libzonecfg/Makefile.com > usr/src/lib/libzonecfg/common/mapfile-vers > > - ok > > ---------- > usr/src/head/libzonecfg.h > > - nit: for consistency please remove "handle" from > zonecfg_dev_manifest(...) Done. > ---------- > usr/src/lib/brand/native/zone/Makefile > > - nit: instead of: > include ../../../../cmd/Makefile.targ > how about > include $(SRC)/cmd/Makefile.targ Done. > - why do you need -I$(SRCDIR)? > (there are no .h files in usr/src/lib/brand/native/zone/.) Removed. > - isn't _REENTRANT defined by higher level makefiles? No. > ---------- > usr/src/lib/brand/native/zone/config.xml > > - should /usr/lib/brand/native/postclone be folded into sw_support > for consistency? Done. > - nit: perhaps you could delete all the empty tags? Done. > ---------- > usr/src/lib/libbrand/dtd/brand.dtd.1 > > - given that we have postattach and predetach, perhaps we should have > preattach instead of attach and postdetach instead of detach? I don't want to change this since they mean different things. Also, it is possible that there are some other brands we don't know about which might be using the existing hooks, even though this is a private interface. > - could the comments be explain some of the differences in system and > zone state between when > - the attach and postattach callbacks are invoked? > - the predetach and detach callbacks are invoked? > > - the attach comment only talks about the -F argument, but the man page > also documents -u and -n. are those arguments ever passed to the > callback? if so, instead of documenting individual arguments in > the comments perhaps they comment should just reference the > zoneadm man page? I reworked many of the comments based on your feedback. > - given that native is not becomming a brand, does this clone comment > make sense anymore? > If no hook is provided, the internal zoneadm cloning code will be used. I am not sure I understand what you mean about "native not becoming a brand". I do want the comment in the dtd and I added a similar comment to the other hooks where there is internal handling when a hook is not provided. This is new behavior with this change. > - i'm confused. you seem to be introducing hooks that no brands is > using: > clone > uninstall > are theses going to be needed for IPS? I don't know what we'll need for IPS yet. This change is for all of the sw-related handling in zones so that we can support install (which includes attach/clone) and uninstall of a zone. Thus, I added all of the hooks related to sw management for zones. > - please update the comments to describe how the return codes are > handled (or ignored) for all the new callbacks. I did that as part of the comment rework mentioned above. Thanks again for your input. I am rebuilding and retesting with these changes. Once that is done, I'll post an updated webrev. Jerry _______________________________________________ zones-discuss mailing list firstname.lastname@example.org