| Message ID | 20190819180125.17504-1-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Xen-devel] xen/console: Fix build when CONFIG_DEBUG_TRACE=y | expand |
On 19/08/2019 19:01, Julien Grall wrote: > Commit b5e6e1ee8da "xen/console: Don't treat NUL character as the end > of the buffer" extended sercon_puts to take the number of character > to print in argument. > > Sadly, a couple of couple of the callers in debugtrace_dump_worker() > were not converted. This result to a build failure when enabling > CONFIG_DEBUG_TRACE. > > Spotted by Travis using randconfig > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/drivers/char/console.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c > index 2c14c2ca73..924d4971ca 100644 > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -1185,11 +1185,12 @@ static void debugtrace_dump_worker(void) > > /* Print oldest portion of the ring. */ > ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); > - sercon_puts(&debugtrace_buf[debugtrace_prd]); > + sercon_puts(&debugtrace_buf[debugtrace_prd], > + strlen(&debugtrace_buf[debugtrace_prd])); Isn't this just debugtrace_bytes - debugtrace_prd - 1 ? ~Andrew > > /* Print youngest portion of the ring. */ > debugtrace_buf[debugtrace_prd] = '\0'; > - sercon_puts(&debugtrace_buf[0]); > + sercon_puts(&debugtrace_buf[0], debugtrace_prd); > > memset(debugtrace_buf, '\0', debugtrace_bytes); >
Hi Andrew, On 8/19/19 7:04 PM, Andrew Cooper wrote: > On 19/08/2019 19:01, Julien Grall wrote: >> Commit b5e6e1ee8da "xen/console: Don't treat NUL character as the end >> of the buffer" extended sercon_puts to take the number of character >> to print in argument. >> >> Sadly, a couple of couple of the callers in debugtrace_dump_worker() >> were not converted. This result to a build failure when enabling >> CONFIG_DEBUG_TRACE. >> >> Spotted by Travis using randconfig >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> --- >> xen/drivers/char/console.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c >> index 2c14c2ca73..924d4971ca 100644 >> --- a/xen/drivers/char/console.c >> +++ b/xen/drivers/char/console.c >> @@ -1185,11 +1185,12 @@ static void debugtrace_dump_worker(void) >> >> /* Print oldest portion of the ring. */ >> ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); >> - sercon_puts(&debugtrace_buf[debugtrace_prd]); >> + sercon_puts(&debugtrace_buf[debugtrace_prd], >> + strlen(&debugtrace_buf[debugtrace_prd])); > > Isn't this just debugtrace_bytes - debugtrace_prd - 1 ? I tried and it resulted to print a lot of @^ on the console. This is because the ring may not be full. So the portion between debugtrace_prd and debugtrace_bytes will be full of zero. Looking at the code again, I think this portion and either be full of zero character or full of non-zero character. In other word, a mix would not be possible. So how about: if ( debugtrace_buf[debugtrace_prd] != '\0' ) sercon_puts(&debugtrace_buf[debugtrace_prd], debugtrace_bytes - debugtrace_prd - 1); Cheers,
On 19/08/2019 19:37, Julien Grall wrote: > Hi Andrew, > > On 8/19/19 7:04 PM, Andrew Cooper wrote: >> On 19/08/2019 19:01, Julien Grall wrote: >>> Commit b5e6e1ee8da "xen/console: Don't treat NUL character as the end >>> of the buffer" extended sercon_puts to take the number of character >>> to print in argument. >>> >>> Sadly, a couple of couple of the callers in debugtrace_dump_worker() >>> were not converted. This result to a build failure when enabling >>> CONFIG_DEBUG_TRACE. >>> >>> Spotted by Travis using randconfig >>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>> --- >>> xen/drivers/char/console.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c >>> index 2c14c2ca73..924d4971ca 100644 >>> --- a/xen/drivers/char/console.c >>> +++ b/xen/drivers/char/console.c >>> @@ -1185,11 +1185,12 @@ static void debugtrace_dump_worker(void) >>> /* Print oldest portion of the ring. */ >>> ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); >>> - sercon_puts(&debugtrace_buf[debugtrace_prd]); >>> + sercon_puts(&debugtrace_buf[debugtrace_prd], >>> + strlen(&debugtrace_buf[debugtrace_prd])); >> >> Isn't this just debugtrace_bytes - debugtrace_prd - 1 ? > > I tried and it resulted to print a lot of @^ on the console. This is > because the ring may not be full. > > So the portion between debugtrace_prd and debugtrace_bytes will be > full of zero. > > Looking at the code again, I think this portion and either be full of > zero character or full of non-zero character. In other word, a mix > would not be possible. So how about: > > if ( debugtrace_buf[debugtrace_prd] != '\0' ) > sercon_puts(&debugtrace_buf[debugtrace_prd], > debugtrace_bytes - debugtrace_prd - 1); LGTM. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Hi, On 20/08/2019 14:12, Andrew Cooper wrote: > On 19/08/2019 19:37, Julien Grall wrote: >> Hi Andrew, >> >> On 8/19/19 7:04 PM, Andrew Cooper wrote: >>> On 19/08/2019 19:01, Julien Grall wrote: >>>> Commit b5e6e1ee8da "xen/console: Don't treat NUL character as the end >>>> of the buffer" extended sercon_puts to take the number of character >>>> to print in argument. >>>> >>>> Sadly, a couple of couple of the callers in debugtrace_dump_worker() >>>> were not converted. This result to a build failure when enabling >>>> CONFIG_DEBUG_TRACE. >>>> >>>> Spotted by Travis using randconfig >>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>>> --- >>>> xen/drivers/char/console.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c >>>> index 2c14c2ca73..924d4971ca 100644 >>>> --- a/xen/drivers/char/console.c >>>> +++ b/xen/drivers/char/console.c >>>> @@ -1185,11 +1185,12 @@ static void debugtrace_dump_worker(void) >>>> /* Print oldest portion of the ring. */ >>>> ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); >>>> - sercon_puts(&debugtrace_buf[debugtrace_prd]); >>>> + sercon_puts(&debugtrace_buf[debugtrace_prd], >>>> + strlen(&debugtrace_buf[debugtrace_prd])); >>> >>> Isn't this just debugtrace_bytes - debugtrace_prd - 1 ? >> >> I tried and it resulted to print a lot of @^ on the console. This is >> because the ring may not be full. >> >> So the portion between debugtrace_prd and debugtrace_bytes will be >> full of zero. >> >> Looking at the code again, I think this portion and either be full of >> zero character or full of non-zero character. In other word, a mix >> would not be possible. So how about: >> >> if ( debugtrace_buf[debugtrace_prd] != '\0' ) >> sercon_puts(&debugtrace_buf[debugtrace_prd], >> debugtrace_bytes - debugtrace_prd - 1); > > LGTM. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thank you! I will update the patch and commit it. Cheers,
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 2c14c2ca73..924d4971ca 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -1185,11 +1185,12 @@ static void debugtrace_dump_worker(void) /* Print oldest portion of the ring. */ ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); - sercon_puts(&debugtrace_buf[debugtrace_prd]); + sercon_puts(&debugtrace_buf[debugtrace_prd], + strlen(&debugtrace_buf[debugtrace_prd])); /* Print youngest portion of the ring. */ debugtrace_buf[debugtrace_prd] = '\0'; - sercon_puts(&debugtrace_buf[0]); + sercon_puts(&debugtrace_buf[0], debugtrace_prd); memset(debugtrace_buf, '\0', debugtrace_bytes);
Commit b5e6e1ee8da "xen/console: Don't treat NUL character as the end of the buffer" extended sercon_puts to take the number of character to print in argument. Sadly, a couple of couple of the callers in debugtrace_dump_worker() were not converted. This result to a build failure when enabling CONFIG_DEBUG_TRACE. Spotted by Travis using randconfig Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/drivers/char/console.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)