Hi Ethan,
Responses in-line. Thank you for the continuing refinement.
-Clay
>>> usr/src/cmd/installadm/Makefile
>>> ----------------------------------------------------
>>> 115 - Was there a particular need for this? package creation
>>> sets up the file perms anyway.
>> This was used out of the ai-webserver makefile which was copying the
>> auto-install directory's makefile. That's a good point this isn't
>> necessary. I've sent Jan an e-mail asking if he had any other reasons for
>> having done this in the auto-install Makefile. Further all the shell
>> scripts get their permissions changed in a post-processing step. Perhaps
>> this is all misguided chmod'ing goofiness?
>
> It would seem like it to me unless there was some special need to
> execute the just built binary from the source dir or proto dir
> itself as part of the build. (We used to have Yacc files, for
> example) But this doesn't seem to be the case here.
Nope, no reason like that. I've edited the Makefiles not to change the
permissions for python files in any of the three directories where that's
known.
>>> 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.
> 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
>>> ---------------------------
>>> 85 - comment needs to be fixed
>>
>> Any particular fix desired? (And I'm thinking you mean line 84?)
>
> I think I meant 95 -- misspelled word "kililng".
Thank you
>>
>>> 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.
>>
>>> 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).
>> 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.
>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.
>> 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().
>>
>>> 224 - Can fileName be left blank here instead? I know all the
>>> usage instances we've got pass in a particular name, but I
>>> just don't like hardcoded "rpool" in their. At a minimum, can
>>> you change this to "/boot/grub/menu.lst" instead.
>>
>> I can leave fileName unset but I'd like it to point to the default grub
>> menu location for now
>
> But that's not the default grub menu location. That's the default
> menu location for the particular pool named 'rpool'.
>
>> so it's documenting of what type of file the class is meant to be used
>> with. The reason I picked what I did was:
>>> ls -l /boot/grub/menu.lst
>> /boot/grub/menu.lst: No such file or directory
>
> This is likely a bug (its missing on my desktop too, but maybe
> installations via liveCD end up missing this?).
>
> On this machine, do a 'pkg verify SUNWgrub' You'll see the
> complaint.
Ok, I'll use /boot/grub/menu.lst being that's where it's supposed to be
>>> ls -l /rpool/boot/grub/menu.lst
>> -rw-r--r-- 1 root root 1077 Aug 25 11:05
>> /rpool/boot/grub/menu.lst
>
>
>>
>>> 229,237 - Cut n' pasted comments?
>> Ah yup, no VFSTabs were harmed in the use of the grubMenu class :)
>
> okay.
>
>
> thanks,
> -ethan
>
