diff mbox series

[OpenWrt-Devel] base-files: make wifi report unknown command

Message ID 20180803161839.2314-1-hacks@slashdirt.org
State Rejected
Delegated to: John Crispin
Headers show
Series [OpenWrt-Devel] base-files: make wifi report unknown command | expand

Commit Message

Thibaut Aug. 3, 2018, 4:18 p.m. UTC
Avoid having /sbin/wifi silently ignore unknown keywords and execute
"enable"; instead display the help message and exit with an error.

Signed-off-by: Thibaut VARÈNE <hacks@slashdirt.org>
---
 package/base-files/files/sbin/wifi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

John Crispin Aug. 6, 2018, 5:20 a.m. UTC | #1
On 03/08/18 18:18, Thibaut VARÈNE wrote:
> Avoid having /sbin/wifi silently ignore unknown keywords and execute
> "enable"; instead display the help message and exit with an error.
>
> Signed-off-by: Thibaut VARÈNE <hacks@slashdirt.org>
> ---
>   package/base-files/files/sbin/wifi | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/package/base-files/files/sbin/wifi b/package/base-files/files/sbin/wifi
> index 83befc0d6f..09e483ec55 100755
> --- a/package/base-files/files/sbin/wifi
> +++ b/package/base-files/files/sbin/wifi
> @@ -241,5 +241,5 @@ case "$1" in
>   	reload) wifi_reload "$2";;
>   	reload_legacy) wifi_reload_legacy "$2";;
>   	--help|help) usage;;
> -	*) ubus call network reload; wifi_updown "enable" "$2";;
> +	*) usage; exit 1;;
>   esac

NAK, this changes expected behaviour. i regularly call "wifi" to resync 
my config with runstate.
     John
Karl Palsson Aug. 6, 2018, 2:49 p.m. UTC | #2
John Crispin <john@phrozen.org> wrote:
> 
> 
> On 03/08/18 18:18, Thibaut VARÈNE wrote:
> > Avoid having /sbin/wifi silently ignore unknown keywords and execute
> > "enable"; instead display the help message and exit with an error.
> >
> > Signed-off-by: Thibaut VARÈNE <hacks@slashdirt.org>
> > ---
> >   package/base-files/files/sbin/wifi | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/package/base-files/files/sbin/wifi b/package/base-files/files/sbin/wifi
> > index 83befc0d6f..09e483ec55 100755
> > --- a/package/base-files/files/sbin/wifi
> > +++ b/package/base-files/files/sbin/wifi
> > @@ -241,5 +241,5 @@ case "$1" in
> >   	reload) wifi_reload "$2";;
> >   	reload_legacy) wifi_reload_legacy "$2";;
> >   	--help|help) usage;;
> > -	*) ubus call network reload; wifi_updown "enable" "$2";;
> > +	*) usage; exit 1;;
> >   esac
> 
> NAK, this changes expected behaviour. i regularly call "wifi"
> to resync my config with runstate.
>      John

respectfully, the behaviour of the "wifi" command is one of the most obtuse parts of openwrt's tooling.  It does "something" with no command output, and responds ~instantly.    This is expected behaviour for very few people.  even "wifi asdfasdfa" returns ~instantly, with no warning that it is an unknown command.  Does the command perhaps not take arguments?  What would they be?  

Do we _reallly_ have to keep it's behaviour this way? 

What this patch _is_ missing, is a new command to replace the old
"anything but the other commands".

ie, the one that does "ubus call network reload; wifi_updown
"enable" "$2""

Sincerely,
Karl Palsson
Alberto Bursi Aug. 6, 2018, 2:58 p.m. UTC | #3
On 06/08/2018 07:20, John Crispin wrote:
>
>
> On 03/08/18 18:18, Thibaut VARÈNE wrote:
>> Avoid having /sbin/wifi silently ignore unknown keywords and execute
>> "enable"; instead display the help message and exit with an error.
>>
>> Signed-off-by: Thibaut VARÈNE <hacks@slashdirt.org>
>> ---
>>   package/base-files/files/sbin/wifi | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/package/base-files/files/sbin/wifi 
>> b/package/base-files/files/sbin/wifi
>> index 83befc0d6f..09e483ec55 100755
>> --- a/package/base-files/files/sbin/wifi
>> +++ b/package/base-files/files/sbin/wifi
>> @@ -241,5 +241,5 @@ case "$1" in
>>       reload) wifi_reload "$2";;
>>       reload_legacy) wifi_reload_legacy "$2";;
>>       --help|help) usage;;
>> -    *) ubus call network reload; wifi_updown "enable" "$2";;
>> +    *) usage; exit 1;;
>>   esac
>
> NAK, this changes expected behaviour. i regularly call "wifi" to 
> resync my config with runstate.
>     John
>

There are also scripts in packages (like wifitoggle) that expect to call 
"wifi" without arguments to do a status refresh.

Please make it print an error only if "wifi" is actually called with an 
unknown (=wrong) argument, calling "wifi" with no argument is ok.

