From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH 1/3 v2] allow remote syslog via TCP in syslogdctrl.c
Date: Sun, 19 Nov 2017 16:03:21 +0000
Message-ID: <1511107401.4838.524.camel@ipfire.org>
In-Reply-To: <20171119151454.26778013.peter.mueller@link38.eu>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============3866179551837346368=="
List-Id: <development.lists.ipfire.org>

--===============3866179551837346368==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Sun, 2017-11-19 at 15:14 +0100, Peter M=C3=BCller wrote:
> Make syslogctrl.c use TCP as remote logging file if specified so.
> Thanks to Michael for going through the first version of this.
>=20
> 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:

>=20
> Signed-off-by: Peter M=C3=BCller <peter.mueller(a)link38.eu>
> ---
>  src/misc-progs/syslogdctrl.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>=20
> 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.
=20
>  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 =3D 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);
> =20
> @@ -67,6 +68,12 @@ int main(void)
>        exit(ERR_SETTINGS);
>     }
> =20
> +   if (!findkey(kv, "REMOTELOG_PROTOCOL", protocol))
> +   {
> +      /* fall back to UDP if no protocol was given */
> +      protocol =3D "udp";
> +   }
> +

This is better.

>     if (strspn(hostname, VALID_FQDN) !=3D strlen(hostname))
>     {
>        fprintf(stderr, "Bad REMOTELOG_ADDR: %s\n", hostname);
> @@ -106,8 +113,19 @@ int main(void)
>     }
> =20
>     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") =
=3D=3D
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 );
> =20
>       /* 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

--===============3866179551837346368==
Content-Type: application/pgp-signature
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="signature.asc"
MIME-Version: 1.0

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUFCQ2dBZEZpRUU1L3JXNWwzR0dl
Mnlwa3R4Z0hudy8yK1FDUWNGQWxvUnEwa0FDZ2tRZ0hudy8yK1EKQ1FjMS9oQUFwVkdtdnVTUi9K
RGxBbkZLQ0YwMXF1OCtPdVkyWU9xZ2ZXVk9UOERHTkt2TCt6RTNtcXRQUTBUWQpTL3BnbjZ1dmh6
d2tLRHVSa2Z6SlViU2Y4RlZQMFpsMGF3R1NEQjU1TjBLSk94QjhyWkFnWTJselFBcW9kbHNnCmFa
Y2hhY0ZtM0VnR0FUYThOaXlhTGk4NlkvODNhb1ZNazJhSDdUSXROWHBJa2VlaitqS29SUUUzSzBu
YS9FNW4KczV4VGk3QnZiS0RvSkt0QWRYU09qcE9wdC84bHJvU25OQ2FXYWN3VWRFRHN3VDExZmV0
Nyt5V3IrVnh1enJkQQpJbGxjb2tQa1FxQ3ZRQ2ZmWER3RTZidlZEUVJacVRNWGZTUHN3Nko5bWVQ
cjVUQnZmVzV3NXk4VnZucHBITzFvClhjMCtHMlJGZHZNOFZwekgyamJONTdaNTdsQXRNUDJxcmp4
anJqdm5mdFd6KzFtVW4waStQMTRVRmhqQjBab2QKZC9FN2dXdnVPMFIvQnFlbG1tYkNnclFEbDdu
aENHSEtiTmdoNWdRaDNTd1dxY0ZuYktwak02anRpcWtOdFRZawo1cjJ3R1p0dXljeGJDQWtSUW04
UlZhYkNrRUE1Y1FyUXJEbUYrckk5WXprN3Z6elFoTWoyMUFpZ1VTdkdqWjc0CnFrdnNZYkovY0pV
Z09BVWVHVEdwTXJFaUswUktzSE1SLy9lSTdYNnNyZ0xZM0kxR2Z2VWcyUCtkTmVKbHhOb2MKUFll
U0RwOUE1TVM2NGkxSmVRMFFmUjR0cU1hVXFzZ0UvdUowYmZSSkZpTU03YW9tdVdvUkQ4SHk3azB4
eXlCRgp0M1hYeXRhNDY0OUJCWXRwWkdqc2dldE9rMFgyeUpRb3VuS1hrLy9uclBtRWJyWXVRNm89
Cj1ucE0wCi0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo=

--===============3866179551837346368==--