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