-Alberto
Kevin Darbyshire-Bryant Aug. 7, 2018, 8:55 a.m. UTC | #4
> On 6 Aug 2018, at 15:49, Karl Palsson <karlp@tweak.net.au> wrote:
> 
> Signed PGP part
> 
> John Crispin <john@phrozen.org> wrote:
>> 
>> 
>> On 03/08/18 18:18, Thibaut VARÈNE wrote:
>>> Avoid having /sbin/wifi silently ignore unknown keywords and execute
>>> "enable"; instead display the help message and exit with an error.
>>> 
>>> Signed-off-by: Thibaut VARÈNE <hacks@slashdirt.org>
>>> ---
>>>  package/base-files/files/sbin/wifi | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/package/base-files/files/sbin/wifi b/package/base-files/files/sbin/wifi
>>> index 83befc0d6f..09e483ec55 100755
>>> --- a/package/base-files/files/sbin/wifi
>>> +++ b/package/base-files/files/sbin/wifi
>>> @@ -241,5 +241,5 @@ case "$1" in
>>>  	reload) wifi_reload "$2";;
>>>  	reload_legacy) wifi_reload_legacy "$2";;
>>>  	--help|help) usage;;
>>> -	*) ubus call network reload; wifi_updown "enable" "$2";;
>>> +	*) usage; exit 1;;
>>>  esac
>> 
>> NAK, this changes expected behaviour. i regularly call "wifi"
>> to resync my config with runstate.
>>     John
> 
> respectfully, the behaviour of the "wifi" command is one of the most obtuse parts of openwrt's tooling.  It does "something" with no command output, and responds ~instantly.    This is expected behaviour for very few people.  even "wifi asdfasdfa" returns ~instantly, with no warning that it is an unknown command.  Does the command perhaps not take arguments?  What would they be?
> 
> Do we _reallly_ have to keep it's behaviour this way?
> 
> What this patch _is_ missing, is a new command to replace the old
> "anything but the other commands".
> 
> ie, the one that does "ubus call network reload; wifi_updown
> "enable" "$2""
> 
> Sincerely,
> Karl Palsson
> 

+1
Nicely written and argued.  ‘wifi’ is incredibly obtuse and unfriendly.  I hate running it.  Does ‘magic’ no idea what, it doesn’t say.



Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A
Jo-Philipp Wich Aug. 7, 2018, 9:06 a.m. UTC | #5
Hi,

> respectfully, the behaviour of the "wifi" command is one of the most
> obtuse parts of openwrt's tooling.  It does "something" with no
> command output, and responds ~instantly.    This is expected
> behaviour for very few people.  even "wifi asdfasdfa" returns
> ~instantly, with no warning that it is an unknown command.  Does the
> command perhaps not take arguments?  What would they be?

The (implicit) default command could be explicitly spelled out as "wifi
apply" or similar, mentioned in the usage and called when no argument is
provided.

Is that what you were thinking?

~ Jo
Karl Palsson Aug. 7, 2018, 9:48 a.m. UTC | #6
Jo-Philipp Wich <jo@mein.io> wrote:
> Hi,
> 
> > respectfully, the behaviour of the "wifi" command is one of the most
> > obtuse parts of openwrt's tooling.  It does "something" with no
> > command output, and responds ~instantly.    This is expected
> > behaviour for very few people.  even "wifi asdfasdfa" returns
> > ~instantly, with no warning that it is an unknown command.  Does the
> > command perhaps not take arguments?  What would they be?
> 
> The (implicit) default command could be explicitly spelled out
> as "wifi apply" or similar, mentioned in the usage and called
> when no argument is provided.
> 
> Is that what you were thinking?

Personally I'd like to see the "apply" (or any other name) added,
and the implicit action dropped. Or at the very least, the
implicit action only taken when there are no arguments. At
present, you get the implicit action with no args, and with any
arg that doesn't match some other args. I'd suggest it is more
reasonable that unknown args are not treated as some default,
which is what the original patch suggested. It was definitely
broken however in that the implicit step was no longer available.


Something like this say....

This lets "wifi" still do the same as before.
This makes "wifi adsfads" print usage arguments
This makes "wifi enable interfacename" do the same as "wifi
interfacename" did before, if anyone knew about that.

The usage text could/should be expanded still, but hoenstly, I
don't know the difference between them all. What's the difference
between config and detect? should detect be listed in the usage?
what's reload_legacy? when would I want it? Should it be listed
in the usage? What's the difference between enable and reload?
Why is there down but no up? (it's like "enable" but not quite?)

Sincerely,
Karl Palsson

diff --git a/package/base-files/files/sbin/wifi
b/package/base-files/files/sbin/wifi index 83befc0d6f..cbe7e950bd
100755
--- a/package/base-files/files/sbin/wifi
+++ b/package/base-files/files/sbin/wifi
@@ -6,7 +6,7 @@
 
 usage() {
        cat <<EOF
-Usage: $0 [config|down|reload|status]
+Usage: $0 [config|down|enable|reload|status]
 enables (default), disables or configures devices not yet configured.
 EOF
        exit 1
@@ -233,6 +233,13 @@ DRIVERS=
 include /lib/wifi
 scan_wifi
 
+[ $# -lt 2 ] && {
+       echo "Implicitly applying config"
+       ubus call network reload
+       wifi_updown "enable"
+       exit 0
+}
+
 case "$1" in
        down) wifi_updown "disable" "$2";;
        detect) wifi_detect_notice ;;
@@ -241,5 +248,6 @@ case "$1" in
        reload) wifi_reload "$2";;
        reload_legacy) wifi_reload_legacy "$2";;
        --help|help) usage;;
-       *) ubus call network reload; wifi_updown "enable" "$2";;
+       enable) ubus call network reload; wifi_updown "enable" "$2";;
+       *) usage; exit 1;;
 esac
diff mbox series

Patch

diff --git a/package/base-files/files/sbin/wifi b/package/base-files/files/sbin/wifi
index 83befc0d6f..09e483ec55 100755
--- a/package/base-files/files/sbin/wifi
+++ b/package/base-files/files/sbin/wifi
@@ -241,5 +241,5 @@  case "$1" in
 	reload) wifi_reload "$2";;
 	reload_legacy) wifi_reload_legacy "$2";;
 	--help|help) usage;;
-	*) ubus call network reload; wifi_updown "enable" "$2";;
+	*) usage; exit 1;;
 esac