If I didn't comment consider your suggestion implemented.
Danek Duvall wrote:
> 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.
>
Yes I noticed this throughout the pkg stuff. Why spaces instead of tabs? spaces
are tiring. I changed them to be consistent.
> - 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!"
>
lol.
> 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.)
>
Is this a requirement? I may not have time to implement this.
> - 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.
>
Yes there currently defined like:
#define BE_ATTR_MOUNTED "mounted"
So I don't think there retrievable unless you know how to extract them?
> - line 76: why is a "beList" a dictionary?
Seemed the most appropriate type to use when creating the C wrapper and
extracting values passed by be_list. So all BE keys and values are one dict
entry and another BE's keys and values are another dict entry... Here's an
example of the dict:
[{'orig_be_name': 'BE3', 'space_used': 108032L, 'active': False, 'mountpoint':
'legacy', 'active_boot': False, 'mounted': False, 'orig_be_pool': 'rpool'},
{'status': False, 'mountpoint': 'legacy', 'mounted': False, 'space_used':
18432L, 'dataset': 'BE3/opt'}, {'orig_be_name': 'BE1', 'space_used':
1924565504L, 'active': False, 'mountpoint': '/tt', 'active_boot': True,
'mounted': True, 'orig_be_pool': 'rpool'}, {'status': False, 'mountpoint':
'/tt/opt', 'mounted': False, 'space_used': 18432L, 'dataset': 'BE1/opt'},
{'date': 1207154668L, 'policy': 'static', 'snap_name': '[EMAIL PROTECTED]'}]
So once I find BE3 I know I know all the other keys and values relate to that
BE.
>
> - 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]
>
I implemented enumerate() since I need all the values at a particular index as
shown above
> 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?
hmm. your right. if root = '/' then be_mountpoint = "legacy" so I needed to
check be_active for the '/' case. However if we run across '/' before the
intended target of the -R argument the this is wrong. Good catch!
>
> - 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.
>
Yeah the return codes need work in snap.
> - 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.
Yes it should be in libbe but didn'nt make it in time. :-(
>
> - line 182, 266, et al: *Definitely* needs wrapping.
>
> - line 192: Don't we need to call bootadm for a non-live BE, too?
The design was to only activate the live BE since it seemed less intrusive.
>
> - 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?
>
Thanks for the thorough review!
A webrev will be forthcoming after I test.
Tim
> Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
