On Thu, Apr 03, 2008 at 01:08:08PM -0600, Tim Knitter wrote:
> http://cr.opensolaris.org/~tsk/863/
Makefile:
- please keep the list sorted
client.py:
- lines 293,294: 7-space indent?
- line 350: Please use the named constant from imageplan.py. In fact,
just use it down on line 414.
bootenv.py:
- Please follow the indentation used everywhere else -- eight spaces.
- I'm going to chime in with my dislike of the virtualization you've done
here. We haven't really established a pattern for this yet, but here's
my proposal:
snax.py:
try:
import scooby
except ImportError:
pass
class Snax(object):
def eat(self):
print "Aww, man, I ain't got no Scooby snax!"
class ScoobySnax(object):
def eat(self):
print "Wow! That's some good Scooby snax!"
if "scooby" in locals():
Snax = ScoobySnax
eatsnax.py:
from snax import Snax
snax = Snax()
snax.eat()
will be happy if the "scooby" module exists, and will probably just go
toke some more if it doesn't. (You can say "import os as scooby" to
test out the success case.)
- line 30: are these constants defined in libbe, too? If so, I'd extract
them from that, rather than having to keep two lists in sync.
- line 76: why is a "beList" a dictionary?
- line 90: follow Dan's advice here. Note that if you do, you can get
rid of self.beList. But I would also prefer if you weren't iterating
over a list, checking each value, but rather just index straight into a
dictionary. In other words, something like:
try:
thebe = be.BEs[root]
except KeyError:
return
self.be_name = thebe[be_name]
self.dataset = thebe[be_dataset]
and so on.
- I'm a bit confused by why you have the if clause on 91 at all. If I'm
reading it correctly, if we run across the active BE, we end up
ignoring the fact that we were asked to create an object based on
"root". Is that correct? Why is that right?
- line 91: I think "if be_active in self.beList[idx]:" is more natural
here.
- line 106: use auto-splitting here:
err, self.snapshot_name = ...
Though I would suggest that a return value of None is more idiomatic
than a separate return code.
- line 121: I'd skip setting this here, and just do so in
init_image_recovery(), where I'd also use tempfile.mkdtemp(), which
gets around the isdir() mess.
- line 148: A directory with mode 0644?
- line 153, 184, et al: You need to narrow your except.
- line 154: Please wrap the line. You can break the string into bits:
"foo" "bar" is equivalent to "foobar".
- line 177: Please use subprocess.call(). os.system() is just as
dangerous as system(3c). Though I'm confused why you have some of the
activation here and some of it in libbe.
- line 182, 266, et al: *Definitely* needs wrapping.
- line 192: Don't we need to call bootadm for a non-live BE, too?
- line 208: I think the message could be less technical -- "clone" and
"debugging" aren't really necessary. Perhaps "The BE where the upgrade
failed is mounted at /blah, if you wish to inspect it."
- line 254: this looks familiar. Refactor?
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
