-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shrared state Async ubus interface #1086
Conversation
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing new line at the end of file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my inline comments
@@ -0,0 +1,15 @@ | |||
{ | |||
"lime-app": { | |||
"description": "lime-app public access", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we explicitely mention lime-app here? other components could benefit to access shared-state-async via ubus too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While logged in via ssh, you have direct, full access to ubus. This file is to enable lime app to use it. other users should enable other profiles.
|
||
local ERROR_LOG_FILE_NAME = "/tmp/shared_state_rpcd_error.log" | ||
|
||
local function parseErrors() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way of handling errors doesn't seems robust to me :-/, what if a few calls are performed in parallel and some fails and some not?
shared-state-async command return the error code to the calling shell if some error happens, in that case the error should be bubbled up without more parsing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this prove too cumbersome to do in Lua, consider rewriting the whole ubus plugin using ucode, or ash which are supported by default by OpenWrt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit e891527 implements a new way of handling, what do you think?
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1086 +/- ##
==========================================
+ Coverage 79.55% 79.64% +0.08%
==========================================
Files 53 53
Lines 4564 4579 +15
==========================================
+ Hits 3631 3647 +16
+ Misses 933 932 -1 ☔ View full report in Codecov by Sentry. |
local function get(msg) | ||
local error = os.execute("shared-state-async get " .. | ||
msg.data_type .. " 2>/dev/null ") | ||
utils.printJson({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That the error get printed only on failure happens implicitely, please add at least a comment documentig this behaviour, so we cna understand what's happening when we look at this code in a few months or years ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ! it is a weird behavior that gives a lot of headaches. Any output that is printed in the console is used as output. If its a json everything goes fine, if it is not a json you get "invalid input" as a response.
This pull request adds shrare-state-async ubus interface. Get method returns sharedstate "get" json data or reported error in case no json is found. Errors are bubbled up as they come from sharedstate inside a json structure with the name "error".
Since there is no Shared State async binary available in the testing environment, testing possibilities are reduced. Manual testing can be done on a router with bat-hosts package using this commands:
Get bat-hosts
Get an invalid data type
Sync valid data type
Sync invalid data type
Sync valid data type with unreachable ipv4 addresses
Sync valid data type with invalid ipv4 address
Sync invalid data type with specified ipv4 address
Sync valid data type with reachable ipv4 addresses