I have made a widespread commit to flip the sense of the default
value for the 'bool secure' argument in RegisterCommand() to be true
rather than false.
Very few instances of RegisterCommand() took advantage of the argument
defaults because 'secure' defaulted to false when most instances
needed true.
In addition, the default for the 'max' arg was INT_MAX whereas most
instances should have max = 0, so that default is also changed. And
lastly, a new default value NULL was added for the 'execute' function
pointer.
It was not necessary to change any of the calls to RegisterCommand()
other than those that used the secure = false default. However, I
decided to go ahead and review all the instances in the code and
change them to make maximal use of the arg defaults so that the
existing code can serve as a useful pattern for the addition of new
commands. In particular, for secure, non-terminal commands, such as
the top-level "framework" commands, the model should simply be:
RegisterCommand("name", "Title");
For secure, terminal (sub)commands that don't require any additional
parameters, the model should be:
RegisterCommand("name", "Title", execute);
This model also applies if the command has children but the command
itself wants to execute a default operation if no subcommand is
specified. It is incorrect to specify min = 0, max = 1 to indicate an
optional subcommand; that is indicated by the presence of the execute
function at the same time as a non-empty children array. The default
usage string will list all the children.
Any command with required or optional parameters should provide a
usage string hinting about the parameters in addition to specifying
the min and max number of parameters:
RegisterCommand("name", "Title", execute, "usage", min, max);
I found several instances of commands with parameters that left the
usage string empty. If you recognize some of these as yours, please
add hints for the parameters in the usage string:
cmd_ecu->RegisterCommand("list","Show OBDII ECU pid list",obd2ecu_list, "", 0, 1);
cmd_re->RegisterCommand("list","List RE records",re_list, "", 0, 1);
cmd_dbc->RegisterCommand("list","List RE DBC records",re_dbc_list, "", 0, 1);
cmd_discover_list->RegisterCommand("changed","List changed records",re_list_changed, "", 0, 1);
cmd_discover_list->RegisterCommand("discovered","List discovered records",re_list_discovered, "", 0, 1);
cmd_test->RegisterCommand("strverscmp", "Test strverscmp function", test_strverscmp, "", 2, 2);
The following one includes a usage string, but since the parameter is
optional (indicated by min = 0), the usage should be "[<url>]", so I
made that change:
cmd_otaflash->RegisterCommand("http","OTA flash http",ota_flash_http,"<url>",0,1,true);
The iMiEV and Kia Soul command hierarchies marked the top-level
command as secure but all the subcommands as not secure. This does
not make sense as the subcommands could not be reached if not
enabled. Furthermore, a subcommand to unlock the doors should be
secure indeed. This commit changes all the subcommands to be secure.
Similarly, the 'status' subcommands for 'server v2' and 'server v3'
were marked not secure, apparently with the intention of letting the
status be examined without enabling, but the top-level commands were
marked secure so the status command could not be reached. The status
subcommands are now marked secure.
-- Steve
