On Thu 03 Apr 2008 at 01:08PM, Tim Knitter wrote:
> OK. I have another cut of the webrev at the same location which implements
> Krister's suggestons:
>
> http://cr.opensolaris.org/~tsk/863/
>
> Please provide comments.
bootenv.py
75 self.is_BEenv = False
The meaning of this member variable is not obvious-- a comment
about what it represents, or a better name, would help.
79 # Need to find the name of the BE were operating on in
order to
80 # create a snapshot and/or a clone of the BE.
Nit: "we're" or "we are".
Line 90: Couldn't this be a lot cleaner as:
for ibe in self.beList:
If you really need the index, too, you can use python's enumerate
routine. I think you can reverse the sense of the comparison to
make things a little more whitespace-efficient.
Also, I think there is excessive vertical whitespace. Seems like this could
be:
for ibe in self.beList:
if ibe.get(be_active):
self.is_live_BE = True
if not (ibe.get(be_mountpoint) == root or
self.is_live_BE):
continue
# Set the needed BE components so snapshots
# and clones can be managed.
self.be_name = ibe.get(be_name)
self.dataset = ibe.get(be_dataset)
# Let libbe provide the snapshot name
snapRet = be.beCreateSnapshot(self.be_name)
# Check first field for failure.
# 2nd field is the returned snapshot name
if snapRet[0] == 0:
self.snapshot_name = snapRet[1]
else:
print _("pkg: unable to create an auto
snapshot.")
print _(" pkg recovery is disabled.")
return
self.be_name_clone = self.be_name + "_" + \
self.snapshot_name
self.clone_dir = "/tmp/" + self.be_name_clone
self.is_BEenv = True
break
Your various error messages should be printed to stderr.
I loathe the use of <angle brackets> to surround data items in messages.
Can we please stop doing that?
-dp
> Thanks
> Tim
>
> Tim Knitter wrote:
> >
> >
> > [EMAIL PROTECTED] wrote:
> >>>> It took me a minute to figure this out. The way this is implemented is
> >>>> kindof sneaky. I had to notice that every function starts with a call
> >>> OK maybe sneaky, but the intent wasn't to be.
> >>
> >> Not accusing you of malicious intent. :)
> >>>> to get_env() and get_env() checks is_BEenv. It might just be easier to
> >>>> check is_BEenv directly inside the methods. There are a couple of
> >>>> other
> >>>> possible approaches too:
> >>> Yeah I wasn't sure what the OO convention was, wrap a variable in a
> >>> method or just access it directly. If it is most conventional to
> >>> access the member directly I'll do that.
> >>
> >> My take is that if you're the class, it's okay to access the variable
> >> directly. If you're outside the class, you may want to use the accessor
> >> function. Python has some additional functionality that lets you do
> >
> > Right. I initially thought some of the methods might be called from
> > outside the class but since none are, I'll access the class variables
> > directly.
> >
> >> neat things here, but I don't know if any of that is applicable.
> >>
> >>>> 1. Create an abstract BootEnv class, define BootEnvZFS and BootEnvNull
> >>>> classes. When we can import libbe, use the BootEnvZFS class, otherwise
> >>>> use the BootEnvNull class. Presumably, the null class has methods that
> >>>> don't actually do any work.
> >>>>
> >>>> You could also just make the parent class have null methods, and then
> >>>> have BootEnvZFS do work. There are a few different ways you could go
> >>>> with this general idea. Stephen/Danek may want to say more about this,
> >>>> too.
> >>> Each of these are great ideas but wouldn't the first one create
> >>> duplicate code?
> >>
> >> Not exactly. The idea would be that you have the same methods defined
> >> for each class, but only BootEnvZFS would contain code that does any
> >> work.
> >>
> >> I like the progress that we're making and I'd like to get a couple of
> >> other reviewers to sign off on your wad too.
> >>
> >
> > Yes me too especially before I send out what is hopefully the last
> > webrev. ;-)
> >
> > Tim
> >
> >> -j
> _______________________________________________
> pkg-discuss mailing list
> [email protected]
> http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
--
Daniel Price - Solaris Kernel Engineering - [EMAIL PROTECTED] - blogs.sun.com/dp
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
