[PATCH v2 6/6] dhcpcd: Add hooks

[Chris Johns]

On 07/05/2018 17:04, Sebastian Huber wrote:
> On 04/05/18 02:42, Chris Johns wrote:
>> On 03/05/2018 16:49, Sebastian Huber wrote:
>>
>>> +#endif /* __rtems__ */
>>> diff --git a/dhcpcd/namespace.h b/dhcpcd/namespace.h
>>> index efff909ad..c9f5fb1fa 100644
>>> --- a/dhcpcd/namespace.h
>>> +++ b/dhcpcd/namespace.h
>>> @@ -156,6 +156,7 @@ extern rtems_recursive_mutex dhcpcd_mutex;
>>>   #define print_string dhcpcd_print_string
>>>   #define read_config dhcpcd_read_config
>>>   #define read_lease dhcpcd_read_lease
>>> +#define script_runreason dhcpcd_script_runreason
>>>   #define select_profile dhcpcd_select_profile
>>>   #define set_cloexec dhcpcd_set_cloexec
>>>   #define set_nonblock dhcpcd_set_nonblock
>>> diff --git a/dhcpcd/script.c b/dhcpcd/script.c
>>> index 2de1e6347..2c184c248 100644
>>> --- a/dhcpcd/script.c
>>> +++ b/dhcpcd/script.c
>>> @@ -51,7 +51,30 @@
>>>   #include "ipv6nd.h"
>>>   #include "net.h"
>>>   #include "script.h"
>>> +#ifdef __rtems__
>>> +#include <rtems/dhcpcd.h>
>>>   +static SLIST_HEAD(, rtems_dhcpcd_hook) dhcpcd_hooks =
>>> +  SLIST_HEAD_INITIALIZER(dhcpcd_hooks);
>>> +
>>> +void
>>> +rtems_dhcpcd_add_hook(rtems_dhcpcd_hook *hook)
>>> +{
>>> +    rtems_recursive_mutex_lock(&dhcpcd_mutex);
>>> +    SLIST_INSERT_HEAD(&dhcpcd_hooks, hook, node);
>>> +    rtems_recursive_mutex_unlock(&dhcpcd_mutex);
>>> +}
>>> +
>>> +void
>>> +rtems_dhcpcd_remove_hook(rtems_dhcpcd_hook *hook)
>>> +{
>>> +    rtems_recursive_mutex_lock(&dhcpcd_mutex);
>>> +    SLIST_REMOVE(&dhcpcd_hooks, hook, rtems_dhcpcd_hook, node);
>>> +    rtems_recursive_mutex_unlock(&dhcpcd_mutex);
>>> +}
>>> +#endif /* __rtems__ */
>> This seems like a common issue we will face with FreeBSD, I am sure callouts
>> which normally exec's something would be needed else where. I am thinking of
>> wifi?
>>
>> Could this be made a common base type struct and used here?
> 
> I am not sure if the handler signature will be the same.

There is some historical logic behind scripts with argc/argv and that is it's
common interface of a command line. The rc.conf code provides a way to manage
argc/argv which can be reused.

Given this interface is to replace exec'ing a script why not call with
argc/argv? There must be exiting support to do this?

