[Ovmsdev] Warning in Duktape
frog at bunyip.wheelycreek.net
frog at bunyip.wheelycreek.net
Tue Aug 6 18:15:12 HKT 2024
That seems sensible - I’ve implemented something close to your suggestion with a small ‘improvement’:
I’ve made it so the ‘usr’ commands are not there by default. But will be auto-created as necessary.
To do this, rather than a Boolean allow_user_commands I added an enum as follows:
enum class OvmsCommandType {
System, // << Normal system command – can’t have usr subcommand.
SystemAllowUserCmd, //< System command that Allows user commands to be added to it
SystemAllowUsrDir, //< System command that allow ‘usr’ sub-command to be auto-added.
SystemUsrDir, //< an auto-added usr cub-command
User //< A user command.
};
By default I have the root command as SystemAllowUsrDir as well as I’ve done it for Ioniq 5 as well (to test that bit).
The calls
OvmsCommand.Register(mycommand, "", "me", "My Script", "[string]", 0, 4)
OvmsCommand.Register(mycommand, "xhiq", "extra", "My Script", "[string]", 0, 4)
Would mean the commands ‘usr me’ and ‘xhiq usr extra’ would be created. Exactly the same as
OvmsCommand.Register(mycommand, "usr", "me", "My Script", "[string]", 0, 4)
OvmsCommand.Register(mycommand, "xhiq usr", "extra", "My Script", "[string]", 0, 4)
Thoughts?
Btw to add a sub-command to xhiq usr extra you would need to specify it as “xhiq usr extra” as the parent.
//.ichael
From: OvmsDev <ovmsdev-bounces at lists.openvehicles.com> On Behalf Of Michael Balzer via OvmsDev
Sent: Saturday, August 3, 2024 3:09 PM
To: ovmsdev at lists.openvehicles.com
Cc: Michael Balzer <dexter at expeedo.de>
Subject: Re: [Ovmsdev] Warning in Duktape
I've just merged your PR.
`OvmsCommand.Register()` is well designed.
On security: I don't think we actually need to allow adding user commands to system command groups at all.
I'd opt for allowing only command root "usr", and optionally "x…" (vehicle specific namespaces), for user commands. Allowing "x…" should be a choice/config of the vehicle module.
We could add a flag to the command definition and extend the registration accordingly to…
OvmsCommand* RegisterCommand(const char* name, const char* title,
OvmsCommandExecuteCallback_t execute = NULL,
const char *usage = "", int min = 0, int max = 0, bool secure = true,
OvmsCommandValidateCallback_t validate = NULL,
bool allow_user_commands = false);
Similar to the config param "usr" init, we then generally create an empty command root "usr" on system init with allow_user_commands = true.
Vehicles can decide to create their full "x…" root, or some "x… usr" placeholder with that grant.
What do you think?
Regards,
Michael
Am 22.07.24 um 02:36 schrieb Michael Geddes via OvmsDev:
I have tidied up the implementation significantly and I've fixed up all the life-time issues, as well as disallowing overriding existing commands.
(Though you could still add a sub-command to a system command). It now persists quite happily, doesn't crash on GC and functions as expected :). I've also implemented shut-down registering so it cleans up before getting to the destructor.
It's now down to security and the question of whether we differentiate user plugin commands from system ones.
//.ichael
On Sun, 21 Jul 2024 at 08:34, Michael Geddes <frog at bunyip.wheelycreek.net <mailto:frog at bunyip.wheelycreek.net> > wrote:
Well since I've been doing some Duktape stuff lately, I recalled this loose end and have come up with a solution.
There were a few issues - please let me know if you have a problem with any of them.
Firstly the Command registration map is using const char * and we can't guarantee the lifetime of the duktape strings so I've moved them over to std::string.
This applies to OvmsCommandMap itself as well as OvmsCommand - m_name, m_title and m_usage_template.
I think the intention was to have something like this for the register command.. it was missing a parameter. It now looks like this:
OvmsCommand.Register( function, parent, command, description, parameter_desc, minargs, maxargs).
Missing maxargs will mean maxargs = minargs.
Missing minargs will mean no parameters.
The calling function will be passed 2 parameters: the name of the command and an array of strings (the argv).
For sub-commands, these will be separated by '/'. So something like "me/too".
I think this way is simple but effective.
It would be good to add completion, but that can be added later.
One of the things I wanted to talk about is what security protections we need to add to prevent a script from taking over an existing function! (or future function).
When vim initially added user commands and functions it was a free-for-all but somebody soon realised there needed to be some restrictions to avoid issues..
so what they did was force user function and commands to start with a capital letter. I would like people's thoughts on whether we need to do something similar.
It might be that we would be satisfied with failing if system commands were attempted to be overridden. Should we allow adding sub-commands to system commands?
I can see that might be useful..
I have added a sample below of a working function:
--8<----
mycommand = function(c, argv){
print("Hello There: "+c+"\n");
if (argv.length == 0)
print("Simple\n");
else
print("Script: "+argv[0]+"\n")
}
OvmsCommand.Register(mycommand, "", "me", "My Script", "[string]", 0, 3)
OVMS# me xx
Hello There: me
Script: xx
OVMS# me xx yy
Hello There: me
Script: xx
OVMS# me xx yy
Hello There: me
Script: xx
OVMS#
On Sun, 23 Oct 2022 at 22:15, Michael Balzer <dexter at expeedo.de <mailto:dexter at expeedo.de> > wrote:
That's an open end, implementing command registration for Duktape.
There's a thread on this… found it:
http://lists.openvehicles.com/pipermail/ovmsdev/2020-July/006952.html
Regards,
Michael
Am 23.10.22 um 15:38 schrieb Michael Geddes:
There's an unused variable 'dcc' warning in the below code.
It seems that the 'Perform the callback' is not being 'performed!
Is this deliberate?
//.ichael
void DukOvmsCommandRegisterRun(int verbosity, OvmsWriter* writer, OvmsCommand* cmd, int argc, const char* const* argv)
{
ESP_LOGD(TAG, "DukOvmsCommandRegisterRun(%s)",cmd->GetName());
auto it = MyDuktape.m_cmdmap.find(cmd);
if (it == MyDuktape.m_cmdmap.end())
{
ESP_LOGE(TAG, "Command '%s' cannot be found in registry",cmd->GetName());
return;
}
else
{
DuktapeConsoleCommand* dcc = it->second;
// Perform the callback
}
}
_______________________________________________
OvmsDev mailing list
OvmsDev at lists.openvehicles.com <mailto:OvmsDev at lists.openvehicles.com>
http://lists.openvehicles.com/mailman/listinfo/ovmsdev
--
Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
Fon 02333 / 833 5735 * Handy 0176 / 206 989 26
_______________________________________________
OvmsDev mailing list
OvmsDev at lists.openvehicles.com <mailto:OvmsDev at lists.openvehicles.com>
http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________
OvmsDev mailing list
OvmsDev at lists.openvehicles.com <mailto:OvmsDev at lists.openvehicles.com>
http://lists.openvehicles.com/mailman/listinfo/ovmsdev
--
Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
Fon 02333 / 833 5735 * Handy 0176 / 206 989 26
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20240806/312b8f2b/attachment-0001.htm>
More information about the OvmsDev
mailing list