Clay,
I have some additional comments on the updated webrev ...
usr/src/cmd/installadm/installadm.c
-----------------------------------
889 - Upon successful creation of an install service, it's
enabled by default isn't it? So we shouldn't need the call
to check_for_enabled_install_services() here it seems...
946 - Don't we need to check to see if we just deleted the
last service somewhere around here, and if so, disable our
smf service? Or is this logic somewhere else that I'm missing?
usr/src/cmd/installadm/installadm_common.py
-------------------------------------------
36 - What's the significance of using the name "Unix" here?
65 - In this method, if newlines = False, and blanklines = True,
what will get returned at 107 when processing a blankline in
from a file? An empty string '' ??
198-200 - So it seems this caching doesn't save any effort on rereading
the entire file from disk, but just contents processing?
Could we cache based on modtime of the file or something
like that?
usr/src/cmd/installadm/delete-service.py
----------------------------------------
413 - I dislike how you have to visit an additional time just
to check existence. I thought there existed some NO_IDX
exception when searching a list. If so, you can then just
wrap 415 in a try
try:
idx = vfstabObj.fields.MOUNT_POINT.index(boot_archive)
try:
# remove line containing micro root
del(vfstabObj.fields.MOUNT_POINT[idx])
except IOError, e:
sys.stderr.write(str(e) + "\n")
except NO_INDEX_OR_WHATEVER_THE_EX_IS, e:
sys.stderr.write (_("Microroot for service %s " +
"not in vfstab.\n") % service.serviceName)
(Ideally, I would have like to have seen the caller not
need to handle the idx at all, but I won't push for that
at this point.)
424 - It would be more useful if the error message could also
include the value of what wasn't found in the vfstab.
i.e. boot_archive
453 - This doesn't look correct. The manpage for in.tftpd
does not say that 'homedir' is the argument to the -s
option. In fact, based on the synopsis, it looks like
'homedir' can float.
SYNOPSIS
in.tftpd [-d] [-T rexmtval] [-s] [homedir]
So it seems you have to parse fields.SERVER_ARGUMENTS,
looking for the argument that's not an option, and not
an option argument.
468 - misspelled "Directory"
474-478 - I really think we should leave it up to the caller
to check this, so that the interface of findTFTProot()
is defined as always to return something, with the default
being /tftpboot
(Looking ahead at your code, the callers already do this
checking)
500-501, 688-689 - Umm, can you please make findTFTProot()
cache its answer then. The chances of someone changing
/etc/inetd.conf in between time is negligible afaic.
And actually, if you do my previously suggested comment,
you can remove the if()'s at 500 and 688.
usr/src/cmd/installadm/delete_client.py
---------------------------------------
41 - this comment isn't needed anymore.
43 - is this comment still accurate?
usr/src/pkgdefs/SUNWinstalladm-tools/prototype_com
--------------------------------------------------
45-48 - Is there a benefit for doing this symlinking? Why don't
we just deliver two binaries, delete-service and
delete-client?
thanks,
-ethan
Clay Baenziger wrote:
> Hi again all,
> I got a way to get webrev to work for me to do a differential
> including delete_service.py (still known as delete-service.py in the
> differentail webrev). See:
> http://cr.opensolaris.org/~clayb/delete_service/webrev2.diff2/
>
> Thank you,
> Clay
>
> On Wed, 23 Sep 2009, Clay Baenziger wrote:
>
>> Hi again all,
>> Well the addition of a thousand lines of code and 50+ pages of
>> comments I think I've got this re-spun for everyone's enjoyment,
>> please see:
>>
>> http://cr.opensolaris.org/~clayb/delete_service/webrev2.diff/
>> (Unfortunately I can't get webrev to track that delete-service.py
>> became delete_service.py (so that it can be imported by delete_client))
>>
>> Or full webrev:
>> http://cr.opensolaris.org/~clayb/delete_service/webrev2/
>>
>> The bug list has grown to include:
>> 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
>> 8666 create-service: prints out SMF messages no matter what's going on
>> 8773 create-service followed quickly by delete-service hangs
>> 10740 Need way to interact with SMF from Python for installadm
>> components
>> in Python
>> 11292 delete-client: should remove SPARC clients too
>> 11486 delete-service/delete-client: should check inetd.conf for
>> tftp root
>>
>>
>> To Drew:
>> --------
>> To address the ps(1) pain, I consolidated the function down and filed
>> 11524 - Should look to using PSI (Python System
>> Information) for Python process management
>>
>> I looked into Bill's bootadm work but I don't fit an "alternate root"
>> environment and I'd still need to provide a lot of parsing anyways.
>>
>> To Sundar:
>> ----------
>> I think our phone call Thursday cleared up your questions?
>>
>> For those in the code walk-through:
>> -----------------------------------
>> I chose to append our findings of being able to have both a SPARC and
>> X86 client to a bug on create-client rather than address finding all
>> possible nooks for a client and spewing lots of not found messages to
>> a user (or having to catch the messages in funky ways).
>>
>> Jack:
>> -----
>> Per the agreement between Drew's coding style suggestions, those of
>> PEP8's hanging indents and Google's Python style guide I've followed
>> PEP8/Google's Style guide, however, I hope next Tuesday we'll have
>> time to come to a consensus on Python style there as this should
>> expand past this one push, of course. Thank you for getting me to
>> think about this so much!
>>
>> Thank you,
>> Clay
>>
