Re: [PATCH 1/3] allow remote syslog via TCP in syslogdctrl.c

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 3545 bytes --]

Hello,

On Sun, 2017-11-12 at 08:13 +0100, Peter Müller wrote:
> Make syslogctrl.c use TCP as remote logging file if specified so.
> 
> NOTE: This patch likely contains errors, since I do not know anything
> about C at all. Please have a close look at it. Sorry.

Yes it has some errors and won't work. Not sure how you tested this
successfully.

> Signed-off-by: Peter Müller <peter.mueller(a)link38.eu>
> ---
>  src/misc-progs/syslogdctrl.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/src/misc-progs/syslogdctrl.c b/src/misc-progs/syslogdctrl.c
> index 52719023e..b356ebe49 100644
> --- a/src/misc-progs/syslogdctrl.c
> +++ b/src/misc-progs/syslogdctrl.c
> @@ -32,13 +32,14 @@
>  
>  int main(void)
>  {
> -   char buffer[STRING_SIZE], command[STRING_SIZE], hostname[STRING_SIZE];
> +   char buffer[STRING_SIZE], command[STRING_SIZE], hostname[STRING_SIZE],
> protocol[STRING_SIZE];
>     char varmessages[STRING_SIZE], asynclog[STRING_SIZE];
>     int config_fd,rc,fd,pid;
>     struct stat st;
>     struct keyvalue *kv = NULL;
>     memset(buffer, 0, STRING_SIZE);
>     memset(hostname, 0, STRING_SIZE);
> +   memset(protocol, 0, STRING_SIZE);
>     memset(varmessages, 0, STRING_SIZE);
>     memset(asynclog, 0, STRING_SIZE);

There is no need to call memset here, but since it is for everything else it
probably doesn't hurt.

Better would be to initialise the string with "" right when it is defined. Would
spare the memset at runtime.

Or to change findkey() to set the string to "" when the key wasn't found.

> @@ -73,6 +74,12 @@ int main(void)
>        exit(ERR_SETTINGS);
>     }
>  
> +   if (!findkey(kv, "REMOTELOG_PROTOCOL", protocol))
> +   {
> +      fprintf(stderr, "Cannot read REMOTELOG_PROTOCOL\n");
> +      exit(ERR_SETTINGS);
> +   }
> +
>     freekeyvalues(kv);

Should we not default to UDP here in case the value is not being set?

I think exiting is a bit much.
  
> @@ -105,9 +112,22 @@ int main(void)
>        exit(ERR_CONFIG);
>     }
>  
> +   /* differ between UDP and TCP as rsyslog protocol */
>     if (!strcmp(buffer,"on"))
> -      snprintf(buffer, STRING_SIZE - 1, "/bin/sed -e
> 's/^#\\?\\(\\*\\.\\*[[:blank:]]\\+@\\).\\+$/\\1%s/' /etc/syslog.conf >&%d",
> hostname, config_fd );
> +      if ( protocol == "udp" )
> +      {
> +         snprintf(buffer, STRING_SIZE - 1, "/bin/sed -e
> 's/^#\\?\\(\\*\\.\\*[[:blank:]]\\+@\\).\\+$/\\1%s/' /etc/syslog.conf >&%d",
> hostname, config_fd );
> +      }
> +      elif ( protocol == "tcp" )
> +      {
> +         snprintf(buffer, STRING_SIZE - 1, "/bin/sed -e
> 's/^#\\?\\(\\*\\.\\*[[:blank:]]\\+@@\\).\\+$/\\1%s/' /etc/syslog.conf >&%d",
> hostname, config_fd );
> +      }
> +      else
> +      {
> +         fprintf(stderr, "Received invalid protocol for remote log\n");
> +      }
>     else
> +      /* turn off remote syslog if specified */
>        snprintf(buffer, STRING_SIZE - 1, "/bin/sed -e
> 's/^#\\?\\(\\*\\.\\*[[:blank:]]\\+(a).\\+\\)$/#\\1/' /etc/syslog.conf >&%d",
> config_fd );
>  
>       /* if the return code isn't 0 failsafe */

This block is slightly messy with calling all the seds. But I it has been like
this before, performance is not important here, etc.

But you can't compare strings in C like this. You will compare the pointer
values here which will never match.

The correct check would be: strcmp(protocol, "udp") == 0

And likewise for TCP.

Best,
-Michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]