?

Log in

No account? Create an account

How To Review A Bug

« previous entry | next entry »
Feb. 12th, 2007 | 10:27 am

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.

    • Link | Leave a comment |

    Comments {1}

    (no subject)

    from: jamesd
    date: Feb. 12th, 2007 09:33 pm (UTC)
    Link

    If you are changing behavior, get the Support team to agree first. Customers will ensure that they are the ones after your head if you don't... and if you do have to break something else, they will have to explain why it was the least bad option. :)

    Reply | Thread