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

[Alexandru Ardelean]

On Sun, Mar 26, 2017 at 7:38 PM, Hans Dedecker <dedeckeh at gmail.com> wrote:
> On Sun, Mar 26, 2017 at 6:21 PM, Alexandru Ardelean
> <ardeleanalex at gmail.com> wrote:
>> On Sun, Mar 26, 2017 at 7:06 PM, Hans Dedecker <dedeckeh at gmail.com> wrote:
>>> On Mon, Mar 20, 2017 at 3: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;
>>> Is there any reason why only an ubus error code is returned for a
>>> network uci failure and not for a wireless uci failure ?
>>>
>>> Hans
>>>> +               default:
>>>> +                       return UBUS_STATUS_OK;
>>>> +       }
>>>>  }
>>>>
>>>>  enum {
>>>> --
>>>> 2.7.4
>>>>
>>
>>
>> That would make the "ubus call network reload" return non-zero.
>> Hmm, I guess that would not be a problem.
>> I interpreted Felix's comment a bit differently regarding not failing
>> the wireless config.
> I understood Felix's comment as still load the network config in case
> a wireless uci error is raised; but I would still return an error
> code.. This would make the CONFIG_INIT enum definition superfluous and
> will simplify the code.

ack ; will update

the main thing i would like to get some input on atm [before
re-spinning], is the base-files change in rc.common [ regarding reload
& restart ]
i'm not too confident about it ; and it's a subtle change that can
have a big impact on processes


Alex

>
> Hans
>>
>> Alex