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

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@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:]]\+@.\+\)$/#\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