You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/07/01 14:25:28 UTC

[GitHub] [incubator-nuttx] Donny9 opened a new pull request #4032: syslog: fix crash when print localtime by syslog

Donny9 opened a new pull request #4032:
URL: https://github.com/apache/incubator-nuttx/pull/4032


   ## Summary
   syslog: fix crash when print localtime by syslog
   
   test need to enable config:
   CONFIG_LIBC_LOCATIME=y
   CONFIG_SYSLOG_TIMESTAMP_FORMATTED=y
   CONFIG_SYSLOG_TIMESTAMP_LOCALTIME=y
   CONFIG_SYSLOG_TIMESTAMP_REALTIME=y
   
   Change-Id: I322830e0818237a7eb65a158a6a0dea8f9da9b6c
   Signed-off-by: Jiuzhu Dong <do...@xiaomi.com>
   ## Impact
   output local time in syslog.
   ## Testing
   daily test.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4032: syslog: fix crash when print localtime by syslog

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4032:
URL: https://github.com/apache/incubator-nuttx/pull/4032#issuecomment-872962876


   > @Donny9 What was the problem with this? I tried it, but I didn't have any kind of "crash", it worked OK for me.
   > 
   > I am following the changes, but I don't understand the functional changes this PR brings. It seems to me only "styling" changes are made. Can you please elaborate on the issue and the solution?
   > 
   
   The key change is move localtime_r/gmtime_r into if (OSINIT_HW_READY()).
   
   > Also, to my understanding this PR introduces a bug.
   > If `OSINIT_HW_READY()` returns false, then the variable `tm` will be used uninitialized in line 132.
   > The same applies for variable `ts` in line 140.
   > 
   
   ts and tm is initialized to zero at line 74-76 and 79-81.
   
   > Also, why ingore the return code of the various calls in `clock_gettime()`, `clock_gettime()`, `clock_systime_timespec()`?
   > Isn't it better to check for the error, and provide sane values in `ts` and `tm`?
   
   The sane value(zero) set at the definition.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] fjpanag commented on pull request #4032: syslog: fix crash when print localtime by syslog

Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #4032:
URL: https://github.com/apache/incubator-nuttx/pull/4032#issuecomment-872993592


   @Donny9 I now understand the issue, thank you!  
   BTW, as I see, there is no `CONFIG_LIBC_LOCATIME` defined anywhere....
   
   @xiaoxiang781216 You are correct.  
   I followed the code with my debugger, and saw the variables containing garbage. But it was my mistake, they are in fact initialized before being used, as they should.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] fjpanag commented on pull request #4032: syslog: fix crash when print localtime by syslog

Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #4032:
URL: https://github.com/apache/incubator-nuttx/pull/4032#issuecomment-872912717


   @Donny9 What was the problem with this? I tried it, but I didn't have any kind of "crash", it worked OK for me.
   
   I am following the changes, but I don't understand the functional changes this PR brings. It seems to me only "styling" changes are made. Can you please elaborate on the issue and the solution?
   
   Also, to my understanding this PR introduces a bug.
   If `OSINIT_HW_READY()` returns false, then the variable `tm` will be used uninitialized in line 132.  
   The same applies for variable `ts` in line 140.
   
   Also, why ingore the return code of the various calls in `clock_gettime()`, `clock_gettime(), `clock_systime_timespec()`?  
   Isn't it better to check for the error, and provide sane values in `ts` and `tm`?
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] Donny9 commented on pull request #4032: syslog: fix crash when print localtime by syslog

Posted by GitBox <gi...@apache.org>.
Donny9 commented on pull request #4032:
URL: https://github.com/apache/incubator-nuttx/pull/4032#issuecomment-872959309


   > @Donny9 What was the problem with this? I tried it, but I didn't have any kind of "crash", it worked OK for me.
   > 
   > I am following the changes, but I don't understand the functional changes this PR brings. It seems to me only "styling" changes are made. Can you please elaborate on the issue and the solution?
   > 
   > Also, to my understanding this PR introduces a bug.
   > If `OSINIT_HW_READY()` returns false, then the variable `tm` will be used uninitialized in line 132.
   > The same applies for variable `ts` in line 140.
   > 
   > Also, why ingore the return code of the various calls in `clock_gettime()`, `clock_gettime()`, `clock_systime_timespec()`?
   > Isn't it better to check for the error, and provide sane values in `ts` and `tm`?
   
   Hello @fjpanag , you need to enable follow config, then use syslog to output log with localtime in origin code base.
   CONFIG_LIBC_LOCATIME=y
   CONFIG_SYSLOG_TIMESTAMP_FORMATTED=y
   CONFIG_SYSLOG_TIMESTAMP_LOCALTIME=y
   CONFIG_SYSLOG_TIMESTAMP_REALTIME=y
   
   The root cause of cash is that the localtime_r can't be called before OSINIT_HW_READY, because localtime_r will call sem_wait, but semaphore subsystem is uninitialized.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4032: syslog: fix crash when print localtime by syslog

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4032:
URL: https://github.com/apache/incubator-nuttx/pull/4032#issuecomment-873077047


   Yes, CONFIG_LIBC_LOCATIME is enabled in our product to convert UTC to local time correclty, which is seldom selected in the demo project. That's why the problem is found until now.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #4032: syslog: fix crash when print localtime by syslog

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4032:
URL: https://github.com/apache/incubator-nuttx/pull/4032


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] fjpanag edited a comment on pull request #4032: syslog: fix crash when print localtime by syslog

Posted by GitBox <gi...@apache.org>.
fjpanag edited a comment on pull request #4032:
URL: https://github.com/apache/incubator-nuttx/pull/4032#issuecomment-872912717


   @Donny9 What was the problem with this? I tried it, but I didn't have any kind of "crash", it worked OK for me.
   
   I am following the changes, but I don't understand the functional changes this PR brings. It seems to me only "styling" changes are made. Can you please elaborate on the issue and the solution?
   
   Also, to my understanding this PR introduces a bug.
   If `OSINIT_HW_READY()` returns false, then the variable `tm` will be used uninitialized in line 132.  
   The same applies for variable `ts` in line 140.
   
   Also, why ingore the return code of the various calls in `clock_gettime()`, `clock_gettime()`, `clock_systime_timespec()`?  
   Isn't it better to check for the error, and provide sane values in `ts` and `tm`?
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org