Hi Clay.
Here are my comments.
usr/src/cmd/installadm/Makefile
-------------------------------
38: nit: indentation
87: remove the -g from cflags
Have tried doing a build after removing delete-client from the gate?
/usr/src/cmd/installadm/delete-service.py
-----------------------------------------
I believe style is modelled after cstyle: 4 spaces for a continuation
line, a
tab (8 spaces) for a new nesting level. DC was done this way, and a quick
inspection shows pkg was done this way too. Being that python depends
so much
on indentation since it doesn't use {} makes this even more important.
It will
make code more readable. I've made other comments about specific areas
in the
code to point out places where the importance of this shows.
Having code which is clear and consistent throughout a gate adds to the
quality
of the code.
65, 70: Can instantiate smf.AISCF(FMRI="system/install/server") once and
reuse.
81: Nit: "usage=usage" looks weird. I'd prefer string in 80 to have
different
name.
89: I don't understand the comment.
95: typo: kililng -> killing
100: Would an error message be helpful here? (Only if "service" can't be
anything other than an smf.AIservice)
116: This method doesn't raise any exceptions. SystemErrors are caught.
127: Message is clearer to me if you say "run the following on your DHCP
server:"
134, 142, 152, other places? : Rather than com.dhcpData() which allocates an
instance, why not use the class name directly, since you are calling static
methods. For example, line 142: nets = com.dhcpData.networks()
159, 162, other places?: I would avoid hardwired numbers. Numbers make
it hard
to see where changes need to be made if one changes what that number
represents,
and is thus not maintainable.
Nit: Since the comments on 128 and 172 link their two commands
together, you
could define a list containing the parts of the command, and reuse it in
both
places. Not sure if this is over the top though...
177: redundant return
183: Comment can be clarified. How about: "If requested for removal and not
in use"
184: 2nd part of this comment is redundant with the previous line
190-196: Not sure if you want to mention which files are for SPARC and
which for
X86, since we decided files for both platforms will be marked for deletion.
203 and other places: Nit: "fn" is quite cryptic for a variable name
203: Seems like either removeFile should march on when an error occurs (for
more complete processing), or else the callers to removeFile should stop
processing when removeFile encounters an error. The current solution is not
complete on errors.
211: typo: listidr -> listdir
220: hardwired false
Please mention that rmtree will ignore errors because of that false.
checkwanbootconf: comment misleading. Says delete file, but only builds
list
231, 232, 237: returns can be removed
240: () helps make things clearer
245-246: Logic here is very confusing. Please change the first "dir" to
another name-- how about "deldir"-- to distinguish it from the other dir.
264, 288: Debugging would be easier with an error message here.
279: I would like to see names for field indeces, instead of hardwired
numbers.
This makes the code less prone to breakage if the fields change for some
reason.
290: "Check if we're tasked..."
291: if (not removeImageBool):
294: Comment incorrect. Not looking for txt_record here.
340-344 Please explain the logic here.
359: As discussed in person, no need to assemble every field of the line to
delete if the mountpoints are all unique. (A side effect of this is that
comment lines containing the mountpoint may be deleted as well.
Shouldn't be a
problem, right?)
385, 390 redundant returns
414: filesToRemove.append(curPath)
419: Are you sure that microRootGrub is not an absolute path and that
its path
starts from baseDir?" If so, please add a comment. If not, calling join()
here is inappropriate.
422-427: If I understand this line correctly,
- it lists all files under baseDir
- it forms their absolute pathname
- if a pathname is a link, it adds it to a list l
- it counts how many items in the list are the microRootGrub string
- it checks that the count is one
This line does too many things. Please split this line up, else someone
will
have a hard time understanding it.
426-427: indentation.
438, 443-444, 490, 495: indentation.
474: I believe calling /tftpboot/rm.<service-name> will remove some
files but
not those which get appended to filesToRemove list (which get removed
later).
Is this correct? Whatever the relationship between the filesToRemove
list and
the files in /tftpboot/rm.<service-name>, please add a comment
explaining it.
(This sort of situation is why cstyle indents 4 spaces on continuation
lines and
8 spaces for new nesting levels.)
477: Code below this line sets the rm script permissions to rwx-r--r--
478, 482: Can optimize with a single string for "rm." + service.serviceName
499: Add a comment that this is the start of the remove_files() method.
507 - 545: per verbal discussion, removal of both sparc and x86 files
will be
attempted, correct?
552: Please clarify that a "function" here returns a list of files to remove
554: apply() has ben deprecated since 2.3.
565: What is hrm?
582, 675: Would having an error message here be appropriate for debugging
purposes?
629, 659: as discussed, SIGKILL may be too abrupt. A milder signal like
SIGTERM
or maybe SIGINT would be better as it would allow cleanup.
641: How does service.instance.services get decremented? If this happens as
processes are terminated, we might need to call waitpid() to wait for the
os.kill of 629 to take effect before checking service.instance.services.
647: does this code need to be put into a try/exept similar to the block
at 616?
679: Add a header to note this is the start of the program.
installadm.c
------------
898-899: The comment doesn't reflect that delete_service terminates all
processes related to a service, as well as delete the image and files
associated with it.
Also, 899 is its own sentence.
934: You can just say
if (delete_image) {
installadm.h
------------
OK
installadm_common.py
--------------------
Not sure how much this file will change when its class is reimplemented as a
child of dict, but here's a fast review of this file.
85: A lot of work gets done in this routine, for it to get called over
and over
again to reprocess the same data / file (I'm thinking __getitem__() does
this.)
Can you check at the beginning whether self.data['lines'],
...['rawFields'] or
...['fields'] exists, and just return if things are already set up?
(Another
option: you can add an arg to force a reload, in case you need to see
changes
in the file.)
87: begining -> beginning
91-92: nit: comment can be one line.
102-103: Can be optimized: the same line gets split() twice.
182: delimitered -> delimited
185, 190: delimeter -> delimiter
258 please put a comment explaining that you are splitting on the string
"\ntitle". Or else move the comment from 275 to there.
334: symblos->symbles
377: From comments below this point, I think it is the command that produces
the stuff on 378-387. Please clarify that it is the command that
produces that,
not the function.
386: The "a" at the end is superfluous, right?
416: "Run a command given by a dictionary, check for stderr output..."
437: Remove the comment here about DHCP cleanup. This function doesn't
have to
do with DHCP cleanup.
installadm_util.c
-----------------
OK
setup-service.sh
----------------
OK
/usr/src/lib/... : Not reviewed
Thanks,
Jack
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
