- Download Diff

Summary:

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

Review Request #2413 — Created Oct. 23, 2019 and submitted Nov. 11, 2019, 4:57 p.m.

Information
Owner:	rejohnst
Repository:	illumos-gate
Branch:
Bugs:	11862
Depends On:
Reviewers
Groups:	general
People:

Description

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.

Testing Done

See testing notes in the ticket:

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

Issues

0
0
14
1
All Issues: 15

Description	From	Last Updated
I'm not sure if this is relevant, but should we use something like strnlen(hba_ptr->device_path, 1) == 1 to make this ...	kkantor	Oct. 24, 2019, 8:54 p.m.
This is missing a void between the parentheses.	andy_js	Oct. 24, 2019, 8:54 p.m.
Missing void here too.	andy_js	Oct. 24, 2019, 8:54 p.m.
sizeof on a pointer doesn't look right.	andy_js	Oct. 24, 2019, 8:54 p.m.
Ditto.	andy_js	Oct. 24, 2019, 8:54 p.m.
You only need to check the first byte: if (hba_ptr->device_path[0] != '\0')	andy_js	Oct. 24, 2019, 9:45 p.m.
Ditto.	andy_js	Oct. 24, 2019, 9:45 p.m.
And again here.	andy_js	Oct. 24, 2019, 9:45 p.m.
Here too.	andy_js	Oct. 24, 2019, 9:45 p.m.
And here.	andy_js	Oct. 24, 2019, 9:45 p.m.
And here.	andy_js	Oct. 24, 2019, 9:45 p.m.
And here.	andy_js	Oct. 24, 2019, 9:45 p.m.
I think you need to put this check back in.	andy_js	Oct. 28, 2019, 6:06 p.m.
This check needs to be inverted.	andy_js	Oct. 28, 2019, 4:37 p.m.
Likewise.	andy_js	Oct. 28, 2019, 4:38 p.m.

Change Summary:

Missed a few wscheck errors

Diff:

Revision 2 (+230 -197)

Show changes

	usr/src/lib/sun_sas/Makefile.com
	usr/src/lib/sun_sas/common/Sun_sasFreeLibrary.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterName.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterPortAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetDiscoveredPortAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetNumberOfAdapters.c
	usr/src/lib/sun_sas/common/Sun_sasGetPhyStatistics.c
	10 more

Looks good to me. I say 'ship it' if the comment about strlen/strnlen is irrelevant.

usr/src/lib/sun_sas/common/Sun_sasGetAdapterPortAttributes.c (Diff revision 2)

Show all issues

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?

rejohnst Oct. 24, 2019, 5:59 p.m.

My preference would be to leave those as-is.

usr/src/lib/sun_sas/common/devtree_device_disco.c (Diff revision 2)
```
Ah yes, ctysle! I remember thee.
```

Ship it!

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)?

rejohnst Oct. 24, 2019, 5:58 p.m.
```
Yes - done :)
```

Ship it!

Ship It!

igork Oct. 23, 2019, 11:21 p.m.

agreed with others comments and cleanup of gcc warning will be good, but also agreed with current tested changes.

rejohnst Oct. 24, 2019, 5:58 p.m.

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

Change Summary:

Fixes gcc warnings and updates Makefile.com to not suppress gcc warnings

Diff:

Revision 3 (+233 -203)

Show changes

	usr/src/lib/sun_sas/Makefile.com
	usr/src/lib/sun_sas/common/Sun_sasFreeLibrary.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterName.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterPortAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetDiscoveredPortAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetNumberOfAdapters.c
	usr/src/lib/sun_sas/common/Sun_sasGetPhyStatistics.c
	11 more

usr/src/lib/sun_sas/common/Sun_sasFreeLibrary.c (Diff revision 3)
Show all issues
```
This is missing a void between the parentheses.
```
usr/src/lib/sun_sas/common/Sun_sasGetNumberOfAdapters.c (Diff revision 3)
Show all issues
```
Missing void here too.
```
usr/src/lib/sun_sas/common/devtree_device_disco.c (Diff revision 3)
Show all issues
```
sizeof on a pointer doesn't look right.
```
usr/src/lib/sun_sas/common/devtree_device_disco.c (Diff revision 3)
Show all issues
```
Ditto.
```

