Hi Clay.
Here's my_reply (your_reply (my_reply (your_review)))) :)
I deleted stuff I didn't have further comments on.
On 09/11/09 16:14, Clay Baenziger wrote:
> Hi Jack,
> Comments in-line.
> Thank you,
> Clay
>
> On Tue, 8 Sep 2009, Jack Schwartz wrote:
>
>
>
>
>> Have tried doing a build after removing delete-client from the gate?
>
> Yes all seemed fine, do you suspect a problem?
Nothing in particular; I thought I saw that file in your gate.
Sometimes a build will fail (packaging-related things, in particular)
after old files are manually removed.
>
>> /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.
OK, but a rule of thumb which I learned the hard way was that if there
are too many indentation levels to fit (in 80 spaces using tabs for
nesting levels) then the code can often be rewritten in a clearer way
(by breaking large methods into smaller ones, restructuring loops and
ifs, and so on).
>
>> 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?
Sounds good.
>> 89: I don't understand the comment.
>
> Does "return the MACAddress object and options object clarify better?
The comment currently says:
# return the MAC and options (will except if MAC is not valid)
I don't want to lose info with your change. Maybe you meant:
# return the MAC address and options except if the MAC address is not valid
Is this accurate?
>> 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
Yes, that clarifies. Thanks.
>> 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.
OK.
>> 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 :/
OK
>
>> 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?
How about "filename"? It has as many letters as "function" does...
>
>> 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.
XXX
>
>> 240: () helps make things clearer
>
> Remove the wanboot between the parens?
Never mind. If I run through all the possible cases
if not True and True
if not True and False
if not False and True
if not False and False
parentheses don't seem to matter! That is
if (not A) and B
if not (A and B)
yield the same result.
I'm sure this is an obvious thing to a math wiz, but it is fascinating
to me...
>
>> 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.
OK.
>
>> 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.
XXX
>> 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.
OK. Thanks for the explanation. Not sure if the directory will be as
empty as expected, though. I think what you say above is more accurate:
that it doesn't contain something not being removed. Won't the files to
be removed from it only have been appended to the "files" list but not
yet deleted? If not, then the len(listdir()) check won't be accurate,
right?
> Do you have a suggestion on a comment for this block?
>> 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.
If you think it helps clarity, then please leave it in.
>
>
>> 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.
OK.
>
>> 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
I suggest: "Resolve all links in baseDir to the paths they represent"
> paths=[os.readlink(l) for l in links]
> # ensure microRootGrub is not shared
> if (paths.count(microRootGrub) == 1):
Just checking: you only want to check links when checking that
microRootGrub isn't shared, right?
>
>> 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.
PEP8 (http://python.org/dev/peps/pep-0008/) doesn't say that.
PEP8 says "Use 4 spaces per indentation level" in the "Code lay-out"
section. To me an indentation level means nesting level, and does not
include continuation lines.
In the PEP8 document, look at the first "if" in __init__() under class
Rectangle, and you will see that its continuation lines are not indented
as much as the next nesting level (raise...). (They use 3 spaces for
continuations, though I would probably use two.) In the paragraph just
above that, it says "Make sure to indent the continued line appropriately."
Finally, even if the rule would be broken, the document says in the
section just past the introduction, that one good reason to break a
particular rule is "when applying the rule would make the code less
readable..."
Code is less readable when continuation lines and lines of the next
nesting level are indented the same. Yes, it's my opinion, but I'll be
you dinner next time I see you that others would agree with me :)
>
>> 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?
Put a colon at the end of 486. This should make things clearer, so that
even a person who's tired (as I probably was that day) can understand
the comment better.
>
>> (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.
I didn't find where PEP8 says you can't use 2 space continuations. See
above.
>
>> 478, 482: Can optimize with a single string for "rm." +
>> service.serviceName
>
> What's being optimized? I don't understand.
rm_script = os.path.join(baseDir, "rm." + service.serviceName)
os.chmod(rm_script, stat.xxx | stat.yyy)
com.run_cmd({"cmd" : [rm_script]})
>
>> 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.
Docs say it should be equivalent.
>
>> 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.
Thanks for enlightening me on this term :)
>
>> 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
OK
>
>> 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
OK. I wasn't sure, since 638 says "If we are the last service", which
to me implies that there were other services which had died or been
killed, and this is the last one waiting to be terminated. Maybe the
wording of 638 can be altered?
>> 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.
Well you do mention -x which is script specific... but OK.
I would reword the comment to say:
Call the SERVICE_DELETE_SCRIPT to delete the service. The script will
remove the image if the -x arg is passed to it.
>>
>> 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)
If the data gets "changed" when you initialize and when you write(),
then why not call loadData at those two times only, and just have
__getitem__() return self.data['fields'][key]) ? Seems much more
efficient, unless I'm missing something.
>
>
>
>
>> 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.
Hmmm... OK. This is probably splitting hairs, so don't worry about it
if too much trouble.
>
>
>> 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:"?
Thanks. That clarifies.
Thanks,
Jack
