Clay Baenziger wrote: > Hi Ethan, > Responses in-line. Thank you for the continuing refinement. > -Clay > >>>> usr/src/cmd/installadm/installadm.c >>>> --------------------------------------------------------- >>>> 182 - Is there a defined string to compare this to here instead? >>> >>> I don't believe so. Like 73 uses just the quoted string too. Should I >>> add this in installadm.h? >> >> Yes please. Either do that or better yet move the whole call into >> the separate subcommand-functions that would indeed require a kick >> to the smf service. For example, do the "list" or "help" subcommands >> really require an attempt at smf enablement -- *after* executing no >> less? > > Yes, I can try to fix that - didn't realized that about help and list, > yuck.
If you do decide to fix it this way with this wad, make sure you run full regression tests on all subcommands then. > >> The latter request here is probably the root cause of some other >> bugs like 8666 so I'm okay if you don't fix that here. But at >> least do the former. >> >>> >>>> delete-service.py >>>> --------------------------- >>>> 173 - I thought it was clear to you that we decided we're >>>> not doing this with this wad. >>> >>> Yes, I'll take any code out which performs action and replace it with >>> print's >> >> Okay. Can you briefly describe how you're going to address, or >> should I just wait for the updated webrev? > > Essentially what's on line 129, but with the warnings printed from lines > 140-168. okay. >>>> 685-704 - This really bugs me. Could we have created one as a >>>> subclass of the other, or both as a subclass of a more generic >>>> class? > > This is on-going but I've sent you an intermediate webrev for you to > review this restructuring (anyone else is welcome to see, if you ping me). Are you talking about client vs service here, or the DBFile thing? My comment here was on the former, and I don't see any restructure of that in the intermediate webrev you've sent me from one night ago. > >>> This would go away when we have installadm(1) written in Python to >>> call the appropriate functions and do option parsing. Until then I >>> can roll thins up to be more dynamic in one function, however, I >>> think it would obscure what's going on to someone looking at the code >>> down the line. This keeps it obvious there are two ways this file may >>> be called. >>> >>> Do you have a specific way you think this could be cleaned up and >>> kept clear? >> >> Do you think that in the long run that a client won't need >> its own object definition? >> >> Even if what you mention above happens, you'd still be using >> what would seem to be service specific functions to handle client >> instances. All of these methods take service objects as their >> argument, no? > > No, they take a AIservice object or a dictionary masquerading as one. This is where I see things could really break down. For example, at line 692, if its a client instance you set serviceName to "01<MAC>" because you "know" later on the serviceName is going to be processed in particular ways in the various functions. In one function example, remove_DHCP_macro(), lines 118-123, the function ends up interpreting serviceName to figure out if it should do "service" work or "client" work, which really are the two types the caller new about anyway. If we're really sticking with delete logic being in these methods in delete-service.py, then make the lines between service and client cleaner, not hidden, with interpretation logic spread about in the caller and method and well. > From reviewer comments it seems comments need to be addressed to > clarify this, however, as initially these were written with only > services in mind. > >>>> installadm_common.py >>>> ------------------------------------- >>>> 134 - Does this mean the vfstab is written out headerless? >>>> That's really icky of so. The vfstab class doesn't seem to >>>> handle preserving commented lines (and their locations) >>>> in the vfstab either. I know we're in a zfs world but that's >>>> really valuable to some ppl. >>> >>> This writes out whatever someone hands in as data. >> >> Okay, I see how you're doing this now. So users of the vfstab >> class have to understand that they should only call the write >> member function using the data output of dump, regexed through >> however they want to alter the content, using direct access to >> a preloaded tabled copy of the content for content comparison? >> >> That seems like rather a strange thing to provide. >> >> Why not just hide all this impl detail and provide a member >> to provide this functionality for them? Removing a vfstab >> entry based on some col=val seems like a good thing to provide. >> In our case here col is mountpoint (I don't care if you don't >> genericalize out the specification of which col is being used >> on at this point) >> >> def remove_entry(self, mountpoint): >> >> or something along those lines. > > This is not C code ;) I'll try to implement removal of a key for the > mount point and raise notImplementedError on anything which can't be > handled. okay. thanks, -ethan > >>> I don't use it to write out headerless, comment free data but if one >>> was nasty they could -- this is just a function to wrap file.write(). >>>
