John Fischer wrote: > All, > > Updated webrev for this set of changes at: > > http://cr.opensolaris.org/~johnfisc/list > > These changes include feedback during a phone conversation from Ethan > as well as the installadm man page change to document the list > subcommand. >
General nit: changeset comments on all files need to be collapsed down to the canonical form: <bug number> <description> Note that there is no "-" in there. General issue: I'm not comfortable with the "-v" option added here; not clear on its purpose, exactly, and versioning of subcommands is most unusual. It raises all sorts of sticky questions about versioning other subcommands that I think we really don't want to get into. Similarly, the -h seems odd; why here, and not in create-service, create-client, etc.? What's the problem it solves? Makefile: 75: Don't comment out, delete line. 86: INCLUDEDIR isn't defined anywhere, remove 132: clean target seems to require update for PYTHON_EXECS installadm.c: 984: Why? snprintf() null-terminates. Seems like you could have used the pre-existing call_script() function (line 236) here, though. list.py: 93 et al: The doc comments seem too generally too terse. Some prose, perhaps? 95: This is an unreliable test, as multiboot is a deprecated system element (since the kernel was switched to direct boot) that may go away at any time and lead us to start seeing x86 images as sparc when they're not. Need a better test here, preferably something stable or something we control such as contents of .image_info (which may mean that the image builds need enhancement). 165, 216: We don't have a path definition somewhere that can be shared between the server and the tools? 180: Should it be "service" not "server"? 224: Won't this fail with some bad exception if maisql = None? 248: Whether there's always a default has been an issue of some contention in the past - do we need to handle it specially this way, or can we just let getManNames() provide it? 340: will this function work properly if users have edited menu.lst to have additional custom entries? Seems unlikely from my read. Dave > Thanks, > > John > > John Fischer wrote: >> Ethan, Clay and Sundar, >> >> When you get a chance can you review my webrev at: >> >> http://cr.opensolaris.org/~johnfisc/list >> >> These changes are for the installadm list subcommand fix and >> will address: >> >> 4330 - installadm list should show which server provide the install >> services. >> 4113 - nice to have an option to show what service the client is >> using >> 4298 - installadm list -n should give different output if there is >> no custom manifest >> 4597 - installadm list: expect usage statement when giving service >> name without "-n" flag. >> 4646 - list: showing added manifest for a non running service. >> 5300 - list: does not show informational message for non running >> service. >> 8496 - list: no verbiage indicating that a service does not exist >> when a non-existent service is given. >> 8529 - 'installadm list' command lists same service three times >> 6811 - list: should have similar output between list and list -n >> 8015 - list-manifests output is hard to read >> 9094 - installadm list: prints colons for empty MAC fields, and >> doesn't account for colons or periods in MAC field's width >> 4175 - install list error slips out >> >> The changed and added files are: >> >> usr/src/cmd/installadm/Makefile >> usr/src/pkgdefs/SUNWinstalladm-tools/prototype_com >> usr/src/cmd/installadm/installadm.c >> usr/src/cmd/installadm/installadm.h >> usr/src/cmd/installadm/list.py >> >> These changes also include a change to the Makefile and prototype_com >> file for the delete_service and delete_client to be consistent with >> the other python scripts within the /usr/lib/installadm directory. >> >> The remote service listing is being postponed due to a timing issue >> with multiple remote services that grows with the number of services >> within the domain. >> >> Thanks, >> >> John >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
