Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code, part 10

What's wrong with this code, part 10

  • Comments 64
Ok, time for another "what's wrong with this code".  This one's trivial from a code standpoint, but it's tricky...

// ----------------------------------------------------------------------
// Function:
// CThing1::OnSomethingHappening()
//
// Description:
// Called when something happens
//
// Return:
// S_OK if successful
//
// ----------------------------------------------------------------------
HRESULT CThing1::OnSomethingHappening()
{
    HRESULT hr;
        :

        :
    <Do Some Stuff>
        :
   
    // Perform some operation...
    hr = PerformAnOperation();
    if (FAILED(hr))
        hr = ERROR_NOT_SUPPORTED;
    IF_FAILED_JUMP(hr, Error);

Exit:
    return hr;

Error:
    goto Exit;
}

Not much code, no?  So what's wrong with it?

As usual, answers and kudos tomorrow.

  • Someone: oops :) You're right, it's a typo.
  • mirobin,
    A goto can be the last statement, that's just fine. And the compiler doesn't care about the dead code issue - it just silently optimizes it away (which is good).
  • Right before "Exit:" I would put "hr = S_OK;"
  • Greg,
    An interesting and really subtle point. It's not what's wrong, but a good point nontheless.
  • I got it!

    PerformAnOperation() could return S_FALSE to mean "Success". However, the way this function is crafted, the caller would assume that S_FALSE is a "Failure" code. In fact, S_FALSE would be treated the same as ERROR_INVALID_FUNCTION.

    What needs to happen is this:
    if (SUCCEEDED(hr)) hr = S_OK;
  • Someone: This is SOOO painful. You guys are SO close, but that's not it.

    S_FALSE is a great, subtle point (essentially it's the same point that Greg Wishart made just above).

    But it's not the problem with this code.

    Let's consider what I've said above:

    #1: This is a tricky problem (this is IMPORTANT).

    #2: hr = HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED) is NOT the error. That part of the code is correct.

    Ask yourselves "Under what circumstances would #2 be correct?".

    Here's another (subtle) hint: http://blogs.msdn.com/larryosterman/archive/2004/11/09/254561.aspx
  • I think its the following issues:

    1) The function calls PerformAnOperation and upon its failure blindly converts the failure into success by setting it to ERROR_NOT_SUPPORTED. (This doesn't indicate the E_NOTIMPL, but rather the operation is not allowed in the current state of the object/system/function whatever PerformAnOperation does.) ERROR_NOT_SUPPORTED is however a Win32 positive error code, which by OLE/COM conventions is a success code.

    2) OnSomethingHappening is documented as to return S_OK only, which is wrong: The implementation additionally returns ERROR_NOT_SUPPORTED and whatever success codes are returned from PerformAnOperation. (e.g. S_FALSE and others)

    3) In fact the documentation of at least one error state (ERROR_NOT_SUPPORTED) is missing. This must appear in the documentation block!

    4) As noted already the IF_FAILED_JUMP is unnecessary, but the compiler would remove it.

    Basically the main problem lies in the handling of the result of PerformAnOperation and the documentation of the OnSomethingHappening function itself. If a caller relies on the function specification (that it returns S_OK only), the caller could assume it will always succeeds and not do an error check at all - which could in the worst case destabilize a whole component/process.
    The missing documentation and wrong use of HRESULT values are key to this post - my thoughts.

  • Michael:

    #1 is not a problem, the code that converts the failure to ERROR_NOT_SUPPORTED is correct.

    #2: This is SOOO close. It's the closest anyone's been so far.

    #3: Absolutely.

    #4: Yup.

    I'm going to declare victory for Michael - he nailed what is the crux of the problem: The semantics of returning ERROR_NOT_SUPPORTED are not spelled out anywhere in the routine, even though the semantics are important.

    And the absence of that documentation caused a very real bug that took one of my peers the better part of a day to track down last week. More tomorrow.
  • Possibly a stupid question: is PerformAnOperation() definitely returning an HRESULT?
  • When PerformAnOpertion() actually fails and an ERROR_NOT_SUPPORTED is returned... if the caller does if(FAILED(OnSomethingHappened()) it will interpret failures as success.
  • I'm coming in way late here, sorry if I'm repeating anything, but the big problem is that if OnSomethingHappening() encounters an error along the way (for example PerformAnOperation() fails), then it returns a *successful* HRESULT.
    Since the caller (hopefully) tests the retval of OnSomethingHappening() with the SUCCEEDED/FAILED macros, the caller has no way to know that OnSomethingHappening() failed.
    If the caller is testing against S_OK explicitly, then this code is misusing HRESULTs.
  • I should expand upon that:

    HRESULT is just a typedef for LONG, so if PerformAnOperation returns a standard win32 error (or some arbitrary DWORD) then those macros could be confused.

    PerformAnOperation might even have a failure result that == S_OK!
  • This post reminds me of my software engineering class in college: Software development is not only coding. Which again reminds me to take a closer look at my code and its documentation :-)
  • It doesn't actually guarantee S_OK on success.

    And it gobbles up errors. Bad tester!
  • Just my opinion but I think the problem of treating ERROR_NOT_SUPPORTED as an HRESULT when it is not, is a much bigger deal than the documentation of the function.

    If the real problem were corrected, the "problem" you were looking for would not have been an issue.

    Just my two cents.
Page 3 of 5 (64 items) 12345