Skip to content
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

use ubus.STATUS_NOT_FOUND if the uspot instance has not been found. A… #20

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

pierrejoye
Copy link

Trivial litle change but handy to get the right error

Before:

ubus call 'uspotfilter' 'client_list'  "{'interface': 'captive2'}"
Command failed: Invalid argument

With STATUS_NOT_FOUND:

ubus call 'uspotfilter' 'client_list'  "{'interface': 'captive2'}"
Command failed: Not found

@f00b4r0
Copy link
Owner

f00b4r0 commented Jan 28, 2025

Good catch, but a few formatting issues with your patch commit message: plese check the commit history and reformat accordingly (loosely based on kernel commit message styling).

Also please add a SoB line (even if I myself forgot to do so on the last commit, that was a mistake not to be repeated ;)

@pierrejoye
Copy link
Author

pierrejoye commented Jan 28, 2025

Good catch, but a few formatting issues with your patch commit message: plese check the commit history and reformat accordingly (loosely based on kernel commit message styling).

Will do!

Also please add a SoB line (even if I myself forgot to do so on the last commit, that was a mistake not to be repeated ;)

Done :)

while the argument is valid

Signed-off-by: Pierre Joye <[email protected]>
@pierrejoye pierrejoye force-pushed the fix/use-ubus.status_not_found branch from 77f53bb to f722537 Compare January 28, 2025 13:16
@f00b4r0
Copy link
Owner

f00b4r0 commented Jan 30, 2025

I'm actually having second thoughts about this: I think INVALID_ARGUMENT is actually correct: while a client may be "not found", sending a wrong uspot name as parameter is invalid since uspot names are statically defined in configuration file. So I think it's better to keep things as they are so that NOT_FOUND is only used for elements that are added/removed dynamically. Otherwise there can be ambiguity between "not found" client or uspot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants