Return value from execstring(), or construct Options for a builtin?

Return value from execstring(), or construct Options for a builtin?

[Sebastian Gniazdowski]

Hello
I'm finishing the unification of db/* modules, to make them use ztie / zuntie. I call backend function via:

backend_cmd = "zrtie"; // (or "zgtie")
execstring(backend_cmd, 1, 0, "ztie");

The problem is execstring() is void return type. Does it store retval somewhere?

I can also call builtin, have builtintab correctly searched, proper HashNode is obtained. But not sure how to fill Options argument. I have no options to pass:

bin_zrtie(char *nam, char **args, Options ops, UNUSED(int func));

--
Sebastian Gniazdowski
psprint /at/ zdharma.org

Re: Return value from execstring(), or construct Options for a builtin?

[Bart Schaefer]

On Jun 7, 12:20pm, Sebastian Gniazdowski wrote:
}
} backend_cmd = "zrtie"; // (or "zgtie")
} execstring(backend_cmd, 1, 0, "ztie");

I believe this is very much the wrong way to do this (even though I
suggested it as a solution for a different specific problem).

The right thing would be to handle this the way that ZLE entry points
are managed (see Src/init.c:zleentry), with one module to contain ztie
and related functions which makes an entry point call back to the chosen
backend module.

The decisions about how to abstract bin_ztie et al. from gdbm should be
made by discussion with this group, not by you racing ahead on your own.
Proposed approaches are of course welcome (see for example the discussion
about numeric sorting).

} The problem is execstring() is void return type. Does it store retval
} somewhere?

It's in the global variable "lastval".

} I can also call builtin, have builtintab correctly searched, proper
} HashNode is obtained. But not sure how to fill Options argument.

This is handled by Src/builtin.c:execbuiltin, Options objects are
allocated by the static function new_optarg.  But you should not be
in the position of needing to care about this, you should never be
calling bin_* functions directly.

Re: Return value from execstring(), or construct Options for a builtin?

[Sebastian Gniazdowski]

On 7 czerwca 2017 at 18:11:30, Bart Schaefer (schaefer@brasslantern.com) wrote:
> On Jun 7, 12:20pm, Sebastian Gniazdowski wrote:
> }
> } backend_cmd = "zrtie"; // (or "zgtie")
> } execstring(backend_cmd, 1, 0, "ztie");
>  
> I believe this is very much the wrong way to do this (even though I
> suggested it as a solution for a different specific problem).

I tried the parameter-way, to pass to backend via ZSH_DATABASE_SPEC. No problem with builtintab searching, it's fixed.

> The right thing would be to handle this the way that ZLE entry points
> are managed (see Src/init.c:zleentry), with one module to contain ztie
> and related functions which makes an entry point call back to the chosen
> backend module.

I depend on coffee, which has ended for today, worked for 6 hours on the zleentry-way, I think I got it. I declare the following in db.c:

/* For casts, as below mod_export variables are void-no-arguments */
typedef int (*DbBackendEntryPoint)(VA_ALIST1(int cmd));

/**/
mod_export void (*backend_gdbm_entry_ptr)();

/**/
mod_export void (*backend_redis_entry_ptr)();

/**/
mod_export void (*backend_custom1_entry_ptr)();

...

There are three custom "db/custom[0-9]" backends. This way a custom module can attach, or also, it can fully replace db.c and provide own above variables in its custom db.c. In backend I include db.epro, to have the declarations of above vars, but backend module can just define own externs. This is to not depend on zsh.h. For this I also used macros not enum, for command ids.

> The decisions about how to abstract bin_ztie et al. from gdbm should be
> made by discussion with this group, not by you racing ahead on your own.
> Proposed approaches are of course welcome (see for example the discussion
> about numeric sorting).

This goes on me, coffee+interesting topic+coding. Hope current method is OK, if not I can change.

I've commited zredis plugin using two modules:

https://github.com/zdharma/zredis/blob/master/module/Src/zdharma/db.c
https://github.com/zdharma/zredis/blob/master/module/Src/zdharma/zredis.c

From the last one, on-topic are functions redis_main_entry, zrtie_cmd and zruntie_cmd.

--
Sebastian Gniazdowski
psprint /at/ zdharma.org

Re: Return value from execstring(), or construct Options for a builtin?

[Bart Schaefer]

On Jun 8, 12:04pm, Sebastian Gniazdowski wrote:
} Subject: Re: Return value from execstring(), or construct Options for a bu
}
} I think I got it. I declare the following in db.c:
} 
} /* For casts, as below mod_export variables are void-no-arguments */
} typedef int (*DbBackendEntryPoint)(VA_ALIST1(int cmd));
} 
} ...
} 
} There are three custom "db/custom[0-9]" backends.

Large step in the right direction, good.

I would suggest creating a hash table object in db.c whose keys are the
strings passed to "ztie -d" and whose values are structs containing a
DbBackendEntryPoint pointer.  Then provide callable routines that
can be invoked from the db module setup_ / cleanup_ functions to
add/remove an entry point.

By making it a struct we can later expand to more than one entry point
per module if that turns out to be useful.

Re: Return value from execstring(), or construct Options for a builtin?

[Bart Schaefer]

On Jun 8,  4:14pm, Bart Schaefer wrote:
}
} I would suggest creating a hash table object in db.c whose keys are the
} strings passed to "ztie -d"

Forgot to mention that this also provides an obvious hook for getting a
list of which databases are available, in case a script is interested.

Re: Return value from execstring(), or construct Options for a builtin?

[Sebastian Gniazdowski]

On 9 czerwca 2017 at 01:14:49, Bart Schaefer (schaefer@brasslantern.com) wrote:
> I would suggest creating a hash table object in db.c whose keys are the
> strings passed to "ztie -d" and whose values are structs containing a
> DbBackendEntryPoint pointer. Then provide callable routines that
> can be invoked from the db module setup_ / cleanup_ functions to
> add/remove an entry point.

Done it. I was following curses.c because it has hash table for colorpairs. Now the arrays {zgdbm,zredis}_tied aren't searched to get backend type of given tied parameter. Instead, DB_IS_TIED is invoked on each backend, via hash table scanning. Tests are passing (like yesterday). The diff is quite nice to look at, because it replaces multiple ifs with gethashnode2, etc.:

https://github.com/zdharma/zredis/commit/8107b50362acb169ce9ad9dc6ce418ccdc96fc4f

> Forgot to mention that this also provides an obvious hook for getting a
> list of which databases are available, in case a script is interested.

It will also allow user to assure he loaded the modules correctly. Presence in `zmodload` output can be misleading in case of doing loads and unloads without care.

--
Sebastian Gniazdowski
psprint /at/ zdharma.org