Clay Baenziger wrote:
> Hi Ethan,
> Thank you for reviewing this! Some comments in-line, anything you
> sent not mentioned below, I'll be fixing.
> Thank you,
> Clay
>
>
> On Wed, 9 Sep 2009, Ethan Quach wrote:
>
>> 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.
>
>> 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?
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".
>
>> 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?
>
>> 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 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?
>
>> 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.
> 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.
>> 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
