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

["Peter Müller" <peter.mueller@link38.eu>]

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

Hello Michael,

thanks for the reply. I'll have a look at this at the weekend...

Best regards,
Peter Müller

> 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