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?
> 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?
> delete-service.py
> ---------------------------
> 85 - comment needs to be fixed
Any particular fix desired? (And I'm thinking you mean line 84?)
> 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
> 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?
> 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. 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 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
> 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 :)
