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/04/06 13:38:37 UTC
[GitHub] [incubator-nuttx] fjpanag opened a new pull request #3454: syslog: Proper uninit of syslog_device.
fjpanag opened a new pull request #3454:
URL: https://github.com/apache/incubator-nuttx/pull/3454
## Summary
Previously `syslog_dev_uninitialize()` was using `syslog_dev_outputready()` to check whether the device is in proper state for uninitialization.
This is wrong for a couple of reasons:
* `syslog_dev_outputready()` may try to open the device again if it is in `SYSLOG_REOPEN` state, while what we want is to just close it.
* If `syslog_dev_outputready()` returned false, the uninitialization was aborted. This was unconditional and although the device is in a state that it cannot perform any writes, it may be perfectly able to be closed.
* To make matters worse `syslog_dev_uninitialize()` does not return anything, so the caller is not able to tell whether the device is actually uninitialized and whether the resources were actually freed. The only think left is to assume that it succeeded which may lead to memory leaks.
This new implementation actually checks the device state, and performs uninitialization accordingly.
It ensures that the resources will be freed, or hits assertions in case the syslog API is used incorrectly.
This fix is especially important now that dynamic add/removal of syslog channels is planned.
## Impact
A syslog device will now be uninitialized correctly, with no leaks.
## Testing
I did:
```c
while(1)
{
syslog_file_channel("/dev/console");
syslog(LOG_INFO, "A test message.\n");
}
```
I followed the code, and the uninitialization seems to be executed properly.
No memory leaks were observed.
I simulated the device to be in `SYSLOG_REOPEN` state, and still no problems observed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3454: syslog: Proper uninit of syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3454:
URL: https://github.com/apache/incubator-nuttx/pull/3454#discussion_r608453024
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -435,22 +435,44 @@ void syslog_dev_uninitialize(FAR struct syslog_channel_s *channel)
{
FAR struct syslog_dev_s *syslog_dev = (FAR struct syslog_dev_s *)channel;
- /* Check if the system is ready */
+ /* Uninitializing a SYSLOG device should not take place within
+ * interrupt context.
+ */
- if (syslog_dev_outputready(syslog_dev) < 0)
+ if (up_interrupt_context() || getpid() == 0)
{
+ DEBUGASSERT(!up_interrupt_context() && getpid() != 0);
return;
}
- /* Attempt to flush any buffered data */
+ /* The device cannot be unititialized while it is being
+ * initialized simultaneously.
+ */
+
+ DEBUGASSERT(syslog_dev->sl_state != SYSLOG_UNINITIALIZED &&
+ syslog_dev->sl_state != SYSLOG_INITIALIZING);
+
+ /* Attempt to flush any buffered data. */
sched_lock();
syslog_dev_flush(channel);
- /* Close the detached file instance */
+ /* Close the detached file instance, and destroy the
+ * semaphore. These are both only created when the
+ * device is in SYSLOG_OPENED or SYSLOG_FAILURE
+ * state.
+ */
Review comment:
Done.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3454: syslog: Proper uninit of syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3454:
URL: https://github.com/apache/incubator-nuttx/pull/3454#discussion_r608453718
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -435,22 +435,44 @@ void syslog_dev_uninitialize(FAR struct syslog_channel_s *channel)
{
FAR struct syslog_dev_s *syslog_dev = (FAR struct syslog_dev_s *)channel;
- /* Check if the system is ready */
+ /* Uninitializing a SYSLOG device should not take place within
+ * interrupt context.
+ */
- if (syslog_dev_outputready(syslog_dev) < 0)
+ if (up_interrupt_context() || getpid() == 0)
{
+ DEBUGASSERT(!up_interrupt_context() && getpid() != 0);
return;
}
- /* Attempt to flush any buffered data */
+ /* The device cannot be unititialized while it is being
Review comment:
Good catch, I was staring at this a couple of minutes, trying to understand the difference...
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #3454: syslog: Proper uninit of syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3454:
URL: https://github.com/apache/incubator-nuttx/pull/3454
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3454: syslog: Proper uninit of syslog_device.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3454:
URL: https://github.com/apache/incubator-nuttx/pull/3454#discussion_r608010726
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -435,22 +435,44 @@ void syslog_dev_uninitialize(FAR struct syslog_channel_s *channel)
{
FAR struct syslog_dev_s *syslog_dev = (FAR struct syslog_dev_s *)channel;
- /* Check if the system is ready */
+ /* Uninitializing a SYSLOG device should not take place within
+ * interrupt context.
+ */
- if (syslog_dev_outputready(syslog_dev) < 0)
+ if (up_interrupt_context() || getpid() == 0)
{
+ DEBUGASSERT(!up_interrupt_context() && getpid() != 0);
return;
}
- /* Attempt to flush any buffered data */
+ /* The device cannot be unititialized while it is being
Review comment:
```suggestion
/* The device cannot be uninitialized while it is being
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3454: syslog: Proper uninit of syslog_device.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3454:
URL: https://github.com/apache/incubator-nuttx/pull/3454#discussion_r608012774
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -435,22 +435,44 @@ void syslog_dev_uninitialize(FAR struct syslog_channel_s *channel)
{
FAR struct syslog_dev_s *syslog_dev = (FAR struct syslog_dev_s *)channel;
- /* Check if the system is ready */
+ /* Uninitializing a SYSLOG device should not take place within
+ * interrupt context.
+ */
- if (syslog_dev_outputready(syslog_dev) < 0)
+ if (up_interrupt_context() || getpid() == 0)
{
+ DEBUGASSERT(!up_interrupt_context() && getpid() != 0);
return;
}
- /* Attempt to flush any buffered data */
+ /* The device cannot be unititialized while it is being
+ * initialized simultaneously.
+ */
+
+ DEBUGASSERT(syslog_dev->sl_state != SYSLOG_UNINITIALIZED &&
+ syslog_dev->sl_state != SYSLOG_INITIALIZING);
+
+ /* Attempt to flush any buffered data. */
sched_lock();
syslog_dev_flush(channel);
- /* Close the detached file instance */
+ /* Close the detached file instance, and destroy the
+ * semaphore. These are both only created when the
+ * device is in SYSLOG_OPENED or SYSLOG_FAILURE
+ * state.
+ */
Review comment:
```suggestion
/* Close the detached file instance, and destroy the semaphore. These are
* both only created when the device is in SYSLOG_OPENED or SYSLOG_FAILURE
* state.
*/
```
nit: Improving the line occupancy.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org