Brian "Krow" Aker (krow) wrote,
Brian "Krow" Aker
krow

How To Review A Bug

Last week I and Monty gave a short talk at the MySQL Lead's Meeting in Stockholm on "How to review a bug".

I converted the talk over to a PDF file for those who are interested:
http://krow.net/images/howtoreviewabug.pdf

Here is the outline:

Bad Ideas


  • Suggest to run --record for mysql-test-run for anything that breaks.
  • Disable Tests within a Bug Fix.
  • Both Fix bugs and clean up code in the same patch.
  • Ignore new code that you don’t understand during a review.
  • Discourage developer from writing test to prove that the bug was fixed.
  • Prefer changes that require huge changes to all tests, they were probably broken.
  • Never ask from help from the developer who wrote the original code.

    Methodology


  • Read bug report, find out what user’s problem was, and read the description of the developer’s fix. Read also comments from other developers to verify if the bug really is a bug or if there is a better fix.
  • Scan patch for test case, compare to user’s problem.
  • Look to see if patch was minimal change (read comments in commits on file to see if “additional” fixes were done).
  • Look to see if all new variables that are allocated are properly de’allocated.
  • Look for orphaned variables/functions.
  • Check that all return values are properly handled, or declared VOID()
  • Look at all lock additions and changes in mutex. Make sure lock order is preserved (release first what you last locked).
  • Make sure that “bk collapse” has been used so that only one changeset is pushed.

    Reviewer responsibilities


  • Reviewer need to understand that he is responsible for the quality of the patch
  • Reviewer should understand every single change made
  • In doubt, require more comments
  • There should be a test case that ensures that the reported bug is fixed
  • For a production/beta code review, strive for minimum changes
  • If old test results changes, discuss this with another senior developer
  • In doubt of solution, discuss things with a senior developer
  • If you are first reviewer, read comments from second reviewer

    Tools to use


  • For non trivial changes, require that patch is tested with valgrind
  • All new code should be tested; In doubt, require that everything is tested with dgcov and that purecov comments are used.

    Things to watch out for


  • Memory allocation (Malloc is generally bad); Ensure that all memory is initialized and freed.
  • Question all ‘cast’
  • Check code for portability issues (Wrong functions, alignment, storage of numbers etc)
  • Ensure that coding standard from internals.texi is followed
  • Ensure that all syntax, API and behavior changes are compatible with previous MySQL production releases (when in doubt check with documentation)
  • There should be no compiler warnings
  • Ensure that new code and change set is properly commented

    Other things


  • If there is a better solution (for a future MySQL release), reviewer or implementor should write it into worklog
  • Require comments on all blocks of code, and changesets.
  • Look for regression.
  • Fix the root cause of the bug.
  • Look to see if there are no bugs in similar code (aka code which has similar architecture)

    First and Second Reviewer


  • Goals
  • First reviewers are for teaching
  • Second reviewer is ultimately responsible.
  • First reviewer needs to supply a list of all code that they were not able to verify for the second reviewer.
    • Subscribe

    • Post a new comment

      Error

      Comments allowed for friends only

      Anonymous comments are disabled in this journal

      switch
      default userpic

      Your reply will be screened

      Your IP address will be recorded 

      Help
    • 1 comment