[PATCH v2 6/6] dhcpcd: Add hooks
Sebastian Huber
sebastian.huber at embedded-brains.de
Mon May 7 07:54:54 UTC 2018
On 07/05/18 09:31, Chris Johns wrote:
> 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?
The dhcpcd doesn't use argc and argv to pass information to the scripts.
It uses the environment. I have to check how this could be mapped.
>
>>>> +
>>>> +#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.
This module is new (script.c). It was not used before. This syslog()
message produces no useful information in our case. You can print
specific information in your hooks if you want.
>
>>> 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.
What do you want to reuse? The information passed by the dhcpcd to the
scripts is quite specific.
>
>>>> /* 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.
The dhcpcd doesn't care what your script did and if it was successful or
not. What purpose has a status code here? A generic message is not
really useful. You can print specific error messages in your hook handler.
--
Sebastian Huber, embedded brains GmbH
Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail : sebastian.huber at embedded-brains.de
PGP : Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
More information about the devel
mailing list