| Message ID | 1489144286-29119-1-git-send-email-ardeleanalex@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 2017-03-10 12:11, Alexandru Ardelean wrote: > 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 for /etc/config/network & /etc/config/wireless, > or could not be loaded (via uci_load()). > > Admittedly, failing the entire ubus call could > be a bit much [depending on various views]. > Probably one idea, would be to return the err-code > as a ubus reply data (via ubus_send_reply()). > Not sure. > > Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> > --- > config.c | 10 ++++++++-- > config.h | 2 +- > main.c | 4 ++-- > netifd.h | 2 +- > ubus.c | 5 +++-- > 5 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/config.c b/config.c > index 0d965d3..d70747c 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) > { > uci_network = config_init_package("network"); > if (!uci_network) { > fprintf(stderr, "Failed to load network config\n"); > - return; > + return -1; > } > > uci_wireless = config_init_package("wireless"); > + if (!uci_wireless) { > + fprintf(stderr, "Failed to load wireless config\n"); > + return -1; > + } > > 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 0; > } Failing the network config setup just because there is no wireless config (or an invalid one) is a bad idea. Please use a 'ret' variable to indicate the error without affecting runtime behavior. - Felix
On Fri, Mar 10, 2017 at 1:13 PM, Felix Fietkau <nbd@nbd.name> wrote: > On 2017-03-10 12:11, Alexandru Ardelean wrote: >> 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 for /etc/config/network & /etc/config/wireless, >> or could not be loaded (via uci_load()). >> >> Admittedly, failing the entire ubus call could >> be a bit much [depending on various views]. >> Probably one idea, would be to return the err-code >> as a ubus reply data (via ubus_send_reply()). >> Not sure. >> >> Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> >> --- >> config.c | 10 ++++++++-- >> config.h | 2 +- >> main.c | 4 ++-- >> netifd.h | 2 +- >> ubus.c | 5 +++-- >> 5 files changed, 15 insertions(+), 8 deletions(-) >> >> diff --git a/config.c b/config.c >> index 0d965d3..d70747c 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) >> { >> uci_network = config_init_package("network"); >> if (!uci_network) { >> fprintf(stderr, "Failed to load network config\n"); >> - return; >> + return -1; >> } >> >> uci_wireless = config_init_package("wireless"); >> + if (!uci_wireless) { >> + fprintf(stderr, "Failed to load wireless config\n"); >> + return -1; >> + } >> >> 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 0; >> } > Failing the network config setup just because there is no wireless > config (or an invalid one) is a bad idea. > Please use a 'ret' variable to indicate the error without affecting > runtime behavior. > > - Felix ack my bad
diff --git a/config.c b/config.c index 0d965d3..d70747c 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) { uci_network = config_init_package("network"); if (!uci_network) { fprintf(stderr, "Failed to load network config\n"); - return; + return -1; } uci_wireless = config_init_package("wireless"); + if (!uci_wireless) { + fprintf(stderr, "Failed to load wireless config\n"); + return -1; + } 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 0; } diff --git a/config.h b/config.h index 5adaca6..fae7cd8 100644 --- a/config.h +++ b/config.h @@ -19,6 +19,6 @@ extern bool config_init; -void config_init_all(void); +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 66f714a..c9813fc 100644 --- a/ubus.c +++ b/ubus.c @@ -44,8 +44,9 @@ 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; + if (netifd_reload()) + return UBUS_STATUS_UNKNOWN_ERROR; + return UBUS_STATUS_OK; } enum {
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 for /etc/config/network & /etc/config/wireless, or could not be loaded (via uci_load()). Admittedly, failing the entire ubus call could be a bit much [depending on various views]. Probably one idea, would be to return the err-code as a ubus reply data (via ubus_send_reply()). Not sure. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> --- config.c | 10 ++++++++-- config.h | 2 +- main.c | 4 ++-- netifd.h | 2 +- ubus.c | 5 +++-- 5 files changed, 15 insertions(+), 8 deletions(-)