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

[Hans Dedecker]

On Sun, Mar 26, 2017 at 6:45 PM, Alexandru Ardelean
<ardeleanalex at gmail.com> wrote:
> 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
OK I will check with Felix

Hans
>
>
> Alex
>
>>
>> Hans
>>>
>>> Alex