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

Submitted by Alexandru Ardelean on March 20, 2017, 2:08 p.m.

Details

Message ID 1490018935-16947-1-git-send-email-alexandru.ardelean@riverbed.com
State New
Delegated to: Felix Fietkau
Headers show

Commit Message

Alexandru Ardelean March 20, 2017, 2:08 p.m.
From: Alexandru Ardelean <ardeleanalex@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@gmail.com>
---
 config.c | 10 ++++++++--
 config.h |  9 ++++++++-
 main.c   |  4 ++--
 netifd.h |  2 +-
 ubus.c   |  8 ++++++--
 5 files changed, 25 insertions(+), 8 deletions(-)

Comments

Alexandru Ardelean March 20, 2017, 2:15 p.m.
On Mon, Mar 20, 2017 at 4:08 PM, Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
> From: Alexandru Ardelean <ardeleanalex@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@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
Hans Dedecker March 26, 2017, 4:06 p.m.
On Mon, Mar 20, 2017 at 3:08 PM, Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
> From: Alexandru Ardelean <ardeleanalex@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@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
>
Alexandru Ardelean March 26, 2017, 4:21 p.m.
On Sun, Mar 26, 2017 at 7:06 PM, Hans Dedecker <dedeckeh@gmail.com> wrote:
> On Mon, Mar 20, 2017 at 3:08 PM, Alexandru Ardelean
> <ardeleanalex@gmail.com> wrote:
>> From: Alexandru Ardelean <ardeleanalex@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@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.

Alex
Hans Dedecker March 26, 2017, 4:38 p.m.
On Sun, Mar 26, 2017 at 6:21 PM, Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
> On Sun, Mar 26, 2017 at 7:06 PM, Hans Dedecker <dedeckeh@gmail.com> wrote:
>> On Mon, Mar 20, 2017 at 3:08 PM, Alexandru Ardelean
>> <ardeleanalex@gmail.com> wrote:
>>> From: Alexandru Ardelean <ardeleanalex@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@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
Alexandru Ardelean March 26, 2017, 4:45 p.m.
On Sun, Mar 26, 2017 at 7:38 PM, Hans Dedecker <dedeckeh@gmail.com> wrote:
> On Sun, Mar 26, 2017 at 6:21 PM, Alexandru Ardelean
> <ardeleanalex@gmail.com> wrote:
>> On Sun, Mar 26, 2017 at 7:06 PM, Hans Dedecker <dedeckeh@gmail.com> wrote:
>>> On Mon, Mar 20, 2017 at 3:08 PM, Alexandru Ardelean
>>> <ardeleanalex@gmail.com> wrote:
>>>> From: Alexandru Ardelean <ardeleanalex@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@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
Hans Dedecker March 26, 2017, 6:51 p.m.
On Sun, Mar 26, 2017 at 6:45 PM, Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
> On Sun, Mar 26, 2017 at 7:38 PM, Hans Dedecker <dedeckeh@gmail.com> wrote:
>> On Sun, Mar 26, 2017 at 6:21 PM, Alexandru Ardelean
>> <ardeleanalex@gmail.com> wrote:
>>> On Sun, Mar 26, 2017 at 7:06 PM, Hans Dedecker <dedeckeh@gmail.com> wrote:
>>>> On Mon, Mar 20, 2017 at 3:08 PM, Alexandru Ardelean
>>>> <ardeleanalex@gmail.com> wrote:
>>>>> From: Alexandru Ardelean <ardeleanalex@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@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

Patch hide | download patch | download mbox

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 {