Clay,
What is the status of the regression test run for this wad?
I would like to see the results.
My review comments ....
usr/src/cmd/installadm/Makefile
----------------------------------------------------
115 - Was there a particular need for this? package creation
sets up the file perms anyway.
usr/src/cmd/installadm/installadm.c
---------------------------------------------------------
181 - Combine this new comment with previous comment line.
And can you augment the comment with why we shouldn't be
enabling the install smf service in this case.
182 - strcmp() doesn't return a boolean, compare this to == 0
instead.
182 - Is there a defined string to compare this to here instead?
182 - What you're checking against doesn't match the comment.
delete-service.py
---------------------------
85 - comment needs to be fixed
105 - The message that gets written looks really lean and scary;
could use some more info. I suggest something like:
"Service configuration info not found ...\n"
173 - I thought it was clear to you that we decided we're
not doing this with this wad.
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?
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.
145 - I think Drew already commented on this last Friday
but creating a more generic "fileTable" or some such class
for both the vfstab and mnttab class to inherit would be good.
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.
229,237 - Cut n' pasted comments?
thanks,
-ethan
Clay Baenziger wrote:
> Hello all,
> I've got a webrev for some mighty delete-service changes. The bugs
> I'm hoping to address are:
> 4526 - delete-service is not deleting service as described in section
> 4.3.2 ai_design_doc
> 6587 - delete-service shouldn't remove the source image if there's
> other
> services actives 'linked' to the same source image
> 10740 - Need way to interact with SMF from Python for installadm
> components in Python
>
> Further, this webrev includes the already reviewed (but as yet
> unpushed and related libpyscf code). The files to ignore (unless
> you're interested) are anything under usr/src/lib/*
>
> Otherwise, if I could get initial comments as soon as possible,
> however, Eric has extended the expected timeline for these changes to
> be pushed to next week opposed to the initial hope of Friday.
>
> Webrev:
> http://cr.opensolaris.org/~clayb/delete_service/
>
> Thank you,
> Clay
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
