Dan Price wrote:
> 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.
>
OK however I described it on 42. I changed the name to recoveryEnabled.
>
> 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".
>
OK
> 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
Yes the index is needed to make sure all related items from the dictionary are
parsed as a whole. Cool I'll use enumerate().
> routine. I think you can reverse the sense of the comparison to
> make things a little more whitespace-efficient.
>
yep.
> 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
>
>
ok
> Your various error messages should be printed to stderr.
done.
>
> I loathe the use of <angle brackets> to surround data items in messages.
> Can we please stop doing that?
removed.
Will send out another webrev after addressing Danek's comments,
Thanks for the time Dan!
>
>
> -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
>
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
