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/07 14:44:18 UTC
[GitHub] [incubator-nuttx] fjpanag opened a new pull request #3469: syslog: syslog_device ops are handled internally by the driver.
fjpanag opened a new pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469
## Summary
This commit mostly simplifies the usage of syslog_device.
`syslog_dev_initialize()` now provides a completely initialized and properly working channel, in contrast to the previous implementation were the `sc_ops` had to be set externally.
The addition of `syslog_dev_force()` reduces code duplication, as a default function is now provided for all channels to use.
## Impact
None.
## Testing
Builds and logs correctly.
File logger was mostly tested.
--
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 #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#discussion_r611024914
##########
File path: drivers/syslog/README.txt
##########
@@ -120,21 +120,31 @@ SYSLOG Channels
SYSLOG channel is represented by an interface defined in
include/nuttx/syslog/syslog.h:
- /* This structure provides the interface to a SYSLOG device */
+ /* SYSLOG I/O redirection methods */
- typedef CODE int (*syslog_putc_t)(int ch);
- typedef CODE int (*syslog_flush_t)(void);
+ typedef CODE ssize_t (*syslog_write_t)(FAR struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+ typedef CODE int (*syslog_putc_t)(FAR struct syslog_channel_s *channel,
+ int ch);
+ typedef CODE int (*syslog_flush_t)(FAR struct syslog_channel_s *channel);
+
+ /* SYSLOG device operations */
+
+ struct syslog_channel_ops_s
+ {
+ syslog_putc_t sc_putc; /* Normal buffered output */
Review comment:
But what would be the advantage of this?
My initial thought of removing `sc_put()` was under the condition that the logger will be able to deliver the whole log line to `sc_write()` so it can be written atomically. Calling `sc_write()` with a single character does not introduce any advantage.
--
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 #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#discussion_r609896737
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -751,3 +673,127 @@ int syslog_dev_flush(FAR struct syslog_channel_s *channel)
return OK;
}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: syslog_dev_initialize
+ *
+ * Description:
+ * Initialize to use the character device (or file) at
+ * CONFIG_SYSLOG_DEVPATH as the SYSLOG sink.
+ *
+ * One power up, the SYSLOG facility is non-existent or limited to very
+ * low-level output. This function may be called later in the
+ * initialization sequence after full driver support has been initialized.
+ * (via syslog_initialize()) It installs the configured SYSLOG drivers
+ * and enables full SYSLOGing capability.
+ *
+ * NOTE that this implementation excludes using a network connection as
+ * SYSLOG device. That would be a good extension.
+ *
+ * Input Parameters:
+ * devpath - The full path to the character device to be used.
+ * oflags - File open flags.
+ * mode - File open mode (only if oflags include O_CREAT).
+ *
+ * Returned Value:
+ * Returns a newly created SYSLOG channel, or NULL in case of any failure.
+ *
+ ****************************************************************************/
+
+FAR struct syslog_channel_s *syslog_dev_initialize(FAR const char *devpath,
+ int oflags, int mode)
+{
+ FAR struct syslog_dev_s * syslog_dev;
+
+ syslog_dev = kmm_zalloc(sizeof(struct syslog_dev_s));
+
+ if (syslog_dev == NULL)
+ {
+ return NULL;
+ }
+
+ syslog_dev_open(syslog_dev, devpath, oflags, mode);
+
+ syslog_dev->channel.sc_ops = &g_syslog_dev_ops;
+
+ return (FAR struct syslog_channel_s *)syslog_dev;
+}
+
+/****************************************************************************
+ * Name: syslog_dev_uninitialize
+ *
+ * Description:
+ * Called to disable the last device/file channel in preparation to use
+ * a different SYSLOG device. Currently only used for CONFIG_SYSLOG_FILE.
Review comment:
```suggestion
* Disable the last device/file channel in preparation to use a different
* SYSLOG device. Currently only used for CONFIG_SYSLOG_FILE.
```
nit: Simplify the phrase to use imperative mood ~~to be consistent with the rest of the unit~~. I've just noticed that there is no consistency at all. Well, at least that is a good practice hehe
--
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 commented on a change in pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#discussion_r610572325
##########
File path: drivers/syslog/README.txt
##########
@@ -120,21 +120,31 @@ SYSLOG Channels
SYSLOG channel is represented by an interface defined in
include/nuttx/syslog/syslog.h:
- /* This structure provides the interface to a SYSLOG device */
+ /* SYSLOG I/O redirection methods */
- typedef CODE int (*syslog_putc_t)(int ch);
- typedef CODE int (*syslog_flush_t)(void);
+ typedef CODE ssize_t (*syslog_write_t)(FAR struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+ typedef CODE int (*syslog_putc_t)(FAR struct syslog_channel_s *channel,
+ int ch);
+ typedef CODE int (*syslog_flush_t)(FAR struct syslog_channel_s *channel);
+
+ /* SYSLOG device operations */
+
+ struct syslog_channel_ops_s
+ {
+ syslog_putc_t sc_putc; /* Normal buffered output */
Review comment:
@fjpanag we can create a new patch to remove sc_putc if all driver implement sc_write.
--
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 pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#issuecomment-816645806
@xiaoxiang781216 I realized another issue with `syslog_device` driver.
In case of failure, the device is re-opened only during initialization. Not on subsequent failures.
I am now preparing the changes for this.
Do you want me to add these to this PR, or a new one should be created?
--
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 commented on a change in pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#discussion_r610590413
##########
File path: drivers/syslog/README.txt
##########
@@ -120,21 +120,31 @@ SYSLOG Channels
SYSLOG channel is represented by an interface defined in
include/nuttx/syslog/syslog.h:
- /* This structure provides the interface to a SYSLOG device */
+ /* SYSLOG I/O redirection methods */
- typedef CODE int (*syslog_putc_t)(int ch);
- typedef CODE int (*syslog_flush_t)(void);
+ typedef CODE ssize_t (*syslog_write_t)(FAR struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+ typedef CODE int (*syslog_putc_t)(FAR struct syslog_channel_s *channel,
+ int ch);
+ typedef CODE int (*syslog_flush_t)(FAR struct syslog_channel_s *channel);
+
+ /* SYSLOG device operations */
+
+ struct syslog_channel_ops_s
+ {
+ syslog_putc_t sc_putc; /* Normal buffered output */
Review comment:
We can call sc_write with one char: sc_putc(c) always equals sc_write(&c, 1).
--
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 #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469
--
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 pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#issuecomment-816663923
> If the problem is made by the current patch, let's fix it in this patch. If it's new problem, the new patch is better.
No, it is not caused by this. The issue is related to the existing behavior of the driver, that can be improved.
So please, merge this, and I will submit the new one today.
--
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 #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#discussion_r609896737
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -751,3 +673,127 @@ int syslog_dev_flush(FAR struct syslog_channel_s *channel)
return OK;
}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: syslog_dev_initialize
+ *
+ * Description:
+ * Initialize to use the character device (or file) at
+ * CONFIG_SYSLOG_DEVPATH as the SYSLOG sink.
+ *
+ * One power up, the SYSLOG facility is non-existent or limited to very
+ * low-level output. This function may be called later in the
+ * initialization sequence after full driver support has been initialized.
+ * (via syslog_initialize()) It installs the configured SYSLOG drivers
+ * and enables full SYSLOGing capability.
+ *
+ * NOTE that this implementation excludes using a network connection as
+ * SYSLOG device. That would be a good extension.
+ *
+ * Input Parameters:
+ * devpath - The full path to the character device to be used.
+ * oflags - File open flags.
+ * mode - File open mode (only if oflags include O_CREAT).
+ *
+ * Returned Value:
+ * Returns a newly created SYSLOG channel, or NULL in case of any failure.
+ *
+ ****************************************************************************/
+
+FAR struct syslog_channel_s *syslog_dev_initialize(FAR const char *devpath,
+ int oflags, int mode)
+{
+ FAR struct syslog_dev_s * syslog_dev;
+
+ syslog_dev = kmm_zalloc(sizeof(struct syslog_dev_s));
+
+ if (syslog_dev == NULL)
+ {
+ return NULL;
+ }
+
+ syslog_dev_open(syslog_dev, devpath, oflags, mode);
+
+ syslog_dev->channel.sc_ops = &g_syslog_dev_ops;
+
+ return (FAR struct syslog_channel_s *)syslog_dev;
+}
+
+/****************************************************************************
+ * Name: syslog_dev_uninitialize
+ *
+ * Description:
+ * Called to disable the last device/file channel in preparation to use
+ * a different SYSLOG device. Currently only used for CONFIG_SYSLOG_FILE.
Review comment:
```suggestion
* Disable the last device/file channel in preparation to use a different
* SYSLOG device. Currently only used for CONFIG_SYSLOG_FILE.
```
nit: Simplify the phrase to use imperative mood, to be consistent with the rest of the unit.
--
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 #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#discussion_r609893470
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -751,3 +673,127 @@ int syslog_dev_flush(FAR struct syslog_channel_s *channel)
return OK;
}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: syslog_dev_initialize
+ *
+ * Description:
+ * Initialize to use the character device (or file) at
+ * CONFIG_SYSLOG_DEVPATH as the SYSLOG sink.
+ *
+ * One power up, the SYSLOG facility is non-existent or limited to very
Review comment:
```suggestion
* On power up, the SYSLOG facility is non-existent or limited to very
```
--
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 #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#discussion_r609896737
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -751,3 +673,127 @@ int syslog_dev_flush(FAR struct syslog_channel_s *channel)
return OK;
}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: syslog_dev_initialize
+ *
+ * Description:
+ * Initialize to use the character device (or file) at
+ * CONFIG_SYSLOG_DEVPATH as the SYSLOG sink.
+ *
+ * One power up, the SYSLOG facility is non-existent or limited to very
+ * low-level output. This function may be called later in the
+ * initialization sequence after full driver support has been initialized.
+ * (via syslog_initialize()) It installs the configured SYSLOG drivers
+ * and enables full SYSLOGing capability.
+ *
+ * NOTE that this implementation excludes using a network connection as
+ * SYSLOG device. That would be a good extension.
+ *
+ * Input Parameters:
+ * devpath - The full path to the character device to be used.
+ * oflags - File open flags.
+ * mode - File open mode (only if oflags include O_CREAT).
+ *
+ * Returned Value:
+ * Returns a newly created SYSLOG channel, or NULL in case of any failure.
+ *
+ ****************************************************************************/
+
+FAR struct syslog_channel_s *syslog_dev_initialize(FAR const char *devpath,
+ int oflags, int mode)
+{
+ FAR struct syslog_dev_s * syslog_dev;
+
+ syslog_dev = kmm_zalloc(sizeof(struct syslog_dev_s));
+
+ if (syslog_dev == NULL)
+ {
+ return NULL;
+ }
+
+ syslog_dev_open(syslog_dev, devpath, oflags, mode);
+
+ syslog_dev->channel.sc_ops = &g_syslog_dev_ops;
+
+ return (FAR struct syslog_channel_s *)syslog_dev;
+}
+
+/****************************************************************************
+ * Name: syslog_dev_uninitialize
+ *
+ * Description:
+ * Called to disable the last device/file channel in preparation to use
+ * a different SYSLOG device. Currently only used for CONFIG_SYSLOG_FILE.
Review comment:
```suggestion
* Disable the last device/file channel in preparation to use a different
* SYSLOG device. Currently only used for CONFIG_SYSLOG_FILE.
```
nit: Simplify the phrase to use imperative mood~~, to be consistent with the rest of the unit~~. I've just noticed that there is no consistency at all. Well, at least that is a good practice hehe
--
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 pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#issuecomment-815005971
> > One question regarding this.
> > In file `nuttx/drivers/syslog/syslog_consolechannel.c` there is the function `syslog_console_force()`.
> > This function makes the assumption that `/dev/console/` is always the same device with `up_putc()`.
> > Is this a correct assumption, for all cases? Or maybe the default `syslog_dev_force()` shall also be used here?
>
> It's not always true.
So `syslog_console_force()` seems wrong. It may output on the wrong device.
I think I will remove it.
I didn't notice that `syslog_dev_putc()` handles newlines.
Indeed `syslog_devchan_putc()` can be removed.
Provided that both the above functions are removed, all `syslog_dev_xxx()` functions can be private, and I will do so.
--
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 pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#issuecomment-814976281
One question regarding this.
In file `nuttx/drivers/syslog/syslog_consolechannel.c` there is the function `syslog_console_force()`.
This function makes the assumption that `/dev/console/` is always the same device with `up_putc()`.
Is this a correct assumption, for all cases? Or maybe the default `syslog_dev_force()` shall also be used here?
--
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 commented on a change in pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#discussion_r610590413
##########
File path: drivers/syslog/README.txt
##########
@@ -120,21 +120,31 @@ SYSLOG Channels
SYSLOG channel is represented by an interface defined in
include/nuttx/syslog/syslog.h:
- /* This structure provides the interface to a SYSLOG device */
+ /* SYSLOG I/O redirection methods */
- typedef CODE int (*syslog_putc_t)(int ch);
- typedef CODE int (*syslog_flush_t)(void);
+ typedef CODE ssize_t (*syslog_write_t)(FAR struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+ typedef CODE int (*syslog_putc_t)(FAR struct syslog_channel_s *channel,
+ int ch);
+ typedef CODE int (*syslog_flush_t)(FAR struct syslog_channel_s *channel);
+
+ /* SYSLOG device operations */
+
+ struct syslog_channel_ops_s
+ {
+ syslog_putc_t sc_putc; /* Normal buffered output */
Review comment:
We can call sc_write with one char(sc_putc(c) equals sc_write(&c, 1).
--
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 commented on pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#issuecomment-816650406
If the problem is made by the current patch, let's fix it in this patch. If it's new problem, the new patch is better.
--
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 commented on a change in pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#discussion_r611034407
##########
File path: drivers/syslog/README.txt
##########
@@ -120,21 +120,31 @@ SYSLOG Channels
SYSLOG channel is represented by an interface defined in
include/nuttx/syslog/syslog.h:
- /* This structure provides the interface to a SYSLOG device */
+ /* SYSLOG I/O redirection methods */
- typedef CODE int (*syslog_putc_t)(int ch);
- typedef CODE int (*syslog_flush_t)(void);
+ typedef CODE ssize_t (*syslog_write_t)(FAR struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+ typedef CODE int (*syslog_putc_t)(FAR struct syslog_channel_s *channel,
+ int ch);
+ typedef CODE int (*syslog_flush_t)(FAR struct syslog_channel_s *channel);
+
+ /* SYSLOG device operations */
+
+ struct syslog_channel_ops_s
+ {
+ syslog_putc_t sc_putc; /* Normal buffered output */
Review comment:
Yes, it's better to remove sc_put and ensure we call sc_write with the whole line.
--
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 #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#discussion_r610576411
##########
File path: drivers/syslog/README.txt
##########
@@ -120,21 +120,31 @@ SYSLOG Channels
SYSLOG channel is represented by an interface defined in
include/nuttx/syslog/syslog.h:
- /* This structure provides the interface to a SYSLOG device */
+ /* SYSLOG I/O redirection methods */
- typedef CODE int (*syslog_putc_t)(int ch);
- typedef CODE int (*syslog_flush_t)(void);
+ typedef CODE ssize_t (*syslog_write_t)(FAR struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+ typedef CODE int (*syslog_putc_t)(FAR struct syslog_channel_s *channel,
+ int ch);
+ typedef CODE int (*syslog_flush_t)(FAR struct syslog_channel_s *channel);
+
+ /* SYSLOG device operations */
+
+ struct syslog_channel_ops_s
+ {
+ syslog_putc_t sc_putc; /* Normal buffered output */
Review comment:
@xiaoxiang781216 I had a look at this, but it is not as simple as it seems...
`nx_vsyslog()` uses `lib_vsprintf()` which only uses a putc function. And thus this is the only function used, apart from some special cases (like flushing the buffer), that use `sc_write()`.
Unless you have a good idea on how this can be done, I would prefer to leave it as is.
And possibly work on this, when whole issue of syslog thread-safety is resolved, some time in the near future.
--
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 commented on pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#issuecomment-815894957
I think so, it's better to enforce all driver support sc_write method and drop sc_putc.
--
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 commented on a change in pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#discussion_r610590413
##########
File path: drivers/syslog/README.txt
##########
@@ -120,21 +120,31 @@ SYSLOG Channels
SYSLOG channel is represented by an interface defined in
include/nuttx/syslog/syslog.h:
- /* This structure provides the interface to a SYSLOG device */
+ /* SYSLOG I/O redirection methods */
- typedef CODE int (*syslog_putc_t)(int ch);
- typedef CODE int (*syslog_flush_t)(void);
+ typedef CODE ssize_t (*syslog_write_t)(FAR struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+ typedef CODE int (*syslog_putc_t)(FAR struct syslog_channel_s *channel,
+ int ch);
+ typedef CODE int (*syslog_flush_t)(FAR struct syslog_channel_s *channel);
+
+ /* SYSLOG device operations */
+
+ struct syslog_channel_ops_s
+ {
+ syslog_putc_t sc_putc; /* Normal buffered output */
Review comment:
We can call sc_write with one char.
--
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] v01d commented on a change in pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#discussion_r611045235
##########
File path: drivers/syslog/README.txt
##########
@@ -120,21 +120,31 @@ SYSLOG Channels
SYSLOG channel is represented by an interface defined in
include/nuttx/syslog/syslog.h:
- /* This structure provides the interface to a SYSLOG device */
+ /* SYSLOG I/O redirection methods */
- typedef CODE int (*syslog_putc_t)(int ch);
- typedef CODE int (*syslog_flush_t)(void);
+ typedef CODE ssize_t (*syslog_write_t)(FAR struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+ typedef CODE int (*syslog_putc_t)(FAR struct syslog_channel_s *channel,
+ int ch);
+ typedef CODE int (*syslog_flush_t)(FAR struct syslog_channel_s *channel);
+
+ /* SYSLOG device operations */
+
+ struct syslog_channel_ops_s
+ {
+ syslog_putc_t sc_putc; /* Normal buffered output */
Review comment:
I don't fully understand the purpose of `put` either, but my guess is that is done this way so that it is possible to print the message from interrupt handlers without having to hold any lock (there is some comment about this in syslog implementation). When I looked at how to make ramlog implement write() I got stuck with this.
--
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 pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#issuecomment-815719911
Hmm, is `CONFIG_SYSLOG_WRITE` actually useful anymore?
Now that there can be multiple channels, the user may easily want to create a new custom channel, that may use `sc_write`.
I think this is to be removed too, as it forces you to enable a possibly unrelated channel (e.g. file logger), to gain access to the write method, and the syslog buffering.
--
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 commented on a change in pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#discussion_r608735407
##########
File path: drivers/syslog/syslog.h
##########
@@ -324,6 +324,23 @@ ssize_t syslog_dev_write(FAR struct syslog_channel_s *channel,
int syslog_dev_putc(FAR struct syslog_channel_s *channel, int ch);
+/****************************************************************************
+ * Name: syslog_dev_force
+ *
+ * Description:
+ * Dummy, do nothing force write operation.
+ *
+ * Input Parameters:
+ * channel - Handle to syslog channel to be used.
+ *
+ * Returned Value:
+ * On success, the character is echoed back to the caller. A negated
+ * errno value is returned on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_dev_force(FAR struct syslog_channel_s *channel, int ch);
Review comment:
let's change syslog_dev_xxx to static function too
##########
File path: drivers/syslog/syslog_devchannel.c
##########
@@ -169,9 +135,11 @@ int syslog_dev_channel(void)
return -ENOMEM;
}
- /* Register the channel operations */
+ /* Register any custom channel operations */
- g_syslog_dev_channel->sc_ops = &g_syslog_ops;
+#ifdef CONFIG_SYSLOG_CHAR_CRLF
+ g_syslog_dev_channel->sc_ops->sc_putc = syslog_devchan_putc;
Review comment:
could remove syslog_devchan_putc to since syslogstream_putc already append '\r' before '\n'
--
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 commented on pull request #3469: syslog: syslog_device ops are handled internally by the driver.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3469:
URL: https://github.com/apache/incubator-nuttx/pull/3469#issuecomment-814988628
> One question regarding this.
>
> In file `nuttx/drivers/syslog/syslog_consolechannel.c` there is the function `syslog_console_force()`.
> This function makes the assumption that `/dev/console/` is always the same device with `up_putc()`.
> Is this a correct assumption, for all cases? Or maybe the default `syslog_dev_force()` shall also be used here?
It's not always true.
--
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