Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#260 closed enhancement (fixed)

More suggestions to bindctl

Reported by: shane Owned by: shentingting
Priority: low Milestone:
Component: ~bind-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket:
Total Hours: Internal?: no

Description

I looked at ticket #220, and had a few more suggestions to bindctl.

First, we should document that when have a variable named dir that it must have os.sep (usually '/') at the end.

Second, if _get_saved_user_info has an error of some kind, it should probably report it, rather than silently returning empty information. This sort of behavior can be very frustrating for users!

I don't know if it is critical or not, but it might be safer when writing a file to write to a temporary file and then rename it to the "real" file. That way if there is an error (like a full disk) we are never left with a partially-written file.

Finally, what does the traceback.print_exc() output look like? I would prefer not to give users output that is for developers, but I don't know how this looks.

Subtickets

Attachments (2)

bindcmd.diff (1.6 KB) - added by shentingting 9 years ago.
new_bindcmd.diff (2.2 KB) - added by shentingting 9 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 9 years ago by zhanglikun

one suggestions from Jelte: Avoid using "Exception" to catch all errors.

comment:2 Changed 9 years ago by jelte

Let me add to that for a minute, having something that catches any exception is not necessarily a bad thing (and sometimes it's a very good thing), however, losing stacktraces certainly is. For programs like bindctl, that are run from the command line, i personally don't care if it aborts with a stack trace, and i certainly prefer that above printing a simple error (that does not show what exactly went wrong there) and running on.

So I think that a good rule would be that, in Python, if we put in a catch-all (except Exception), we *always* print or log the full stack trace. If we 'expect' a specific exception and know how to handle it, we should check for that specific exception, and not a general one.

For c++ this might be a bit harder (since there's no portable way to get the stacktrace), but the problem there might be less bad since our own exceptions can already provide source file and line.

Changed 9 years ago by shentingting

comment:3 in reply to: ↑ description Changed 9 years ago by shentingting

  • billable set to 1
  • Internal? unset
  • Owner set to shane
  • Status changed from new to reviewing

Replying to shane:
I did a little change according shane`s suggestions#2, #4.See the attached: bindcmd.diff

  1. traceback.print_exc() replaced by specific output information.
  2. Report error produced in _get_saved_user_info().

For #1, I think this is a good idea. All dir variable named have a document that ended with os.sep. For #3, This is useful for large file, First write data to a temporary file, and then write to real file. But for bindcml, the size of the information file is small. It only includes user name and password information. It will not generate partially-written file. So I keep the code unchanged.

comment:4 Changed 9 years ago by shentingting

  • Owner changed from shane to jelte

Hello jelte!

Now, Shane is on a holiday! Could you help me review the changed code ? I

made a little change for the code. thanks!

comment:5 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to shentingting

One error; the last change there prints 'err' but the variable name of the exception is 'e'. Other than that this fix looks fine.

but, this also shows my original problem (which it does not fix), which is in bindctl-source.py.in; there's a statement 'except Exception' there, and while nice for users, it's a problem for developers since the printed message there does not show *where* the error occurred.

I agree with Shane that we should not show users stacktraces, but we (as developers) certainly need them. I think we *should* print full stacktraces for problems we really do not expect (i.e. the places we put a catch-all like 'except Exception'). And we should not get there for problems we do anticipate (like the user-friendly changes made above, which I approve).

Best of both worlds may be do a full stacktrace, only in the catch-all, and only if verbose is set (though there is no verbose option right now, it should be easy to add)

Changed 9 years ago by shentingting

comment:6 in reply to: ↑ 5 Changed 9 years ago by shentingting

Hello jelte:

I changed it again. Change a 'except Exception' to specific exception. So please help me review it again. If it is OK, please help me merge it into trunk. Thanks.

Replying to jelte:

One error; the last change there prints 'err' but the variable name of the exception is 'e'. Other than that this fix looks fine.

but, this also shows my original problem (which it does not fix), which is in bindctl-source.py.in; there's a statement 'except Exception' there, and while nice for users, it's a problem for developers since the printed message there does not show *where* the error occurred.

I agree with Shane that we should not show users stacktraces, but we (as developers) certainly need them. I think we *should* print full stacktraces for problems we really do not expect (i.e. the places we put a catch-all like 'except Exception'). And we should not get there for problems we do anticipate (like the user-friendly changes made above, which I approve).

Best of both worlds may be do a full stacktrace, only in the catch-all, and only if verbose is set (though there is no verbose option right now, it should be easy to add)

comment:7 Changed 9 years ago by jelte

Ok this looks good and I'll merge it, thanks :)

(though I think we should still consider having a -v option and print stacktraces then)

I will slightly tweaked the text of the error messages.

comment:8 Changed 9 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

Committed in r2681, didn't think this needed a changelog entry, if people disagree, let me know.

Closing ticket.

comment:9 Changed 9 years ago by stephen

  • Milestone feature backlog item deleted

Milestone feature backlog item deleted

Note: See TracTickets for help on using tickets.