Danek Duvall wrote:
> On Fri, Apr 04, 2008 at 04:21:58PM -0600, Tim Knitter wrote:
>
>> http://cr.opensolaris.org/~tsk/863.2/
>
> Makefile:
>
> - line 132: get rid of the blank line
>
done
> client.py:
>
> - line 275: now that you have a dummy BootEnv object, you don't need to
> have any if tests here. If the initial object creation fails, just
> create a BootEnvNull object. You can call the methods on that without
> ever having to check before doing it.
>
I tried to do that initially inthe constructor but it didn't work since the
class was already instantiated in client.py. Of course I didn't try it in
client.py. :-(
> You could even have the BootEnv constructor return a BootEnvNull object
> on failure, but that's trickier, and might not allow for the right
> error handling semantics.
>
> - line 363: you're still not pulling this from imageplan.
>
whoops missed it. got it now.
> bootenv.py:
>
> - line 69: nit: beList needn't be an instance variable, as it's only ever
> used in the constructor. And only once, for that matter.
>
right. removed
> - line 72: Note that you're not actually communicating failure back to
> the caller this way, so when you *do* test for recoveryDisabled, it'll
> be false, and you'll be making calls on an improperly initialized
> object.
>
the try: exception: has been deleted.
> - line 82: I still find this logic confusing. You're making the checks
> completely orthogonally to how I'd do them. Here's what I'd expect:
>
> if beVals.get("mountpoint") != root:
> continue
>
This won't catch if the mounpoint is "legacy" and root is '/' and we'll never
get to the meat of the constructor.
> if root == "/" and beVals.get("active"):
> self.is_live_BE = True
>
> That is, skip this item in the list entirely if it's not what was asked
> for. Once we do find the one we asked for, if it happens to be mounted
> at / and is active (though I wonder how the two could ever not always
> be both true or both false at the same time), then set is_live_BE to
> True.
>
it's the legacy value that you may be missing.
We have to make one special test for when root = '/' and mountpoint is
"legacy". The below checks if we're working on '/' and if were are then it's
mountpoint could be "legacy" and but it will allways have an True "active"
field.
if root == '/' and beVals.get("active"):
self.is_live_BE = True
> But your check is slightly different (rewriting to use !=):
>
> if beVals.get("mountpoint") != root and not self.is_live_BE:
> continue
>
> which says that if the item is not the BE we're looking for, but *is* a
> live BE, don't continue -- we're going to snapshot this one anyway.
>
> Like yesterday, I don't think that logic's correct, but maybe I'm
> missing something.
>
> - line 106: no string exceptions; they're archaic, and going away. This
> can even just be "raise RuntimeError, "recoveryDisabled""
>
ok
> - line 111: please delete
yeah my eyes are glazed.
>
> - line 152: I don't think you got the point of "don't use os.system()".
I thought you said to use subprocess? I'd appreciate an example if this
subprocess command isn't right. thanks
> The point is to *not* use the shell. In addition, the command need
Using the shell was the only way it worked on my indiana release. I tried many
different alterations but none worked.
> only be defined in one place, and should use an absolute pathname,
The command is different thus two definitions. One uses clone_dir and the other
root
> rather than relying on PATH.
of course
>
> What happens if the call succeeds, but the command fails? You don't
> check the return code. You also don't give any indication of what the
> failure was. If an exception was raised, what were the details that
> might help the end-user diagnose why there was a problem and take steps
> to correct it?
>
> - line 157, 163, 168: it seems strange to me to unconditionally not pass
> error conditions back up to the caller. You've effectively hard-coded
> policy here. Fine for now, I suppose, but you'll need to revisit this.
>
ok
> - line 206: The error message is a bit strange -- it makes me think that
> the clone is the untouched copy of my bits, rather than the location
> where the upgrade was attempted. (I guess a similar comment could be
> made about the success message on line 174, too.) I think I'd say
> something reassuring about your running bits not being touched.
> Perhaps something like (remember that it'll be preceded by a message
> from client.py about the upgrade / install having failed):
>
> The running system has not been modified. Modifications were done
> only to a clone of the running system. This clone is mounted at
> /blah should you wish to inspect it.
>
yeah that is better.
> Dan's usually pretty good at coming up with good wording for things
> like this. But I wouldn't worry about it too much for now. There's a
> lot of wording I think needs some massaging.
>
I changed it anyway. ;-)
> - line 290: "used" instead of "set"?
>
ok
> - line 293, et al: use "pass" instead of "return"
right.
Thanks
Tim
>
> Thanks,
> Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
