[Ovmsdev] Warning in Duktape
Michael Geddes
frog at bunyip.wheelycreek.net
Fri Aug 9 17:28:41 HKT 2024
It occurred to me that we need to restrict the names of commands we can
register. The minimum would be:
* No whitespace
* Cannot start with a '-' as this means an option
* Probably not starting with a number (which could be an option)
It also occurs to me that we have some issues, at the moment, with utf-8
characters so I think we should probably stick to ascii.
The rules I have now are very English-centric:
* Must start with an alpha character (a-z or A-Z)
* Can contain only a-z, A-Z, 0-9, -, _
* Must end with a-z, A-Z, 0-9
Thoughts? Is this ok for the moment?
I'm also not going to mention being able to miss out the 'usr' and have it
get placed there.. thinking of that as a hidden
'do what I meant' situation. If the parent parameter is '' it will register
it under 'usr'
//.ichael
On Tue, 6 Aug 2024 at 18:15, <frog at bunyip.wheelycreek.net> wrote:
> 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>
> 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> 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
>
> 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
> http://lists.openvehicles.com/mailman/listinfo/ovmsdev
>
>
>
> _______________________________________________
>
> OvmsDev mailing list
>
> 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/20240809/e4e7f1e4/attachment-0001.htm>
More information about the OvmsDev
mailing list