[LEDE-DEV] [PATCH 1/3][RFC] netifd: propagate error code on netifd_reload()

[Alexandru Ardelean]

On Mon, Mar 20, 2017 at 4:08 PM, Alexandru Ardelean
<ardeleanalex at gmail.com> wrote:
> From: Alexandru Ardelean <ardeleanalex at gmail.com>
>
> The context is that we generate some of the UCI config
> for netifd via scripts/programs.
>
> Every once in a while, there's a goof when doing that
> UCI generation, and netifd prints out the error at
> stderr, but returns 0 (success) err-code.
>
> This change will fail the ubus call if UCI config
> is invalid or missing for /etc/config/network.
>
> Signed-off-by: Alexandru Ardelean <ardeleanalex at gmail.com>
> ---
>  config.c | 10 ++++++++--
>  config.h |  9 ++++++++-
>  main.c   |  4 ++--
>  netifd.h |  2 +-
>  ubus.c   |  8 ++++++--
>  5 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/config.c b/config.c
> index 0d965d3..3b1af82 100644
> --- a/config.c
> +++ b/config.c
> @@ -393,16 +393,20 @@ config_init_wireless(void)
>                 vlist_flush(&wdev->interfaces);
>  }
>
> -void
> +int
>  config_init_all(void)
>  {
> +       int ret = CONFIG_INIT_OK;
> +
>         uci_network = config_init_package("network");
>         if (!uci_network) {
>                 fprintf(stderr, "Failed to load network config\n");
> -               return;
> +               return CONFIG_INIT_ERR_NO_NETWORK;
>         }
>
>         uci_wireless = config_init_package("wireless");
> +       if (!uci_wireless)
> +               ret = CONFIG_INIT_ERR_NO_WIRELESS;
>
>         vlist_update(&interfaces);
>         config_init = true;
> @@ -426,4 +430,6 @@ config_init_all(void)
>         interface_refresh_assignments(false);
>         interface_start_pending();
>         wireless_start_pending();
> +
> +       return ret;
>  }
> diff --git a/config.h b/config.h
> index 5adaca6..df30b64 100644
> --- a/config.h
> +++ b/config.h
> @@ -19,6 +19,13 @@
>
>  extern bool config_init;
>
> -void config_init_all(void);
> +enum {
> +       CONFIG_INIT_OK,
> +       CONFIG_INIT_ERR_NO_WIRELESS,
> +       CONFIG_INIT_ERR_NO_NETWORK,
> +       __CONFIG_INIT_LAST
> +};
> +
> +int config_init_all(void);
>
>  #endif
> diff --git a/main.c b/main.c
> index 5717b81..c173cef 100644
> --- a/main.c
> +++ b/main.c
> @@ -208,9 +208,9 @@ static void netifd_do_restart(struct uloop_timeout *timeout)
>         execvp(global_argv[0], global_argv);
>  }
>
> -void netifd_reload(void)
> +int netifd_reload(void)
>  {
> -       config_init_all();
> +       return config_init_all();
>  }
>
>  void netifd_restart(void)
> diff --git a/netifd.h b/netifd.h
> index 5a90858..e565423 100644
> --- a/netifd.h
> +++ b/netifd.h
> @@ -98,6 +98,6 @@ struct interface;
>  extern const char *main_path;
>  extern const char *config_path;
>  void netifd_restart(void);
> -void netifd_reload(void);
> +int netifd_reload(void);
>
>  #endif
> diff --git a/ubus.c b/ubus.c
> index 1b1a4cd..31e535c 100644
> --- a/ubus.c
> +++ b/ubus.c
> @@ -44,8 +44,12 @@ netifd_handle_reload(struct ubus_context *ctx, struct ubus_object *obj,
>                      struct ubus_request_data *req, const char *method,
>                      struct blob_attr *msg)
>  {
> -       netifd_reload();
> -       return 0;
> +       switch (netifd_reload()) {
> +               case CONFIG_INIT_ERR_NO_NETWORK:
> +                       return UBUS_STATUS_UNKNOWN_ERROR;
> +               default:
> +                       return UBUS_STATUS_OK;
> +       }
>  }
>
>  enum {
> --
> 2.7.4
>

hey,

sorry for spamming [if i gave that feeling now].
but, i prefer asking this question with a patch series with a RFC tag

one reason i went silent after my last RFC patch, is because a
colleague notified me that a reload that returns non-zero, would
trigger a restart
which would mask the error when checking if   `/etc/init.d/network
reload `  fails
a restart would not necessarily make things better [ from my point of view ]
i decided to check out [and brain storm internally] for an idea of how
to handle this

so, i am raising this point with you guys to get your take on this
i'll admit my proposals may not be the best approach to the matter

but in our internal tests, it would be nice to run
```
/etc/init.d/network reload || {
     fail_test
}
```

thanks
Alex