[LEDE-DEV] The procd daemon does not handle correctly the instances

[Lebleu Pierre]

Hi,

Thank you for your answer.

First, I am going to clarify a little bit what I am trying to do.

I would like to have more than one instance for a given daemon (here, dnsmasq).
I would like to stop one instance when I need.
I would like to start one instance without stopping the other one.

root at LEDE:~# ps w | grep dnsmasq
11633 dnsmasq   1560 S    /usr/sbin/dnsmasq -C /var/etc/dnsmasq.conf.main -k -x /var/run/dnsmasq/dnsmasq.main.pid
11634 dnsmasq   1556 S    /usr/sbin/dnsmasq -C /var/etc/dnsmasq.conf.second -k -x /var/run/dnsmasq/dnsmasq.second.pid
11636 root      1556 S    /usr/sbin/dnsmasq -C /var/etc/dnsmasq.conf.main -k -x /var/run/dnsmasq/dnsmasq.main.pid
11660 root      1712 R    grep dnsmasq
root at LEDE:~# /etc/init.d/dnsmasq start main
root at LEDE:~# ps w | grep dnsmasq
11633 dnsmasq   1560 S    /usr/sbin/dnsmasq -C /var/etc/dnsmasq.conf.main -k -x /var/run/dnsmasq/dnsmasq.main.pid
11636 root      1556 S    /usr/sbin/dnsmasq -C /var/etc/dnsmasq.conf.main -k -x /var/run/dnsmasq/dnsmasq.main.pid
11715 root      1712 R    grep dnsmasq
root at LEDE:~# /etc/init.d/dnsmasq start second
root at LEDE:~# ps w | grep dnsmasq
11753 dnsmasq   1556 S    /usr/sbin/dnsmasq -C /var/etc/dnsmasq.conf.second -k -x /var/run/dnsmasq/dnsmasq.second.pid
11757 root      1712 R    grep dnsmasq

As you can see, when I want to start the second instance : it starts it but it also kills the first instance...

I had a look at the procd code and it seems it does not flush the other instance if we call the method "add". Am I right ?

Regarding the procd.sh file, it seems there is a lack of something. Indeed, we can call "stop myinstance" but we can't call "start myinstance".

Here, please find my modification :

diff --git a/package/base-files/files/etc/rc.common b/package/base-files/files/etc/rc.common
index e0de073..8cc6e69 100755
--- a/package/base-files/files/etc/rc.common
+++ b/package/base-files/files/etc/rc.common
@@ -102,9 +102,11 @@ ${INIT_TRACE:+set -x}
        . $IPKG_INSTROOT/lib/functions/procd.sh
        basescript=$(readlink "$initscript")
        rc_procd() {
+               local method="set"
+               [ -n "$2" ] && method="add"
                procd_open_service "$(basename ${basescript:-$initscript})" "$initscript"
                "$@"
-               procd_close_service
+               procd_close_service $method
        }
 
        start() {
diff --git a/package/system/procd/files/procd.sh b/package/system/procd/files/procd.sh
index fa6f8a9..1af9c7b 100644
--- a/package/system/procd/files/procd.sh
+++ b/package/system/procd/files/procd.sh
@@ -72,11 +72,15 @@ _procd_open_service() {
 }
 
 _procd_close_service() {
+       local method="set"
+       [ -n "$1" ] && method="$1"
+
        json_close_object
        _procd_open_trigger
        service_triggers
        _procd_close_trigger
-       _procd_ubus_call set
+
+       _procd_ubus_call $method
 }
 
 _procd_add_array_data() {


With this patch, it seems to work.

Pierre
-----Original Message-----
From: John Crispin [mailto:john at phrozen.org] 
Sent: dinsdag 27 september 2016 17:48
To: Lebleu Pierre <Pierre.Lebleu at technicolor.com>; lede-dev at lists.infradead.org
Subject: Re: [LEDE-DEV] The procd daemon does not handle correctly the instances



On 27/09/2016 17:34, Lebleu Pierre wrote:
> Hi all,
> 
> I found a bug in the daemon process management. Indeed, when we have several instances of one daemon such as "dnsmasq" and we want to restart only one instance, the other instance is killed by procd. It seems procd updates the second instance and then, kill it.
> 
> Please find the patch :
> diff --git a/service/service.c b/service/service.c index 
> 0796adb..9ed07da 100644
> --- a/service/service.c
> +++ b/service/service.c
> @@ -132,8 +132,6 @@ service_update(struct service *s, struct blob_attr **tb, bool add)
>         }
> 
>         if (tb[SERVICE_SET_INSTANCES]) {
> -               if (!add)
> -                       vlist_update(&s->instances);
>                 blobmsg_for_each_attr(cur, tb[SERVICE_SET_INSTANCES], rem) {
>                         service_instance_add(s, cur);
>                 }

this bit is certainly not correct. it simply deactivates the feature for all services. the actual problem is that procd stores the instances inside a vlist. this has an update mechanism triggered inside service_instance_update().

what you want to do is compare in_o and in_n and only trigger
instance_update() if there really was a change. if there is no change then simply migrate the pid over to the new instance.


> @@ -238,7 +236,7 @@ service_handle_set(struct ubus_context *ctx, struct ubus_object *obj,
>         int ret;
> 
>         blobmsg_parse(service_set_attrs, __SERVICE_SET_MAX, tb, blob_data(msg), blob_len(msg));
> -       cur = tb[SERVICE_ATTR_NAME];
> +       cur = tb[SERVICE_SET_NAME];
>         if (!cur)
>                 return UBUS_STATUS_INVALID_ARGUMENT;

this is a copy paste error but luckily does not cause an error as SET and ATTR both eval to 0. i have merged this part as a fix.

	John


> 
> 
> Regards,
> 
> Pierre
> 
> 
> _______________________________________________
> Lede-dev mailing list
> Lede-dev at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev
>