Coverity detected logically and structurally dead code

[kcgen]

Logically dead code:

Structurally dead code:

[gpakosz]

Hello @kcgen 👋

Thanks I'll have a look.

[gpakosz]

Can you please try the gh-30 branch?

[kcgen]

Thanks @gpakosz !
I added the update, but error'ing out during build due to error: unknown type name ‘bool’:

https://github.com/dosbox-staging/dosbox-staging/runs/3821433748?check_suite_focus=true

[gpakosz]

I pushed a fixup commit in the gh-30 branch.

To be honest, Coverity complaining about for (;;) is disappointing as it's a nice error handling pattern. It's obvious the loop will only run once, and I don't see the reason why it's reported as "structurally dead code" and why it matters 🤷‍♂️

[kcgen]

(sorry for the dupe; realize I replied in the code and not to the ticket!)

It took me a while to understand what value the loop was adding (given it will never loop twice), and I now see it's purpose is to help error detect on the realpath call.

I think your fix will pacify Coverity - but I agree it's combersome.

A couple other options - This first one replaces the for loop with a direct condition check on the error we're interested in:

  int length = -1;

  resolved = realpath(WAI_PROC_SELF_EXE, buffer);

  if (resolved) {
    length = (int)strlen(resolved);
    if (length <= capacity)
    {
      memcpy(out, resolved, length);
      if (dirname_length)
      {
        int i;
        for (i = length - 1; i >= 0; --i)
        {
          if (out[i] == '/')
          {
            *dirname_length = i;
            break;
          }
        }
      }
    }
  }
  return length;

Second approach:

• The consequence of the failed check (an early-exit with return -1) is made clear by moving the action to the check.
• The length becomes a single readonly assignment.
• The nesting levels drops from 5 to 4

  resolved = realpath(WAI_PROC_SELF_EXE, buffer);
  if (!resolved)
    return -1;

  const int length = (int)strlen(resolved);
  if (length <= capacity)
  {
    memcpy(out, resolved, length);
    if (dirname_length)
    {
      int i;
      for (i = length - 1; i >= 0; --i)
      {
        if (out[i] == '/')
        {
          *dirname_length = i;
          break;
        }
      }
    }
  }
  return length;

[gpakosz]

I don't feel like changing the code style just because 1 tool is a bit unhappy. Does the latest commit make peace with Coverity?

[kcgen]

Absolutely understood - (I now see the use of for/while is quite common in this regard! Sorry; I wasn't aware of this).

I merged the change, and am still waiting on the Coverity run. will report back when we make it through the queue!

[gpakosz]

I merged the change, and am still waiting on the Coverity run. will report back when we make it through the queue!

Thanks!

[kcgen]

Email just came in this afternoon:

Date: Thu, 07 Oct 2021 22:00:15 +0000 (UTC)
From: scan-admin@coverity.com
Subject: Coverity Scan: Analysis completed for dosbox-staging

    Your request for analysis of dosbox-staging has been completed successfully.
    The results are available at https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yrJxY-2BPN3T7aL3Ue7dBgTq0CGyYFVUgj2Utjo7YUzoQsw-3D-3DRv2m_NA4p-2F08z-2FXypRImXETzDvXCbePRCvFwSIPx-2BKsXhlkFPvYcFCF27K49Nw3ahSWBr4HEPyDhcoq0eqCYW8DMo69Ooq3zxBwHrUhDuHs-2FtdUhXVNs6HOMppLQKdhBk30dmBbkWfrd5VcmIYkBOhp4HSx0oz2g75TjzY2EfRTQonxPozOY9D3weOI4SIG9NDTGKg39bvew0WG9i2pwQzERSfA-3D-3D

    Build ID: 411159

    Analysis Summary:
       New defects found: 0
       Defects eliminated: 2

And two new issues are reported as fixed on the dashboard:

Thank you again @gpakosz !

[gpakosz]

Thanks for the test. I'll close this issue when I commit the changes