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

[Hans Dedecker]

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.

Hans
>
> Alex