> 
>>
>>> +
>>> +#ifndef __rtems__
>>>   #define DEFAULT_PATH    "PATH=/usr/bin:/usr/sbin:/bin:/sbin"
>>>     static const char * const if_params[] = {
>>> @@ -75,10 +98,12 @@ if_printoptions(void)
>>>       for (p = if_params; *p; p++)
>>>           printf(" -  %s\n", *p);
>>>   }
>>> +#endif /* __rtems__ */
>>>     static int
>>>   exec_script(char *const *argv, char *const *env)
>>>   {
>>> +#ifndef __rtems__
>>>       pid_t pid;
>>>       posix_spawnattr_t attr;
>>>       short flags;
>>> @@ -103,6 +128,20 @@ exec_script(char *const *argv, char *const *env)
>>>           return -1;
>>>       }
>>>       return pid;
>>> +#else /* __rtems__ */
>>> +    rtems_dhcpcd_hook *hook;
>>> +    rtems_dhcpcd_hook *hook2;
>>> +
>>> +    rtems_recursive_mutex_lock(&dhcpcd_mutex);
>>> +
>>> +    SLIST_FOREACH_SAFE(hook, &dhcpcd_hooks, node, hook2) {
>>> +        (*hook->handler)(env);
>>> +    }
>>> +
>>> +    rtems_recursive_mutex_unlock(&dhcpcd_mutex);
>>> +
>>> +    return 0;
>>> +#endif /* __rtems__ */
>>>   }
>>>     #ifdef INET
>>> @@ -168,6 +207,7 @@ append_config(char ***env, ssize_t *len,
>>>   }
>>>   #endif
>>>   +#ifndef __rtems__
>>>   static size_t
>>>   arraytostr(const char *const *argv, char **s)
>>>   {
>>> @@ -191,6 +231,7 @@ arraytostr(const char *const *argv, char **s)
>>>       }
>>>       return len;
>>>   }
>>> +#endif /* __rtems__ */
>>>     static ssize_t
>>>   make_env(const struct interface *ifp, const char *reason, char ***argv)
>>> @@ -435,6 +476,7 @@ eexit:
>>>       return -1;
>>>   }
>>>   +#ifndef __rtems__
>>>   static int
>>>   send_interface1(int fd, const struct interface *iface, const char *reason)
>>>   {
>>> @@ -507,29 +549,37 @@ send_interface(int fd, const struct interface *iface)
>>>       }
>>>       return retval;
>>>   }
>>> +#endif /* __rtems__ */
>>>     int
>>>   script_runreason(const struct interface *ifp, const char *reason)
>>>   {
>>>       char *const argv[2] = { UNCONST(ifp->options->script), NULL };
>>>       char **env = NULL, **ep;
>>> +#ifndef __rtems__
>>>       char *path, *bigenv;
>>> +#endif /* __rtems__ */
>>>       ssize_t e, elen = 0;
>>>       pid_t pid;
>>>       int status = 0;
>>> +#ifndef __rtems__
>>>       const struct fd_list *fd;
>>>       struct iovec iov[2];
>>> +#endif /* __rtems__ */
>>>         if (ifp->options->script == NULL ||
>>>           ifp->options->script[0] == '\0' ||
>>>           strcmp(ifp->options->script, "/dev/null") == 0)
>>>           return 0;
>>>   +#ifndef __rtems__
>>>       syslog(LOG_DEBUG, "%s: executing `%s' %s",
>>>           ifp->name, argv[0], reason);
>>> +#endif /* __rtems__ */
>> Is there any ability to get some indication a hooked handler is being called?
> 
> You can do whatever you want in your hook handler.
> 

OK, let me rephrase the question, please do not remove the syslog messages? I
have clearly stated before the ability to use the command to debug the client is
important and these messages are part of that.

>>
>> If this due to the exec being removed?
> 
> We don't execute a script here.
> 

Sigh, yes that answer is correct given the question.

Please use 'argc/argv for the hook and please make them generic. Please add the
generic support in a way it can be reused in other parts of the stack. Thanks.

>>
>>>         /* Make our env */
>>>       elen = make_env(ifp, reason, &env);
>>> +#ifndef __rtems__
>>>       ep = realloc(env, sizeof(char *) * (elen + 2));
>>>       if (ep == NULL) {
>>>           elen = -1;
>>> @@ -554,8 +604,10 @@ script_runreason(const struct interface *ifp, const char
>>> *reason)
>>>           }
>>>       }
>>>       env[++elen] = NULL;
>>> +#endif /* __rtems__ */
>>>         pid = exec_script(argv, env);
>>> +#ifndef __rtems__
>>>       if (pid == -1)
>>>           syslog(LOG_ERR, "%s: %s: %m", __func__, argv[0]);
>>>       else if (pid != 0) {
>> Are you expecting no errors? Why not leave this in and use the result in the way
>> it is intended?
> 
> The pid is normally returned by posix_spawn() which we don't have in RTEMS.

How can the hook say something has gone wrong?

Please make the hooks return a status.

> 
>>
>>> @@ -596,6 +648,10 @@ script_runreason(const struct interface *ifp, const char
>>> *reason)
>>>       free(bigenv);
>>>     out:
>>> +#else /* __rtems__ */
>>> +    (void)e;
>>> +    (void)pid;
>>> +#endif /* __rtems__ */
>>>       /* Cleanup */
>>>       ep = env;
>>>       while (*ep)
>>> diff --git a/dhcpcd/script.h b/dhcpcd/script.h
>>> index dfd4a1726..9c5fd4d4e 100644
>>> --- a/dhcpcd/script.h
>>> +++ b/dhcpcd/script.h
>>> @@ -33,16 +33,12 @@
>>>   void if_printoptions(void);
>>>   #ifndef __rtems__
>>>   int send_interface(int, const struct interface *);
>>> -int script_runreason(const struct interface *, const char *);
>>>   #else /* __rtems__ */
>>>   static inline int send_interface(int fd, const struct interface *iface)
>>>   {
>>>       return 0;
>>>   }
>>> -static inline int script_runreason(const struct interface *ifp, const char
>>> *reason)
>>> -{
>>> -    return 0;
>>> -}
>>>   #endif /* __rtems__ */
>>> +int script_runreason(const struct interface *, const char *);
>>>     #endif
>>> diff --git a/libbsd.py b/libbsd.py
>>> index a62581faa..c842534b5 100644
>>> --- a/libbsd.py
>>> +++ b/libbsd.py
>>> @@ -4582,6 +4582,7 @@ class dhcpcd(builder.Module):
>>>                   'dhcpcd/ipv6nd.c',
>>>                   'dhcpcd/net.c',
>>>                   'dhcpcd/platform-bsd.c',
>>> +                'dhcpcd/script.c',
>>>                   'dhcpcd/compat/pselect.c',
>>>                   'dhcpcd/crypt/hmac_md5.c',
>>>               ],
>>> diff --git a/rtemsbsd/include/rtems/dhcpcd.h b/rtemsbsd/include/rtems/dhcpcd.h
>>> index 324c8a90b..c5c5cb54f 100644
>>> --- a/rtemsbsd/include/rtems/dhcpcd.h
>>> +++ b/rtemsbsd/include/rtems/dhcpcd.h
>>> @@ -40,6 +40,9 @@
>>>   #ifndef _RTEMS_DHCPCD_H_
>>>   #define _RTEMS_DHCPCD_H_
>>>   +#include <sys/cdefs.h>
>>> +#include <sys/queue.h>
>>> +
>>>   #include <rtems.h>
>>>     #ifdef __cplusplus
>>> @@ -75,6 +78,25 @@ typedef struct rtems_dhcpcd_config {
>>>    */
>>>   rtems_status_code rtems_dhcpcd_start(const rtems_dhcpcd_config *config);
>>>   +typedef struct rtems_dhcpcd_hook {
>>> +    void (*handler)(char *const *env);
          ^^ return something.  ^^ use argc/argv
>>> +    SLIST_ENTRY(rtems_dhcpcd_hook) node;
>>> +} rtems_dhcpcd_hook;
>> This looks to me like it would be useful elsewhere.
> 
> The handler signature looks quite special to me.
> 

It can be made reusable and made to work.

Chris