Follow-ups in-line. John Fischer wrote: > 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: ... >> 86: INCLUDEDIR isn't defined anywhere, remove > > I'll remove it. However, it was already there before my changes. >
You'll find that my reviews can be expansive :-) If you feel I'm sending you far afield, push back, though usually I'll try to confine myself to simple issues that don't expand scope. >> 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. > Yeah, there's a marked lack of consistency in the existing code. >> 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. > .image_info is something we control, it's built in the Distro Constructor, so we can easily add parameters to it when needed. For this specific case it might make sense to just look for /platform/sun4u (sun4v) or /platform/amd64. I would expect those to be stable interfaces. >> 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. > Maybe you can discuss with Ethan and Clay a way to sort this out. Deferrable, I suppose, but I'll always start asking questions when I see hard-coded paths strewn around in code that has to be kept in sync with another part. >> 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. > OK. >> 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. > Yes, that would be my preference unless there's some reason it doesn't work. >> 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. > I'm not sure yet how we're going to want to classify that interface. I think we're going to want to have a look at the design here, as it's very divergent (and undesirably so, I think) between x86 and SPARC. Which, um, raises the question I didn't catch yesterday: what about SPARC? We don't have a menu.lst.01<mac> for them. Dave
