| Message ID | 1549897385-10091-3-git-send-email-liam.merwick@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] tpm_tis: fix loop that cancels any seizure by a lower locality | expand |
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 > > 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. > for (i = 0; i < len; i++) { > if (i && !(i % 16)) { > printf("\n"); >
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? >> >> 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. > >> for (i = 0; i < len; i++) { >> if (i && !(i % 16)) { >> printf("\n"); >>
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"); >>> > 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"
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.
On 2/11/19 10:03 AM, 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 > > Signed-off-by: Liam Merwick <liam.merwick@oracle.com> Reviewed-by: Stefan Berger <stefanb@linux.ibm.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; > > 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); > for (i = 0; i < len; i++) { > if (i && !(i % 16)) { > printf("\n");
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.
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. >
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
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.
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; 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); for (i = 0; i < len; i++) { if (i && !(i % 16)) { printf("\n");
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 Signed-off-by: Liam Merwick <liam.merwick@oracle.com> --- hw/tpm/tpm_tis.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)