Hi Keith,
Thank you for the sanity-check. More below and to address some
comments which came up in talking with you on the phone and in e-mail:
1) The doctest implies one must overload open() to use WANBootInfo(). To
prevent any confused reading of the doctest, I've explictily commented
that overloading open() is only for testing. Further, I've tried using
open("/some_file_which_does_not_exist") however, doctest seems to
execute in Python restricted mode so open()/file() are not available.
3) For those who might belive a class for "querying" wanboot.conf might
also allow writting, I've made an explicit comment that the class does
not allow writting. I tried setting __setattr__ to None but that caused
other issues which I could not eaisliy resolve.
Thank you,
Clay
On Fri, 30 Jul 2010, Keith Mitchell wrote:
[snip...]
All my prior comments on shell scripts have been addressed. All comments
below are from ai_sd.py, which seems to have gotten rather complicated
suddenly.
120,129: Should this be... "method"? (Why is method listed at all? It's right
above the line, and any pydoc parser should have a reference to the method
name)
This was a carry-over I didn't pay it much attention to; but, indeed these
are misspelled and redundant. I've removed them.
405, 418: platform.processor() should provide a more parseable result,
particularly for sparc.
Ah yes, indeed. Thank you, I'll use processor().
469-509: Would probably be best to move this doctest to a separate PyUnit
test. It's a bit verbose to read which significantly reduces its value as a
test-that-also-documents-usage - ditto for the __getattr__ function's
doctest. Given what's being tested - the opening of a file - there may not
really be a need to test this. I trust that the implementation of "open" in
Python works.
Ah yes, but the API usage to understand what err and f are and when is
useful to know IMO. If anyone doesn't want to see it, even vim supports
code folds to hid the doctest - let's keep code and their tests in one
place for now.
558-559: Between these lines, and the previous function, that's a lot of
effort to: open a file, catch the error, tell the caller there was an error,
then have the caller re-raise an error. Let Python's builtin exceptions do
what they're made for, and just open the file directly (using a "with" block
to automatically close it):
with open(a_file) as the_file:
my_data = the_file.readlines()
# process my_data, etc.
#(File closes automatically at end of 'with' block, much like a finally
clause)
Yes, but this keeps the actual implementation of the service discovery
code consistent be it SPARC or X86 (lines 400-423) which I think is more
important. (I'd rather abstract out the differences and present a
consistent API than have architecture specific checks for IOError, etc;
here if this class has an error it will propagate it and the calling code
can then report it as it wishes.
563-564, 576: A __getattr__ function that reads a file twice is very
expensive for attribute access. I appreciate the magic of Python used here,
but given the limited usage of this class, it seems a bit expensive. A simple
method (rather than a class), that reads the file and returns a dictionary of
the key/value pairs found in the file would probably be far more efficient
and valuable. If it makes it more natural to store that data in a class that
has a "read from file" function, then that'd be ok too.
If a full fledged class is required, perhaps the data could be cached in
__dict__, and only re-read the file if the file has been updated since the
last read. (os.stat(filename).st_mtime)
Yes, this is not very efficient of me; I've simplified the function,
however, I still believe cacheing is a bit overkill since at this time the
function is only called once anyways.
Additionally, regardless of implementation, use str.partition rather than
split. It's a far 'safer' function for cases of the form "key=value", as it
will always return a tuple of length 3. Also, is it truly technically invalid
to have multiple entries of the same name in a wanboot.conf file?
bootconfchk(1M) seems to accept it, so for consistency, if it's valid, we
should as well.
Hrm, that's an interesting question. I hadn't realized that it would
accept multiple values. For the places we use this, it can certainly cause
us issues. (For example multiple root_server keys causes my fix to
net-fs-root to try to download solaris.zlib, et al. multiple times too).
As we generate the wanboot.conf file at this time; this is a potential
situation but one I see as very unlikely. I've filed a bug 16751 -
"Multiple value per key in wanboot.conf causes AI clients confusion" to
track this.
580: If this stays as a class, please wrap this into the class definition
(setting file=None, and then setting wanboot_conf =
WANBootInfo.default_wanboot_conf when file is None should do it). Also, use
"file_" rather than overriding the builtin "file" (similarly, for readability
above when creating a file, use "file_" - or a more descriptive name - rather
than "f").
I've added a doc string mention that the file still acquires a value
despite being left as None. Certainly since Drew too stumbled on the
out-of-class indentation level init() I think you're right, this
should stay with the class.
590/600: Perhaps just create a function?
This is to keep parity across SPARC and X86 for discoverying properties;
so WANBootInfo and DHCPClientInfo are reasonably similar in API.
655-680: Expanding on Drew's comments on use of Popen - if it raises an
OSError, that means a required program (dhcpinfo) is missing; if it's missing
than either there's something corrupt on the image/system, or there's an IPS
dependency error. Both are situations that the ai_sd can't do anything about;
so the best course of action is to propagate up (at the highest level, e.g in
the if __name__ == __main__ block, all exceptions should be logged and
printed to the user with as much information as possible to assist in
evaluating the root cause). Masking the OSError is a bad idea - let it show
through, as that will be easier to diagnose if the situation ever arises, and
has far richer data than the DHCPError will.
No doubt there are some good ideas here to aid in debugging. However, the
code is already structured to use an AI_LOG device and report Popen
OSErrors this way. But you have a very valid point I should pass the
OSError's message through to aid in diagnosis (does dhcpinfo(1) not exist,
have restrictive permissions, etc.?).
I still believe that this should be catchable as coming from the
DHCPClientInfo class, as if the consumer isn't requiring the DHCP info, it
may wish to simply note and continue; if it wishes to propagate it as long
as the necessary info is there it can pass it along to the user (as
ai_sd.py does). I think it's good hygiene for the class to report that it
had an issue, not potentially cause a higher-level consumer to believe it
had an issue (i.e. each layer should catch its own errors -- follow an
encapsulating design).
For ValueError, it should only arise in one of two cases: programming error,
or passing in a dynamic (e.g. user-selected) value. In the former case (which
is the case for the usage scenarios in this file), again, masking with a
DHCPError would make it hard to debug; in the latter, it would be acceptable
to catch, but only by the portion of the program "in control of" the dynamic
value.
I appreciate your concern about ValueError being a programming error not a
run time issues, and as ValueError can only be returned under 2 cases from
subprocess.Popen() -- and we aren't going near them -- I've pulled that
exception check out of both the new and old code.
Overall, the original implementation was much cleaner; really, it just needed
to check the retcode of the Popen object and assert that stdout returned a
usable value (and raise an exception as needed).
No, unfortunately, that was not possible. My original version was not
modular enough -- we needed to be able to pull functionality out to test
it.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
