diff mbox series

ptp: ptp_clockmatrix: initialize variables

Message ID 20201011200955.29992-1-trix@redhat.com
State New
Headers show
Series ptp: ptp_clockmatrix: initialize variables | expand

Commit Message

Tom Rix Oct. 11, 2020, 8:09 p.m. UTC 
From: Tom Rix <trix@redhat.com>

Clang static analysis reports this representative problem

ptp_clockmatrix.c:1852:2: warning: 5th function call argument
  is an uninitialized value
        snprintf(idtcm->version, sizeof(idtcm->version), "%u.%u.%u",
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

idtcm_display_version_info() calls several idtcm_read_*'s without
checking a return status.  Initialize the read variables so if a
read fails, a garbage value is not displayed.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/ptp/ptp_clockmatrix.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Richard Cochran Oct. 12, 2020, 10:01 p.m. UTC | #1 
On Sun, Oct 11, 2020 at 01:09:55PM -0700, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>

> 

> Clang static analysis reports this representative problem

> 

> ptp_clockmatrix.c:1852:2: warning: 5th function call argument

>   is an uninitialized value

>         snprintf(idtcm->version, sizeof(idtcm->version), "%u.%u.%u",

>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> 

> idtcm_display_version_info() calls several idtcm_read_*'s without

> checking a return status.


So why not check the return status?

Your patch papers over the issue.

Thanks,
Richard
Tom Rix Oct. 13, 2020, 4:07 a.m. UTC | #2 
On 10/12/20 3:01 PM, Richard Cochran wrote:
> On Sun, Oct 11, 2020 at 01:09:55PM -0700, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> Clang static analysis reports this representative problem
>>
>> ptp_clockmatrix.c:1852:2: warning: 5th function call argument
>>   is an uninitialized value
>>         snprintf(idtcm->version, sizeof(idtcm->version), "%u.%u.%u",
>>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> idtcm_display_version_info() calls several idtcm_read_*'s without
>> checking a return status.
> So why not check the return status?

calling function is a print information only function that returns void.

I do think not adding error handling is worth it.

I could change the return and then the calls if if you like.

Tom

>
> Your patch papers over the issue.
>
> Thanks,
> Richard
>
Richard Cochran Oct. 13, 2020, 12:40 p.m. UTC | #3 
On Mon, Oct 12, 2020 at 09:07:30PM -0700, Tom Rix wrote:
> calling function is a print information only function that returns void.


That is fine.

> I do think not adding error handling is worth it.

> 

> I could change the return and then the calls if if you like.


How about printing the version string when readable, and otherwise
print an appropriate informative error message?

Thanks,
Richard
diff mbox series

Patch

diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
index e020faff7da5..47e5e62da5d2 100644
--- a/drivers/ptp/ptp_clockmatrix.c
+++ b/drivers/ptp/ptp_clockmatrix.c
@@ -1832,12 +1832,12 @@  static int idtcm_enable_tod(struct idtcm_channel *channel)
 
 static void idtcm_display_version_info(struct idtcm *idtcm)
 {
-	u8 major;
-	u8 minor;
-	u8 hotfix;
-	u16 product_id;
-	u8 hw_rev_id;
-	u8 config_select;
+	u8 major = 0;
+	u8 minor = 0;
+	u8 hotfix = 0;
+	u16 product_id = 0;
+	u8 hw_rev_id = 0;
+	u8 config_select = 0;
 	char *fmt = "%d.%d.%d, Id: 0x%04x  HW Rev: %d  OTP Config Select: %d\n";
 
 	idtcm_read_major_release(idtcm, &major);