| Message ID | 1490018935-16947-1-git-send-email-alexandru.ardelean@riverbed.com |
|---|---|
| State | Superseded |
| Delegated to: | Felix Fietkau |
| Headers | show |
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
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 >
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
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
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
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
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 {