Dave, Thanks a bunch. I'll get a new webrev out for most of the issues.
There are a few things still outstanding like how to determine the architecture, how to cobble together the AI.db path, default manifest controversy ;-) and client information via menu.lst. All your points are addressed below. Thanks again I know it took you a while. John Dave Miner wrote: > 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? Removed. Understood. > Makefile: > > 75: Don't comment out, delete line. Doh. I usually remove stuff like that before code review. Missed it. Removed. > 86: INCLUDEDIR isn't defined anywhere, remove I'll remove it. However, it was already there before my changes. > 132: clean target seems to require update for PYTHON_EXECS Yeap. Thanks. Good eye. > installadm.c: > > 984: Why? snprintf() null-terminates. Seems like you could have used > the pre-existing call_script() function (line 236) here, though. That's funny. Only 1 other subcommand calls call_script() with all the others doing an snprintf() and a installadm_system() call. It is much more simple to do it this way and already tested. It will now use call_script(). Thanks. > list.py: > > 93 et al: The doc comments seem too generally too terse. Some prose, > perhaps? Will beef up. > 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). If there is another way that this can be done now then I'll do it. I just don't know of another way. .image_info only has IMAGE_SIZE and GRUB_TITLE. So if this is something that we want to capture we need to make the enhancement up stream. There is also the .release file but I would presume that we do not control that either and it is probably not-an-interface. > 165, 216: We don't have a path definition somewhere that can be shared > between the server and the tools? Could be but I am not aware of anything. It is not stored in the smf properties. Does anyone know if it is stored elsewhere? Currently list-manifests uses what is passed in on the command line and adds AI.db. list-manifests gets the path from a #define within installadm.h which points at /var/ai + port. Then list-manifest does the os.path.join() method to add AI.db to the path. So it does not appear that it is stored or gotten elsewhere. > 180: Should it be "service" not "server"? This is an error condition on the server itself. The service pointed to via the smf txt_record property may not exist for some reason on the server. I wanted to catch that case rather then simply dieing with a python stack trace. > 224: Won't this fail with some bad exception if maisql = None? Yes. It does with an AttributeError exception which is why I catch the condition around line 180. It should either have try/exception or the else for the os.path.exists() line. (by the way I did not know try/exception existed in python when I first started this exercise--I'll use it for getting the database instead of the exists() method). > 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? Right. So this is one of those things that I changed recently from feedback during a phone conversation. It originally did not have the default manifest code in it but simply relied on getManNames(). So there is still some contention around it. What is your suggestion? It sounds like you favor getManNames() and don't worry about listing the default manifest. > 340: will this function work properly if users have edited menu.lst to > have additional custom entries? Seems unlikely from my read. Unfortunately, this is one of those cases where we do not have a good solution. The client information is not stored anywhere within the system except in the grub menu.lst.01<MAC_ADDRESS>. As you point out the contents are simply ascii which can be edited by a system administrator. The code would only get the first menu item if there was another added. I was under the assumption that this file was a private interface and was not to be edited by them. Is there a more reliable method that can be used? If not then the way that the code is written once a new method is designed it would be trivial to modify the code to get the tuple (service, path, arch) and possible the client. > 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 > >
