Patchwork [v2,3/3] tpm_tis: fix format string specifier in tpm_tis_show_buffer()

login
register
mail settings
Submitter Liam Merwick
Date Feb. 11, 2019, 8:09 p.m.
Message ID <0b53df4a-7065-b3b7-c419-3f8e55cffdfd@oracle.com>
Download mbox | patch
Permalink /patch/723305/
State New
Headers show

Comments

Liam Merwick - Feb. 11, 2019, 8:09 p.m.
On 11/02/2019 19:56, Stefan Berger wrote:
> On 2/11/19 11:02 AM, Philippe Mathieu-Daudé wrote:
>> Hi Liam,
>>
>> On 2/11/19 4:03 PM, Liam Merwick wrote:
>>> cppcheck reports:
>>>
>>> [hw/tpm/tpm_tis.c:113]: (warning) %d in format string (no. 2) 
>>> requires 'int' but the argument type is 'unsigned int'
>>>
>>> Fix this by using %u instead of %d
> 
> 
> Liam, Neither gcc nor cppcheck 1.86 complains about this on my system. 
> What version of cppcheck are you using?

I was using 1.86 too but I had warnings enabled.

% cppcheck -j 8 --platform=unix64 --force --inline-suppr 
--enable=warning --error-exitcode=1 hw/tpm/tpm_tis.c
Checking hw/tpm/tpm_tis.c ...
[hw/tpm/tpm_tis.c:112]: (warning) %d in format string (no. 2) requires 
'int' but the argument type is 'unsigned int'.


