Review Request: 11862 cleanup smatch/cstyle/wscheck issues in usr/src/lib/sun_sas/

Review Request #2413 — Created Oct. 23, 2019 and submitted

rejohnst
illumos-gate
11862
general

This change cleans up all the smatch, cstyle and wscheck warnings in usr/src/lib/sun_sas/, which is a plugin for libSMHBAAPI. The change also enables smatch checking for this directory, going forward.

See testing notes in the ticket:

https://www.illumos.org/issues/11862

  • 0
  • 0
  • 14
  • 1
  • 15
Description From Last Updated
rejohnst
kkantor
  1. Looks good to me. I say 'ship it' if the comment about strlen/strnlen is irrelevant.

  2. I'm not sure if this is relevant, but should we use something like strnlen(hba_ptr->device_path, 1) == 1 to make this and other invocations in this file more safe?

    1. My preference would be to leave those as-is.

  3. Ah yes, ctysle! I remember thee.
  4. 
      
citrus
  1. 
      
  2. usr/src/lib/sun_sas/Makefile.com (Diff revision 2)
     
     

    Since you're doing all this, have you considered also removing these warning suppressions (and fixing the associated problems)?

  3. 
      
igork
  1. Ship It!
    1. agreed with others comments and cleanup of gcc warning will be good, but also agreed with current tested changes.

    2. Agreed - I have fixed the gcc warnings in the latest diff.

  2. 
      
rejohnst
andy_js
  1. 
      
  2. This is missing a void between the parentheses.

  3. Missing void here too.

  4. sizeof on a pointer doesn't look right.

  5. 
      
kkantor
  1. 
      
  2. 
      
rejohnst
rejohnst
andy_js
  1. 
      
  2. You only need to check the first byte:

    if (hba_ptr->device_path[0] != '\0')

  3. usr/src/lib/sun_sas/common/sun_sas.c (Diff revision 4)
     
     

    And here.

  4. 
      
rejohnst
citrus
  1. Ship It!
  2. 
      
andy_js
  1. 
      
  2. I think you need to put this check back in.

    1. This check was removed due to the following smatch error:

      smatch: ../common/Sun_sasGetAdapterName.c:54 Sun_sasGetAdapterName() warn: this array is probably non-NULL. 'hba_ptr->handle_name'

      When I looked at the code, the handle_name field is a statically allocated array - so I think the smatch error is correct.

      https://grok.elemental.org/source/xref/illumos-joyent/usr/src/lib/sun_sas/common/sun_sas.h?r=9e86db79#130

    2. The error is right, but it looks to me like it should be:

      if (hba_ptr->handle_name[0] == '\0') {
      hba_ptr = NULL;
      break;
      }

    3. Yeah, sorry - I should have looked more carefully. You're right. Fixed.

  3. This check needs to be inverted.

    1. Doh! Thanks - fixed.

  4. 
      
rejohnst
rejohnst
andy_js
  1. Ship It!
  2. 
      
igork
  1. Ship It!
  2. 
      
citrus
  1. Ship It!
  2. 
      
rejohnst
Review request changed

Status: Closed (submitted)

Loading...