On 05/18/11 09:24 AM, Jack Schwartz wrote:
Hi Keith.
One residual issue from me... other items are resolved...
On 05/16/11 02:55 PM, Keith Mitchell wrote:
> usr/src/cmd/installadm/list.py
> ---------------------------------------
> 777: Please either add an "else: raise Exception" for the case
where the dbtable
> is not a profile or manifest table (preferred), or else change the
elif on 764
> to an else.
I'm not saying that's not a bad idea, but that's not from ISIM. It's
a programming error if it occurs, and the blanket "except
StandardError" wrapping the whole block makes it the raising of an
exception at that point interesting, but it should be enough to catch
programming errors in the future.
I didnt' quite follow... I get that it's a programming error if
dbtable isn't either a profiles table or manifests table, so I suggest
that either the elif on 764 is unnecessary (since the program is
correct only with one of these two tables anyway) or else flag the
condition that a different table was provided. IMO the best of these
two is to change the elif to an else and add a comment taht a profile
table is expected at that spot in the code.
So just to clarify what I was saying, the code is now (after addressing
your comment) structured like this:
try:
...
if table == <manifests>:
...
elif table == <profiles>:
...
else:
raise ValueError("<message about invalid table>")
except StandardError as err:
<handle error>
Because ValueError is a subclass of StandardError, the except clause at
the end catches it (this is why catching generic exceptions is generally
bad; it makes it difficult for the code to evolve - you can't raise
something new without wondering if it will be accidentally caught by an
overly-broad except clause). In this specific case, the error handling
is to "raise SystemExit(err)", so the end result is "close enough" - if
someone programs their way into that corner, the installadm invocation
will fail and hand off a message.
- Keith
Thanks,
Jack
- Keith
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