> 
> 
>>>
>>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>>> ---
>>>   hw/tpm/tpm_tis.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>>> index 860c2ace7d99..395500e8a61d 100644
>>> --- a/hw/tpm/tpm_tis.c
>>> +++ b/hw/tpm/tpm_tis.c
>>> @@ -110,7 +110,7 @@ static void tpm_tis_show_buffer(const unsigned 
>>> char *buffer,
>>>       uint32_t len, i;
>> If you want to clean this, use a size_t here instead of u32.
>>
>>>       len = MIN(tpm_cmd_get_size(buffer), buffer_size);
>>> -    printf("tpm_tis: %s length = %d\n", string, len);
>>> +    printf("tpm_tis: %s length = %u\n", string, len);
>> So here the format is '%zu'.
>> However in code cleanup we try go get ride of printf() calls and replace
>> them with trace points.
> 
> 
> This code is only used for debugging if DEBUG_TIS has been #defined. No 
> need to add tracing here.

I'd come up the attached change (but that seems like overkill).

Regards,
Liam


> 
> 
>>
>>>       for (i = 0; i < len; i++) {
>>>           if (i && !(i % 16)) {
>>>               printf("\n");
>>>
>
Stefan Berger - Feb. 11, 2019, 9:13 p.m.
On 2/11/19 3:09 PM, Liam Merwick wrote:
>
>
> I'd come up the attached change (but that seems like overkill).


I don't think we need tracing for this.
Philippe Mathieu-Daudé - Feb. 12, 2019, 12:31 p.m.
On 2/11/19 10:13 PM, Stefan Berger wrote:
> On 2/11/19 3:09 PM, Liam Merwick wrote:
>> On 11/02/2019 19:56, Stefan Berger wrote:
>>> On 2/11/19 11:02 AM, Philippe Mathieu-Daudé wrote:
>>>> On 2/11/19 4:03 PM, Liam Merwick wrote:
[...]
>>>>> -    printf("tpm_tis: %s length = %d\n", string, len);
>>>>> +    printf("tpm_tis: %s length = %u\n", string, len);
>>>> So here the format is '%zu'.
>>>> However in code cleanup we try go get ride of printf() calls and
>>>> replace them with trace points.
>>>
>>>
>>> This code is only used for debugging if DEBUG_TIS has been #defined.
>>> No need to add tracing here.
>>
>> I'd come up the attached change (but that seems like overkill).
> 
> 
> I don't think we need tracing for this.

So if you think the code is mature enough, let's remove the DEBUG calls!

Else we prefer to convert DEBUG printf to trace events because (at least):
- no need to recompile to enable debugging
- when compiled with debugging, you don't mess with STDIO which can be
used as a chardev backend.

Thanks,

Phil.
Stefan Berger - Feb. 12, 2019, 1:27 p.m.
On 2/12/19 7:31 AM, Philippe Mathieu-Daudé wrote:
> On 2/11/19 10:13 PM, Stefan Berger wrote:
>> On 2/11/19 3:09 PM, Liam Merwick wrote:
>>> On 11/02/2019 19:56, Stefan Berger wrote:
>>>> On 2/11/19 11:02 AM, Philippe Mathieu-Daudé wrote:
>>>>> On 2/11/19 4:03 PM, Liam Merwick wrote:
> [...]
>>>>>> -    printf("tpm_tis: %s length = %d\n", string, len);
>>>>>> +    printf("tpm_tis: %s length = %u\n", string, len);
>>>>> So here the format is '%zu'.
>>>>> However in code cleanup we try go get ride of printf() calls and
>>>>> replace them with trace points.
>>>>
>>>> This code is only used for debugging if DEBUG_TIS has been #defined.
>>>> No need to add tracing here.
>>> I'd come up the attached change (but that seems like overkill).
>>
>> I don't think we need tracing for this.
> So if you think the code is mature enough, let's remove the DEBUG calls!
>
> Else we prefer to convert DEBUG printf to trace events because (at least):
> - no need to recompile to enable debugging
> - when compiled with debugging, you don't mess with STDIO which can be
> used as a chardev backend.
Fine. Then I withdraw my reviewed-by.
> Thanks,
>
> Phil.
>
Liam Merwick - Feb. 12, 2019, 1:43 p.m.
On 12/02/2019 13:27, Stefan Berger wrote:
> On 2/12/19 7:31 AM, Philippe Mathieu-Daudé wrote:
>> On 2/11/19 10:13 PM, Stefan Berger wrote:
>>> On 2/11/19 3:09 PM, Liam Merwick wrote:
>>>> On 11/02/2019 19:56, Stefan Berger wrote:
>>>>> On 2/11/19 11:02 AM, Philippe Mathieu-Daudé wrote:
>>>>>> On 2/11/19 4:03 PM, Liam Merwick wrote:
>> [...]
>>>>>>> -    printf("tpm_tis: %s length = %d\n", string, len);
>>>>>>> +    printf("tpm_tis: %s length = %u\n", string, len);
>>>>>> So here the format is '%zu'.
>>>>>> However in code cleanup we try go get ride of printf() calls and
>>>>>> replace them with trace points.
>>>>>
>>>>> This code is only used for debugging if DEBUG_TIS has been #defined.
>>>>> No need to add tracing here.
>>>> I'd come up the attached change (but that seems like overkill).
>>>
>>> I don't think we need tracing for this.
>> So if you think the code is mature enough, let's remove the DEBUG calls!
>>
>> Else we prefer to convert DEBUG printf to trace events because (at 
>> least):
>> - no need to recompile to enable debugging
>> - when compiled with debugging, you don't mess with STDIO which can be
>> used as a chardev backend.
> Fine. Then I withdraw my reviewed-by.


I don't see a way of removing the DEBUG calls without adding the 
overhead of a call to tpm_tis_show_buffer() each time even if tracing is 
not enabled (the 3 trace calls are interdependent and need a for loop).
Is there any example of a trace point that calls a function that then 
does non-trivial printing?

I could send a v3 with the patch I attached previously with the 3 printf 
calls changed to trace points but still wrapped in 'if (DEBUG_TIS)' and 
optimised out in non-debug.

Regards,
Liam
Philippe Mathieu-Daudé - Feb. 12, 2019, 2:32 p.m.
On 2/12/19 2:43 PM, Liam Merwick wrote:
> On 12/02/2019 13:27, Stefan Berger wrote:
>> On 2/12/19 7:31 AM, Philippe Mathieu-Daudé wrote:
>>> On 2/11/19 10:13 PM, Stefan Berger wrote:
>>>> On 2/11/19 3:09 PM, Liam Merwick wrote:
>>>>> On 11/02/2019 19:56, Stefan Berger wrote:
>>>>>> On 2/11/19 11:02 AM, Philippe Mathieu-Daudé wrote:
>>>>>>> On 2/11/19 4:03 PM, Liam Merwick wrote:
>>> [...]
>>>>>>>> -    printf("tpm_tis: %s length = %d\n", string, len);
>>>>>>>> +    printf("tpm_tis: %s length = %u\n", string, len);
>>>>>>> So here the format is '%zu'.
>>>>>>> However in code cleanup we try go get ride of printf() calls and
>>>>>>> replace them with trace points.
>>>>>>
>>>>>> This code is only used for debugging if DEBUG_TIS has been #defined.
>>>>>> No need to add tracing here.
>>>>> I'd come up the attached change (but that seems like overkill).
>>>>
>>>> I don't think we need tracing for this.
>>> So if you think the code is mature enough, let's remove the DEBUG calls!
>>>
>>> Else we prefer to convert DEBUG printf to trace events because (at
>>> least):
>>> - no need to recompile to enable debugging
>>> - when compiled with debugging, you don't mess with STDIO which can be
>>> used as a chardev backend.
>> Fine. Then I withdraw my reviewed-by.
> 
> 
> I don't see a way of removing the DEBUG calls without adding the
> overhead of a call to tpm_tis_show_buffer() each time even if tracing is
> not enabled (the 3 trace calls are interdependent and need a for loop).
> Is there any example of a trace point that calls a function that then
> does non-trivial printing?
> 
> I could send a v3 with the patch I attached previously with the 3 printf
> calls changed to trace points but still wrapped in 'if (DEBUG_TIS)' and
> optimised out in non-debug.

You can look at commit 56853498648.

Thanks!

Phil.

Patch

From 5dc2333c009583f5ce7338d5d67f857a4e8f4c74 Mon Sep 17 00:00:00 2001
From: Liam Merwick <liam.merwick@oracle.com>
Date: Mon, 11 Feb 2019 13:15:17 +0000
Subject: [PATCH v2 3/3] tpm_tis: fix format string specifier in
 tpm_tis_show_buffer()

cppcheck reports:

[hw/tpm/tpm_tis.c:113]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'

Fix this by converting it, and the other calls to printf in
tpm_tis_show_buffer(), to use trace points.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 hw/tpm/tpm_tis.c    | 11 ++++++-----
 hw/tpm/trace-events |  3 +++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 860c2ace7d99..5d85b1cf61b1 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -107,17 +107,18 @@  static uint8_t tpm_tis_locality_from_addr(hwaddr addr)
 static void tpm_tis_show_buffer(const unsigned char *buffer,
                                 size_t buffer_size, const char *string)
 {
-    uint32_t len, i;
+    size_t len;
+    uint32_t i;
 
     len = MIN(tpm_cmd_get_size(buffer), buffer_size);
-    printf("tpm_tis: %s length = %d\n", string, len);
+    trace_tpm_tis_show_buffer_hdr(string, len);
     for (i = 0; i < len; i++) {
         if (i && !(i % 16)) {
-            printf("\n");
+            trace_tpm_tis_show_buffer_newline();
         }
-        printf("%.2X ", buffer[i]);
+        trace_tpm_tis_show_buffer_entry(buffer[i]);
     }
-    printf("\n");
+    trace_tpm_tis_show_buffer_newline();
 }
 
 /*
diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
index 920d32ad5514..41d99d489097 100644
--- a/hw/tpm/trace-events
+++ b/hw/tpm/trace-events
@@ -36,6 +36,9 @@  tpm_emulator_pre_save(void) ""
 tpm_emulator_inst_init(void) ""
 
 # hw/tpm/tpm_tis.c
+tpm_tis_show_buffer_hdr(const char *string, size_t buffer_size) ": %s length = %zu"
+tpm_tis_show_buffer_entry(const unsigned char entry) "%.2X "
+tpm_tis_show_buffer_newline(void) "\n"
 tpm_tis_raise_irq(uint32_t irqmask) "Raising IRQ for flag 0x%08x"
 tpm_tis_new_active_locality(uint8_t locty) "Active locality is now %d"
 tpm_tis_abort(uint8_t locty) "New active locality is %d"
-- 
1.8.3.1