[caiman-discuss] Code Review for Delete Service work

[Clay Baenziger]

Hi Jack,
 	Comments in-line.
 					Thank you,
 					Clay

On Tue, 8 Sep 2009, Jack Schwartz wrote:

> Hi Clay.
>
> Here are my comments.
>
> usr/src/cmd/installadm/Makefile
> -------------------------------
>
> 38: nit: indentation

Thanks, will fix.

> 87: remove the -g from cflags

Yes, I'll nondebug the bits, thank you for catching this.

> Have tried doing a build after removing delete-client from the gate?

Yes all seemed fine, do you suspect a problem?

> /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.

I agree, however, we fly in the face of the Python community which 
strongly mandates the use of PEP8. Unfortunately, with some of my 
indentation levels I would be taking 48 of 80 characters following 
C-Style. I've pinged Ethan and Eric, and plans are afoot to migrate our 
gate to follow PEP8 space based indentation and 4 space indents. Thank you 
for catching this and I'm sure there will be more discussion on this.

> 65, 70: Can instantiate smf.AISCF(FMRI="system/install/server") once and 
> reuse.

Ok, this is done later in the code via service.instance... However, it's a 
small cost to instantiate upon need. Shall I just do:
smfInstance = AISCF(...) and use that for the two calls?

> 81: Nit: "usage=usage" looks weird.  I'd prefer string in 80 to have 
> different
> name.

Sure, I'll go through and fix this.

> 89: I don't understand the comment.

Does "return the MACAddress object and options object clarify better?

> 95: typo: kililng -> killing

Thank you, I wish aspell worked...

> 100: Would an error message be helpful here?  (Only if "service" can't be
> anything other than an smf.AIservice)

No, as it can be a dictionary (when we do delete-client) and so we just 
don't want to operate with that. Should I change the comment to:
# if we are not handed an AIservice instance return None as delete-client 
# runs will pass in a dictionary instead which we have no use for

> 116: This method doesn't raise any exceptions.  SystemErrors are caught.

Ah thank you, that is vestigial. Gone

> 127: Message is clearer to me if you say "run the following on your DHCP
> server:"

Yes, it is more clear.

> 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()

Thank you for catching my useless instance creation -- I'll use the class 
instead.

> 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.


I'd like to agree, but don't see another way to do this. All references 
are local to the block (e.g. 159 I want the first argument passed in -- 
there is no symbolic name I can use) and on 162 I want the second object 
of the tuple handed to filter -- again no provision for a symbolic name 
that I know of.

> 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...

I'll see since 172 needs to print the same message now

> 177: redundant return

Yup, will be gone

> 183: Comment can be clarified.  How about: "If requested for removal and not
> in use"

Sure, I'll add that in

> 184: 2nd part of this comment is redundant with the previous line

Line 187? I'm all for removing 187.

> 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.

I think I should point out which architecture each relate to since has 
become a spec for what files end up where post AI install :/

> 203 and other places: Nit: "fn" is quite cryptic for a variable name

I'll accept that if you can come up with a good variable name for filename 
which is reasonably short?

> 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.

Why is this not complete? It keeps processing on errors as do its callers 
and callees.

> 211: typo: listidr -> listdir

Thank you

> 220: hardwired false
> Please mention that rmtree will ignore errors because of that false.

Will do

> checkwanbootconf: comment misleading.  Says delete file, but only builds list

Ah yes, good point

> 231, 232, 237: returns can be removed

Ah yes the big list o' returns

> 240: () helps make things clearer

Remove the wanboot between the parens?

> 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.

Good point

> 264, 288: Debugging would be easier with an error message here.

Again these aren't errors so I'll clarify the error message the same as 
for line 100.

> 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.

