Hi,

On Sun, 2017-11-19 at 15:14 +0100, Peter Müller wrote:

Make syslogctrl.c use TCP as remote logging file if specified so. Thanks to Michael for going through the first version of this.

Patches 2 and 3 of this series are still valid.

Please just post them together next time so that we always have the full patchset to review.

There is still some issues here that I want to point out:

Signed-off-by: Peter Müller peter.mueller@link38.eu

src/misc-progs/syslogdctrl.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/src/misc-progs/syslogdctrl.c b/src/misc-progs/syslogdctrl.c index 52719023e..589b6d009 100644 --- a/src/misc-progs/syslogdctrl.c +++ b/src/misc-progs/syslogdctrl.c @@ -27,18 +27,19 @@ #define ERR_ANY 1 #define ERR_SETTINGS 2 /* error in settings file */ #define ERR_ETC 3 /* error with /etc permissions */ -#define ERR_CONFIG 4 /* error updated sshd_config */ +#define ERR_CONFIG 4 /* error updated rsyslogd config */ #define ERR_SYSLOG 5 /* error restarting syslogd */

Technically we are not using rsyslog. We use the standard syslog daemon.

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);

@@ -67,6 +68,12 @@ int main(void) exit(ERR_SETTINGS); }

• if (!findkey(kv, "REMOTELOG_PROTOCOL", protocol))
• {

•  /* fall back to UDP if no protocol was given */
  

•  protocol = "udp";
  

• }

This is better.

if (strspn(hostname, VALID_FQDN) != strlen(hostname))
{
   fprintf(stderr, "Bad REMOTELOG_ADDR: %s\n", hostname);

@@ -106,8 +113,19 @@ int main(void) }

if (!strcmp(buffer,"on"))

•  snprintf(buffer, STRING_SIZE - 1, "/bin/sed -e
  

's/^#\?\(\*\.\*[[:blank:]]\+@\).\+$/\1%s/' /etc/syslog.conf >&%d", hostname, config_fd );

•  /* check which transmission protocol was given */
  

•  if (strcmp(protocol, "udp"))
  

•  {
  

•     /* write line for UDP */
  

•     snprintf(buffer, STRING_SIZE - 1, "/bin/sed -e
  

's/^#\?\(\*\.\*[[:blank:]]\+@\).\+$/\1%s/' /etc/syslog.conf >&%d", hostname, config_fd );

•  }
  

You are checking for the wrong thing here. You check if protocol is equal to "udp" and strcmp will return 0 if that is the case.

That then leads to "if (0) { ... }" which is what you don't want. You want the opposite case. Therefore you will have to write "if (strcmp(protocol, "udp") == 0) { ... }".

•  if (strcmp(protocol, "tcp"))
  

•  {
  

•     /* write line for TCP */
  

•     snprintf(buffer, STRING_SIZE - 1, "/bin/sed -e
  

's/^#\?\(\*\.\*[[:blank:]]\+@@\).\+$/\1%s/' /etc/syslog.conf >&%d", hostname, config_fd );

•  }
  

  else

•  /* if remote syslog has been disabled */
   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 */

Can we not just test for TCP and then fall into the else case? We practically only have two blocks of code to execute.

-Michael