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
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.
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.
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.
- 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.
- 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
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.
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""
- line 111: please delete
- line 152: I don't think you got the point of "don't use os.system()".
The point is to *not* use the shell. In addition, the command need
only be defined in one place, and should use an absolute pathname,
rather than relying on PATH.
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.
- 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.
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.
- line 290: "used" instead of "set"?
- line 293, et al: use "pass" instead of "return"
Thanks,
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