Unfortunately, split doesn't let one name each split record. I can update 
the comment above to give an example txt_record string (i.e. "split on : 
as the string txt_record should be hostname:port"), however, to give a 
better comprehension to a reader what is being provided.

> 290: "Check if we're tasked..."

Ah yes a conditional makes that more readable

> 291: if (not removeImageBool):

Sure

> 294: Comment incorrect.  Not looking for txt_record here.

Yes, I'll change txt_record to image_path.

> 340-344 Please explain the logic here.

This iterates down the directory structure adding any directory which 
doesn't contain something not being removed to the list of objects to 
remove. (E.g. if we start with dir =
/var/ai/image-server/var/ai/clayb_ai_sparc, then we see if 
/var/ai/image-server/var/ai/ has anything other than clayb_ai_sparc in it, 
and if /var/ai/image-service/var/ has anything other than ai, etc. The 
/var/ai/image-service directory will have the various image-server files in 
it and will break the while loop condition. Do you have a suggestion on a 
comment for this block?

> 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?)

Yes, I'll just look for lines with the mount_point field set to the 
mount point we're deleting. No comment lines won't match this regex or be 
included in lines returned otherwise.

> 385, 390 redundant returns

Okay. That we return after 385 is a bit buried by lines 386-389 though. 
Let me know if you think that's still clear enough to remove the return on 
385 as the redundancy isn't wrong and I think helps clarify.

> 414: filesToRemove.append(curPath)

Ah yes, thank you.

> 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.

It will work if microRootGrub is absolute just fine:
>>> os.path.join("/test","/bob")
'/bob'
>>> os.path.join("/test","bob")
'/test/bob'

This is up to the symlink but the added logic of calling 
os.path.isabs(microRootGrub) seems redundant.

> 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.

Instead, how's:
 	# get all files in baseDir
 	files=[os.path.join(baseDir,f) for f in os.listdir(baseDir)]
 	# get all links in baseDir
 	links=filter(os.path.islink, files)
 	# get all paths in baseDir
 	paths=[os.readlink(l) for l in links]
 	# ensure microRootGrub is not shared
 	if (paths.count(microRootGrub) == 1):

> 426-427: indentation.

I'm not sure what's wrong here, I was following PEP8 which says to use a 
four space indent regardless. I'm trying to understand how best to proceed 
with our gate being traditionally a mangled Sun C-Style for non-C code.

> 438, 443-444, 490, 495: indentation.

Again as above

> 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.

So, the files which rm.<service name> should remove are talked about in 
the lines 487-496. How should I better draw attention to that?

> (This sort of situation is why cstyle indents 4 spaces on continuation lines 
> and
> 8 spaces for new nesting levels.)

I agree that is a nice feature of Sun C-Style, a two space indent for 
continuation would have been a better thing for PEP8 to have specified I'd 
think.

> 477: Code below this line sets the rm script permissions to rwx-r--r--

Ah yes, that's what I meant to do, I'll fix the comment on 477 to reflect 
the truth.

> 478, 482: Can optimize with a single string for "rm." + service.serviceName

What's being optimized? I don't understand.

> 499: Add a comment that this is the start of the remove_files() method.

Yes! Thanks rather hard to see otherwise with all the nested functions in 
the way.

> 507 - 545: per verbal discussion, removal of both sparc and x86 files will be
> attempted, correct?

For delete-client invocations it was either this or running delete-client 
on a create-client run (to prevent having the same client defined for 
multiple architectures). I don't have a definitive agreement on this. I 
think removing for both architectures will be more readily implementable.

> 552: Please clarify that a "function" here returns a list of files to remove

Yes

> 554: apply() has ben deprecated since 2.3.

Yes, I wasn't sure I could do object(service) but I believe that should be 
okay.

> 565: What is hrm?

Haha, think Dave's deep sigh. I'll change the comment to:
error -- as we do not know what this object is -- and this suggests severe 
confusion on the part of the filesToRemove list.

> 582, 675: Would having an error message here be appropriate for debugging
> purposes?

No as for reasons above. The comment will clarify this isn't an error 
condition but just not applicable

> 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.

Indeed

> 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.

service.instance.services doesn't change value, it's simply a list of AI 
services register with SMF -- nothing to do with processes

> 647: does this code need to be put into a try/exept similar to the block at 
> 616?

Yes, thank you for seeing this

> 679: Add a header to note this is the start of the program.

Can do

> 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.

This is intentionally as a comment here is not likely to be updated with 
code in delete-service.py, I'd rather just point to delete-service.py and 
let the developer, etc. follow the trail -- nasty but the cost of 
splitting into multiple pieces until it's all one language.

> Also, 899 is its own sentence.

I'll be getting rid of 898 as per my response to Evan.

> 934: You can just say
>       if (delete_image) {

It looks like the style through out much of installadm.c is to compare 
against B_TRUE even though it isn't necessary.

> 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.)

I can check if a file.read is different from the data last read (a partial 
caching approach)


> 87: begining -> beginning

Thank you

> 91-92: nit: comment can be one line.

Will one line it

> 102-103: Can be optimized: the same line gets split() twice.

Optimized at the potential cost of memory and garbage collection. Let me 
weigh the choice or see if I can understand the Python internals a bit 
better.

> 182: delimitered -> delimited
> 185, 190: delimeter -> delimiter

Thank you

> 258 please put a comment explaining that you are splitting on the string
> "\ntitle".  Or else move the comment from 275 to there.

Ah cool, can do

> 334: symblos->symbles

Haha, wow, my spelling was getting a bit creative.

> 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.

The command returns a string like:
client id     flags    client ip    server ip    lesase expiration   macro 
comment\n
\n
01001B21361F85  00    172.20.24.228    172.20.25.12   08/21/2009 
dhcp_macro_clay_ai_x86\n
[...]

The function takes that string and produces a list after split is run. 
Should I say for line 377:
"After split we have a list like:"?

> 386: The "a" at the end is superfluous, right?

Yes, any :wq's are too ;) I'll grab that

> 416: "Run a command given by a dictionary, check for stderr output..."

Ah articles do make things easier to read

> 437: Remove the comment here about DHCP cleanup.  This function doesn't have 
> to
> do with DHCP cleanup.

Ah thank you, it's indeed vestigial

> installadm_util.c
> -----------------
>
> OK
>
> setup-service.sh
> ----------------
>
> OK
>
> /usr/src/lib/... : Not reviewed