Ship it!

Diff:

Revision 4 (+236 -205)

Show changes

	usr/src/lib/sun_sas/Makefile.com
	usr/src/lib/sun_sas/common/Sun_sasFreeLibrary.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterName.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterPortAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetDiscoveredPortAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetNumberOfAdapters.c
	usr/src/lib/sun_sas/common/Sun_sasGetPhyStatistics.c
	11 more

Change Summary:

Added bug number "Information" section. Previous diff addressed the four comments from Andy S.

Bugs:

+	11862

usr/src/lib/sun_sas/common/Sun_sasGetAdapterPortAttributes.c (Diff revision 4)
Show all issues
```
You only need to check the first byte:
if (hba_ptr->device_path[0] != '\0')
```
usr/src/lib/sun_sas/common/Sun_sasGetDiscoveredPortAttributes.c (Diff revision 4)
Show all issues
```
Ditto.
```
usr/src/lib/sun_sas/common/Sun_sasGetPortAttributesByWWN.c (Diff revision 4)
Show all issues
```
And again here.
```
usr/src/lib/sun_sas/common/Sun_sasGetPortType.c (Diff revision 4)
Show all issues
```
Here too.
```
usr/src/lib/sun_sas/common/Sun_sasScsiInquiry.c (Diff revision 4)
Show all issues
```
And here.
```
usr/src/lib/sun_sas/common/Sun_sasSendSMPPassThru.c (Diff revision 4)
Show all issues
```
And here.
```
usr/src/lib/sun_sas/common/sun_sas.c (Diff revision 4)
Show all issues
```
And here.
```

Diff:

Revision 5 (+246 -215)

Show changes

	usr/src/lib/sun_sas/Makefile.com
	usr/src/lib/sun_sas/common/Sun_sasFreeLibrary.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterName.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterPortAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetDiscoveredPortAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetNumberOfAdapters.c
	usr/src/lib/sun_sas/common/Sun_sasGetPhyStatistics.c
	11 more

Ship it!

```
Ship It!
```

usr/src/lib/sun_sas/common/Sun_sasGetAdapterName.c (Diff revision 5)

Show all issues

I think you need to put this check back in.

rejohnst Oct. 28, 2019, 4:51 p.m.

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

andy_js Oct. 28, 2019, 4:54 p.m.

The error is right, but it looks to me like it should be:
if (hba_ptr->handle_name[0] == '\0') {

    hba_ptr = NULL;

    break;

}

rejohnst Oct. 28, 2019, 5:04 p.m.

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

usr/src/lib/sun_sas/common/Sun_sasScsiInquiry.c (Diff revision 5)
Show all issues
```
This check needs to be inverted.
```
1. rejohnst Oct. 28, 2019, 4:51 p.m.
  Doh! Thanks - fixed.
usr/src/lib/sun_sas/common/Sun_sasSendSMPPassThru.c (Diff revision 5)
Show all issues
```
Likewise.
```
1. rejohnst Oct. 28, 2019, 4:51 p.m.
  Thanks - fixed!

Change Summary:

Address Andrew's latest comments

Diff:

Revision 6 (+246 -215)

Show changes

	usr/src/lib/sun_sas/Makefile.com
	usr/src/lib/sun_sas/common/Sun_sasFreeLibrary.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterName.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterPortAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetDiscoveredPortAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetNumberOfAdapters.c
	usr/src/lib/sun_sas/common/Sun_sasGetPhyStatistics.c
	11 more

Change Summary:

One more change

Diff:

Revision 7 (+250 -215)

Show changes

	usr/src/lib/sun_sas/Makefile.com
	usr/src/lib/sun_sas/common/Sun_sasFreeLibrary.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterName.c
	usr/src/lib/sun_sas/common/Sun_sasGetAdapterPortAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetDiscoveredPortAttributes.c
	usr/src/lib/sun_sas/common/Sun_sasGetNumberOfAdapters.c
	usr/src/lib/sun_sas/common/Sun_sasGetPhyStatistics.c
	11 more

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

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

Change Summary:

Diff:

Change Summary:

Diff:

Diff:

Change Summary:

Bugs:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)