[PATCH 0/2] bindings/swupdate_lua: Progress Message Ease Of Use

[Konrad Schwarz]

From: Konrad Schwarz <konrad....@siemens.com>

The progress message "status" and "source" fields
use the numeric enum codes of the C implementation.
Script writers might prefer using symbolic labels (strings) instead.

The first patch adds new fields "status_label" and "source_label"
with those strings. The fields "status" and "source"
remain as they are, for backwards compatibility.

In an effort to minimize the runtime and garbage
collection costs, the patch hoists as much Lua string
processing as possible into the luaopen_ initialization routine
and refers to those strings (indirectly) via the Lua registry
at run time. This may be overkill.

The second patch simplifies lua_CFunction return argument
handling by utilizing more of Lua's built-in features.
This could be a systematic issue in SWUpdate's use of the Lua API,
but the patch only addresses bindings/swupdate_lua.

Konrad Schwarz (2):
bindings/lua_swupdate: add symbolic fields to progress messages
binding/lua_swupdate: Lua return argument simplification

bindings/lua_swupdate.c | 444 +++++++++++++++++++++++++++++++-------
bindings/lua_swupdate.lua | 10 +-
2 files changed, 375 insertions(+), 79 deletions(-)

--
2.47.3

[Konrad Schwarz]

From: Konrad Schwarz <konrad....@siemens.com>

Progress messages delivered via lua_swupdate.progress contain table fields
"status" and "source", of types "lua_swupdate.RECOVERY_STATUS" resp.
"lua_swupdate.sourcetype".

Somewhat unfortunately, these fields are numeric; the tables
"lua_swupdate.RECOVERY_STATUS" and "lua_swupdate.sourcetype" map from
string labels to these numeric values. It is a bit of a chore to
display the values symbolically as human-readable strings that are
useful for simple monitoring and debugging: The inverse mapping of
"lua_swupdate.RECOVERY_STATUS" and "lua_swupdate.sourcetype" must be
generated and can then be indexed by the field values.

Although these inverse mappings are not difficult to create in Lua,
as a convenience, this patch makes the symbols (strings) available as
the new fields "status_label" and "source_label" in progress messages.

The patch itself takes great pains to hoist as much string
processing out of the receive loop as possible: the labels of
"lua_swupdate.RECOVERY_STATUS" and "lua_swupdate.sourcetype", as well
as the field names are recorded in Lua exactly once, in the package's
luaopen_ routine. To prevent the strings from being garbage collected,
they (or tables holding them) are registered as light userdata in the
Lua registry. No so such string is registered twice; the intent is that
string comparision can be done quickly as pointer identity checks.

Note that the (undocumented) internment of short strings apparently
implemented in at least some Lua versions provides most of the same
benefits -- except that an extra hash table lookup is required to check
if a short string has been interned already each time such a string is
passed to Lua. The purely functional improvement could be implemented
in simpler fashion.

Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
---
bindings/lua_swupdate.c | 418 ++++++++++++++++++++++++++++++++------
bindings/lua_swupdate.lua | 10 +-
2 files changed, 368 insertions(+), 60 deletions(-)

diff --git a/bindings/lua_swupdate.c b/bindings/lua_swupdate.c
index bd5f876a..e81f4030 100644
--- a/bindings/lua_swupdate.c
+++ b/bindings/lua_swupdate.c
@@ -2,15 +2,15 @@
* (C) Copyright 2018
* Stefano Babic, stefan...@swupdate.org.
*
- * SPDX-License-Identifier: LGPL-2.1-or-later
+ * SPDX-License-Identifier: LGPL-2.1-or-later
*/

#include <progress_ipc.h>
#include <lua.h>
#include <lauxlib.h>
-#include <string.h>
+#include <string.h>
#include <ifaddrs.h>
-#include <netinet/in.h>
+#include <netinet/in.h>
#include <net/if.h>
#include <arpa/inet.h>
#include <unistd.h>
@@ -25,27 +25,66 @@
#include "auxiliar.h"
#include "lua_compat.h"

+/* Comment Legend:
+ * Lua uses an argument stack for object manipulation.
+ * Routines that manipulate the stack in non-trival ways
+ * use the following notation to indicate the current stack layout,
+ * derived from FORTH stack notation:
+ */
+ /* stack_bottom ... stack_top */
+/*
+ * I.e., the rightmost element is the stack top.
+ * A stack entry is identified
+ * with a code that is supposed to be mnemonic,
+ * typically utilizing an abbreviation of its type.
+ * E.g., "t_a" for "table A".
+ *
+ * For referencce: LUa defines stack offsets as follows:
+ *
+ * 0 1 2 ... -2 -1
+ *
+ */
+
+
+/* always returning the type helps assertion checking */
+# if 503 > LUA_VERSION_NUM
+# define PR_LUA_RAWGET(L, index)\
+ (lua_rawget (L, index), lua_type (L, -1))
+# define PR_LUA_RAWGETI(L, index, integer)\
+ (lua_rawgeti (L, index, integer), lua_type (L, -1))
+# else
+# define PR_LUA_RAWGET(L, index)\
+ lua_rawget (L, index)
+# define PR_LUA_RAWGETI(L, index, integer)\
+ lua_rawgeti (L, index, integer)
+# endif
+
+
+/* work around the non-const-qualification of argument p */
+# define PR_LUA_PUSHLIGHTUSERDATA(L, P)\
+ lua_pushlightuserdata (L, (void *) P)
+
#define WAIT 1

-#define LUA_PUSH_STRING(key, data) do { \
+#define LUA_PUSH_STRING(key, data) do { \
lua_pushstring(L, key); \
lua_pushstring(L, data); \
lua_settable(L, -3); \
} while (0)

-#define LUA_PUSH_BOOL(key, data) do { \
+#define LUA_PUSH_BOOL(key, data) do { \
lua_pushstring(L, key); \
lua_pushboolean(L, data); \
lua_settable(L, -3); \
} while (0)

-#define LUA_PUSH_NUMBER(key, data) do { \
+#define LUA_PUSH_NUMBER(key, data) do { \
lua_pushstring(L, key); \
lua_pushnumber(L, (double) data); \
lua_settable(L, -3); \
} while (0)

-#define LUA_PUSH_INT(key, data) do { \
+#define LUA_PUSH_INT(key, data) do { \
lua_pushstring(L, key); \
lua_pushinteger(L, data); \
lua_settable(L, -3); \
@@ -62,8 +101,6 @@ static int ctrl_write(lua_State *L);
static int ctrl_close(lua_State *L);
static int ctrl_close_socket(lua_State *L);

-int luaopen_lua_swupdate(lua_State *L);
-
/*
* Return a table with all interface and their IP address
*/
@@ -118,7 +155,7 @@ static luaL_Reg ctrl_methods[] = {
{"connect", ctrl_connect},
{"write", ctrl_write},
{"close", ctrl_close},
- {NULL, NULL}
+ {}
};

/**
@@ -271,12 +308,12 @@ struct prog_obj {

/* progress object methods */
static luaL_Reg progress_methods[] = {
- {"__gc", progress_close},
- {"__tostring", auxiliar_tostring},
- {"close", progress_close},
- {"connect", progress_connect},
- {"receive", progress_receive},
- {NULL, NULL}
+ {"__gc", progress_close},
+ {"__tostring", auxiliar_tostring},
+ {"close", progress_close},
+ {"connect", progress_connect},
+ {"receive", progress_receive},
+ {}
};

/**
@@ -284,7 +321,7 @@ static luaL_Reg progress_methods[] = {
*
* @param [Lua] The swupdate_progress class instance.
@return [Lua] The connection handle (mostly for information), or,
- * in case of errors, nil plus an error message.
+ * in case of errors, nil plus an error message.
*/
static int progress_connect(lua_State *L) {
struct prog_obj *p = (struct prog_obj *) auxiliar_checkclass(L, "swupdate_progress", 1);
@@ -301,37 +338,207 @@ static int progress_connect(lua_State *L) {
p->socket = connfd;
p->status = IDLE;

- lua_pop(L, 1);
lua_pushnumber(L, connfd);
- lua_pushnil(L);

- return 2;
+ return 1;
}

static int progress_close(lua_State __attribute__ ((__unused__)) *L) {
return 1;
}

-static int progress_receive(lua_State *L) {
+
+# define ALL_PROGRESS_LABELS(MACRO)\
+ MACRO (apiversion)\
+ MACRO (status)\
+ MACRO (download)\
+ MACRO (source)\
+ MACRO (nsteps)\
+ MACRO (step)\
+ MACRO (percent)\
+ MACRO (artifact)\
+ MACRO (handler)\
+ MACRO (info)\
+ MACRO (status_label)\
+ MACRO (source_label)\
+
+static struct progress_field {
+ char const *name;
+ size_t len;
+} const progress_field [] = {
+
+# define PROGRESS_FIELD_STRING(LABEL)\
+ { #LABEL, sizeof #LABEL - sizeof "" },
+
+ ALL_PROGRESS_LABELS (PROGRESS_FIELD_STRING)
+
+# undef PROGRESS_FIELD_STRING
+};
+
+enum progress_field_enum {
+
+# define PROGRESS_FIELD_ENUM(LABEL)\
+ progress_field_ ## LABEL,
+
+ ALL_PROGRESS_LABELS (PROGRESS_FIELD_ENUM)
+
+# undef PROGRESS_FIELD_ENUM
+
+ progress_field_enum_last
+
+};
+
+/**
+ * @brief Retrieve the label of a mesasge field and place it at top of stack
+ *
+ * @param<in> field_key the message field's light user data
+ */
+static void progress_get_label (lua_State *const L,
+ struct progress_field const *const field_key)
+{
+ int type;
+
+ /* */
+ PR_LUA_PUSHLIGHTUSERDATA (L, field_key);
+ /* luf */
+ if (LUA_TSTRING != (type = PR_LUA_RAWGET (L, LUA_REGISTRYINDEX)))
+ luaL_error (L, "Expecting a string, got a %s",
+ lua_typename (L, type));
+ /* fs */
+}
+
+static void progress_set_integer (lua_State *const L,
+ struct progress_field const *const field_key,
+ lua_Integer const value)
+{
+ /* pt */
+ progress_get_label (L, field_key);
+ /* pt fs */
+ lua_pushinteger (L, value);
+ /* pt fs i */
+ lua_rawset (L, -3);
+ /* pt */
+}
+
+static void progress_set_string (lua_State *const L,
+ struct progress_field const *const field_key,
+ char const *const value)
+{
+ /* pt */
+ progress_get_label (L, field_key);
+ /* pt fs */
+ lua_pushstring (L, value);
+ /* pt fs s */
+ lua_rawset (L, -3);
+ /* pt */
+}
+
+static void progress_set_lstring (lua_State *const L,
+ struct progress_field const *const field_key,
+ char const *const value,
+ size_t const len)
+{
+ /* pt */
+ progress_get_label (L, field_key);
+ /* pt fs */
+ lua_pushlstring (L, value, len);
+ /* pt fs s */
+ lua_rawset (L, -3);
+ /* pt */
+}
+
+static void progress_set_label (lua_State *const L,
+ struct progress_field const *const field_key,
+ int const *const lu_anchor,
+ lua_Integer const label)
+{
+ int type;
+ /* pt */
+ progress_get_label (L, field_key);
+ /* pt fs */
+ PR_LUA_PUSHLIGHTUSERDATA (L, lu_anchor);
+ /* pt fs lu */
+ if (LUA_TTABLE != (type = PR_LUA_RAWGET (L, LUA_REGISTRYINDEX)))
+ luaL_error (L, "Expecting a table, got a %s",
+ lua_typename (L, type));
+ /* pt fs t */
+ if (LUA_TSTRING != (type = PR_LUA_RAWGETI (L, -1, label)))
+ luaL_error (L, "Expecting a string, got a %s",
+ lua_typename (L, type));
+ /* pt fs t s */
+ lua_remove (L, -2);
+ /* pt fs s */
+ lua_rawset (L, -3);
+ /* pt */
+}
+
+/* light user data anchors -- otherwise unused.
+ * note that (the address of) string constants cannot be used to anchor
+ * light user data, as the linker is allowed to overlay these constants with
+ * others to optimize space */
+static int const recovery_table, source_type_table;
+
+static int progress_receive(lua_State *const L)
+{
struct prog_obj *p = (struct prog_obj *) auxiliar_checkclass(L, "swupdate_progress", 1);
int connfd = p->socket;
if (progress_ipc_receive(&connfd, &p->msg) <= 0) {
- lua_pushnil(L);
- return 2;
+ lua_pushnil(L);
+ return 1;
};
lua_newtable(L);

- LUA_PUSH_INT("apiversion", p->msg.apiversion);
- LUA_PUSH_INT("status", p->msg.status);
- LUA_PUSH_INT("download", p->msg.dwl_percent);
- LUA_PUSH_INT("source", p->msg.source);
- LUA_PUSH_INT("nsteps", p->msg.nsteps);
- LUA_PUSH_INT("step", p->msg.cur_step);
- LUA_PUSH_INT("percent", p->msg.cur_percent);
- LUA_PUSH_STRING("artifact", p->msg.cur_image);
- LUA_PUSH_STRING("handler", p->msg.hnd_name);
+ /* pt */
+
+# define PROGRESS_SET_INTEGER(FIELD, MSG_FIELD)\
+ do {\
+ progress_set_integer (L,\
+ progress_field + progress_field_ ## FIELD,\
+ p->msg.MSG_FIELD);\
+ } while (0)
+
+# define PROGRESS_SET_STRING(FIELD, MSG_FIELD)\
+ do {\
+ progress_set_string (L,\
+ progress_field + progress_field_ ## FIELD,\
+ p->msg.MSG_FIELD);\
+ } while (0)
+
+# define PROGRESS_SET_LSTRING(FIELD, MSG_FIELD, LEN)\
+ do {\
+ progress_set_lstring (L,\
+ progress_field + progress_field_ ## FIELD,\
+ p->msg.MSG_FIELD, LEN);\
+ } while (0)
+
+# define PROGRESS_SET_LABEL(FIELD, LABEL_TABLE, MSG_FIELD)\
+ do {\
+ progress_set_label (L,\
+ progress_field + progress_field_ ## FIELD ## _label,\
+ LABEL_TABLE,\
+ p->msg.MSG_FIELD);\
+ } while (0)
+
+ PROGRESS_SET_INTEGER (apiversion, apiversion);
+ PROGRESS_SET_INTEGER (status, status);
+ PROGRESS_SET_INTEGER (download, dwl_percent);
+ PROGRESS_SET_INTEGER (source, source);
+ PROGRESS_SET_INTEGER (nsteps, nsteps);
+ PROGRESS_SET_INTEGER (step, cur_step);
+ PROGRESS_SET_INTEGER (percent, cur_percent);
+ PROGRESS_SET_STRING (artifact, cur_image);
+ PROGRESS_SET_STRING (handler, hnd_name);
if (p->msg.infolen)
- LUA_PUSH_STRING("info", p->msg.info);
+ PROGRESS_SET_LSTRING (info, info, p->msg.infolen);
+
+ PROGRESS_SET_LABEL (status, &recovery_table, status);
+ PROGRESS_SET_LABEL (source, &source_type_table, source);
+
+# undef PROGRESS_SET_LABEL
+# undef PROGRESS_SET_STRING
+# undef PROGRESS_SET_INTEGER
+
+ /* pt */

p->status = p->msg.status;

@@ -354,39 +561,136 @@ static const luaL_Reg lua_swupdate[] = {
{"progress", progress},
{"control", ctrl},
{"ipv4", netif},
- {NULL, NULL}
+ {}
};

/*
* Initialization of C module
*/
-int luaopen_lua_swupdate(lua_State *L){
- luaL_newlib(L, lua_swupdate);

- /* Export the RECOVERY_STATUS enum */
- lua_pushstring(L, "RECOVERY_STATUS");
+struct label_init {
+ int cardinal;
+ char const *label;
+ size_t label_len;
+};
+
+/**
+ * @brief Initialize label mapping table and its inverse.
+ *
+ * The lua module table must be at the top of the Lua stack.
+ * On return, the stack has the same form.
+ *
+ * @param<in> table_name: the table's name
+ * @param<in> table_name_len: the table name's length
+ * @param<in> anchor: a unique address used to anchor the inverse table as Lua lightuserdata
+ * @param<in> li: the start of the list of labels to be stored in the table and its inverse
+ * @param<in> li_end: the end of the list of labels to be stored in the table and its inverse
+ */
+static void process_labels (lua_State *const L,
+ char const *const table_name,
+ size_t const table_name_len,
+ int const *const anchor,
+ struct label_init const *li,
+ struct label_init const *const li_end)
+{
+ /* mt */
+# if 0
+ char const *const table_lua =
+# endif
+ lua_pushlstring (L, table_name, table_name_len);
+ /* mt tn */
lua_newtable (L);
- LUA_PUSH_INT("IDLE", IDLE);
- LUA_PUSH_INT("START", START);
- LUA_PUSH_INT("RUN", RUN);
- LUA_PUSH_INT("SUCCESS", SUCCESS);
- LUA_PUSH_INT("FAILURE", FAILURE);
- LUA_PUSH_INT("DOWNLOAD", DOWNLOAD);
- LUA_PUSH_INT("DONE", DONE);
- LUA_PUSH_INT("SUBPROCESS", SUBPROCESS);
- LUA_PUSH_INT("PROGRESS", PROGRESS);
- lua_settable(L, -3);
+ /* mt tn t */
+ lua_newtable (L);
+ /* mt tn t t_inv */
+ PR_LUA_PUSHLIGHTUSERDATA (L, anchor);
+ /* mt tn t t_inv lu_a */
+ lua_pushvalue (L, -2);
+ /* mt tn t t_inv lu_a_inv t_inv */
+ lua_rawset (L, LUA_REGISTRYINDEX);
+ /* mt tn t t_inv */
+
+ for (; li_end > li; ++li) {
+ /* mt tn t t_inv */
+# if 0
+ char const *const label_lua =
+# endif
+ lua_pushlstring (L, li->label, li->label_len);
+ /* mt tn t t_inv s */
+ lua_pushinteger (L, li->cardinal);
+ /* mt tn t t_inv s i */
+ lua_pushvalue (L, -2);
+ /* mt tn t t_inv s i s */
+ lua_rawseti (L, -4, li->cardinal);
+ /* mt tn t t_inv s i */
+ lua_rawset (L, -4);
+ /* mt tn t t_inv */
+ }
+ /* mt tn t t_inv */
+ lua_pop (L, 1);
+ /* mt tn t */
+ lua_rawset (L, -3);
+ /* mt */
+}
+
+# pragma GCC diagnostic ignored "-Wmissing-prototypes"
+int
+luaopen_lua_swupdate(lua_State *L)
+{
+ luaL_newlib(L, lua_swupdate);
+ /* mt */
+
+# define LABEL_INIT(X)\
+ { X, #X, sizeof #X - sizeof "" },

+ /* Export the RECOVERY_STATUS enum */
+ static struct label_init const recovery_labels [] = {
+ LABEL_INIT (IDLE)
+ LABEL_INIT (START)
+ LABEL_INIT (RUN)
+ LABEL_INIT (SUCCESS)
+ LABEL_INIT (FAILURE)
+ LABEL_INIT (DOWNLOAD)
+ LABEL_INIT (DONE)
+ LABEL_INIT (SUBPROCESS)
+ LABEL_INIT (PROGRESS)
+ };
+ static char const recovery_status [] = "RECOVERY_STATUS";
+ process_labels (L, recovery_status, sizeof recovery_status - sizeof "",
+ &recovery_table,
+ recovery_labels, 1 [&recovery_labels]);
+ /* mt */
/* Export the sourcetype enum */
- lua_pushstring(L, "sourcetype");
- lua_newtable (L);
- LUA_PUSH_INT("SOURCE_UNKNOWN", SOURCE_UNKNOWN);
- LUA_PUSH_INT("SOURCE_WEBSERVER", SOURCE_WEBSERVER);
- LUA_PUSH_INT("SOURCE_SURICATTA", SOURCE_SURICATTA);
- LUA_PUSH_INT("SOURCE_DOWNLOADER", SOURCE_DOWNLOADER);
- LUA_PUSH_INT("SOURCE_LOCAL", SOURCE_LOCAL);
- LUA_PUSH_INT("SOURCE_CHUNKS_DOWNLOADER", SOURCE_CHUNKS_DOWNLOADER);
- lua_settable(L, -3);
+ static struct label_init const source_labels [] = {
+
+# define SOURCE_LABEL_INIT(S)\
+ LABEL_INIT (SOURCE_ ## S)
+
+ SOURCE_LABEL_INIT (UNKNOWN)
+ SOURCE_LABEL_INIT (WEBSERVER)
+ SOURCE_LABEL_INIT (SURICATTA)
+ SOURCE_LABEL_INIT (DOWNLOADER)
+ SOURCE_LABEL_INIT (LOCAL)
+ SOURCE_LABEL_INIT (CHUNKS_DOWNLOADER)
+ };
+ static char const source_type [] = "sourcetype";
+ process_labels (L, source_type, sizeof source_type - sizeof "",
+ &source_type_table,
+ source_labels, 1 [&source_labels]);
+ /* mt */
+
+ struct progress_field const *f;
+ for (f = progress_field; 1 [&progress_field] > f; ++f) {
+ /* mt */
+ PR_LUA_PUSHLIGHTUSERDATA (L, f);
+ /* mt luf */
+ lua_pushlstring (L, f->name, f->len);
+ /* mt luf s */
+ lua_rawset (L, LUA_REGISTRYINDEX);
+ /* mt */
+ }
+
+ /* mt */

auxiliar_newclass(L, "swupdate_progress", progress_methods);
auxiliar_newclass(L, "swupdate_control", ctrl_methods);
diff --git a/bindings/lua_swupdate.lua b/bindings/lua_swupdate.lua
index b21294b5..f3470189 100644
--- a/bindings/lua_swupdate.lua
+++ b/bindings/lua_swupdate.lua
@@ -43,7 +43,7 @@ lua_swupdate.RECOVERY_STATUS = {
DOWNLOAD = 5,
DONE = 6,
SUBPROCESS = 7,
- PROGRESS = 8
+ PROGRESS = 8,
}


@@ -55,7 +55,7 @@ lua_swupdate.sourcetype = {
SOURCE_SURICATTA = 2,
SOURCE_DOWNLOADER = 3,
SOURCE_LOCAL = 4,
- SOURCE_CHUNKS_DOWNLOADER = 5
+ SOURCE_CHUNKS_DOWNLOADER = 5,
}


@@ -70,7 +70,11 @@ lua_swupdate.sourcetype = {
--- @field percent number Percentage done in current step
--- @field artifact string Name of image to be installed
--- @field handler string Name of running Handler
---- @field info string Additional information about installation
+--- @field info string Additional information about installation, optional
+---
+--- @field status_label string The (string) label corresponding to field status
+--- @field source_label string The (string) label corresponding to field source
+


--- SWUpdate progress socket binding.
--
2.47.3

[Konrad Schwarz]

From: Konrad Schwarz <konrad....@siemens.com>

The existing code takes care to clear the Lua stack of any incoming
arguments before pushing any return values and to push the exact same
number of arguments on each (success or failure) return path.

However, Lua does not require this: as stated in the manual, the
interpreter will use the final n stack entries of a lua_CFunction as
its return arguments, automatically discarding any stack entries below
them, where n is the integer return value of the lua_CFunction. It will
_adjust_ the number of values in an assignment statement or function call
expression to what is needed by dropping extra values resp. padding the
list with additional nil values.

Other Lua interface files in SWUpdate may be using the same approach.

Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
---

bindings/lua_swupdate.c | 26 +++++++-------------------
1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/bindings/lua_swupdate.c b/bindings/lua_swupdate.c
index e81f4030..2101cea9 100644
--- a/bindings/lua_swupdate.c
+++ b/bindings/lua_swupdate.c
@@ -163,13 +163,12 @@ static luaL_Reg ctrl_methods[] = {
*
* @param [Lua] The swupdate_control class instance.
* @return [Lua] The connection handle (mostly for information), or,

- * in case of errors, nil plus an error message.
+ * in case of errors, nil plus an error message.
*/

static int ctrl_connect(lua_State *L) {
struct ctrl_obj *p = (struct ctrl_obj *) auxiliar_checkclass(L, "swupdate_control", 1);
struct swupdate_request req;
if (p->socket != -1) {
- lua_pop(L, 1);
lua_pushnil(L);
lua_pushstring(L, "Already connected to SWUpdate control socket.");
return 2;
@@ -179,7 +178,6 @@ static int ctrl_connect(lua_State *L) {
req.source = SOURCE_LOCAL;
int connfd = ipc_inst_start_ext(&req, sizeof(req));
if (connfd < 0) {
- lua_pop(L, 1);
lua_pushnil(L);
lua_pushstring(L, "Cannot connect to SWUpdate control socket.");
return 2;
@@ -187,11 +185,9 @@ static int ctrl_connect(lua_State *L) {

p->socket = connfd;

- lua_pop(L, 1);
lua_pushnumber(L, connfd);
- lua_pushnil(L);

- return 2;
+ return 1;
}

/**
@@ -208,7 +204,7 @@ static int ctrl_write(lua_State *L) {
if (p->socket == -1) {
lua_pushnil(L);
lua_pushstring(L, "Not connected to SWUpdate control socket.");
- goto ctrl_write_exit;
+ return 2;
}

size_t len = 0;
@@ -216,21 +212,16 @@ static int ctrl_write(lua_State *L) {
if (!buf) {
lua_pushnil(L);
lua_pushstring(L, "Error converting Lua chunk data.");
- goto ctrl_write_exit;
+ return 2;
}
if (ipc_send_data(p->socket, (char *)buf, len) < 0) {
lua_pushnil(L);
lua_pushstring(L, "Error writing to SWUpdate control socket.");
- goto ctrl_write_exit;
+ return 2;
}

lua_pushboolean(L, true);
- lua_pushnil(L);
-
-ctrl_write_exit:
- lua_remove(L, 1);
- lua_remove(L, 1);

- return 2;
+ return 1;
}

static int ctrl_close_socket(lua_State *L) {
@@ -260,7 +251,6 @@ static int ipc_wait_get_msg(ipc_message *msg)
static int ctrl_close(lua_State *L) {
struct ctrl_obj *p = (struct ctrl_obj *) auxiliar_checkclass(L, "swupdate_control", 1);
if (p->socket == -1) {
- lua_pop(L, 1);
lua_pushboolean(L, true);
lua_pushnil(L);
return 2;
@@ -285,8 +275,7 @@ static int ctrl_close(lua_State *L) {
}

lua_pushboolean(L, true);

- lua_pushnil(L);
- return 2;
+ return 1;
}

static int ctrl(lua_State *L) {
@@ -330,7 +319,6 @@ static int progress_connect(lua_State *L) {
close(p->socket);
connfd = progress_ipc_connect(WAIT);
if (connfd < 0) {
- lua_pop(L, 1);
lua_pushnil(L);
lua_pushstring(L, "Cannot connect to SWUpdate progress socket.");
return 2;
--
2.47.3

[Stefano Babic]

Hi Konrad,

On 4/14/26 20:58, Konrad Schwarz wrote:
> From: Konrad Schwarz <konrad....@siemens.com>
>
> The existing code takes care to clear the Lua stack of any incoming
> arguments before pushing any return values and to push the exact same
> number of arguments on each (success or failure) return path.
>
> However, Lua does not require this: as stated in the manual,

This is interesting, and I have always assumed that it is required. Can
you point me where is this in the manual ? Thanks !

But this is unrelated to the commit message - function (see prototype)
returns the connection or in case of error (like here) two parms, nil +
error string. You are changing the API here, or not ?

Best regards,
Stefano

_______________________________________________________________________
Nabla Software Engineering GmbH
Hirschstr. 111A | 86156 Augsburg | Tel: +49 821 45592596
Geschäftsführer : Stefano Babic | HRB 40522 Augsburg
E-Mail: sba...@nabladev.com

[konrad....@gmail.com]

Hello Stefano,

> > The existing code takes care to clear the Lua stack of any incoming
> > arguments before pushing any return values and to push the exact same
> > number of arguments on each (success or failure) return path.
> >
> > However, Lua does not require this: as stated in the manual,
>
> This is interesting, and I have always assumed that it is required. Can you point me
> where is this in the manual ? Thanks !
>
> > the
> > interpreter will use the final n stack entries of a lua_CFunction as
> > its return arguments, automatically discarding any stack entries below
> > them, where n is the integer return value of the lua_CFunction. It
> > will _adjust_ the number of values in an assignment statement or
> > function call expression to what is needed by dropping extra values
> > resp. padding the list with additional nil values.

E.g., for Lua 5.4, see:

* lua_CFunction (https://www.lua.org/manual/5.4/manual.html#lua_CFunction)
* 3.3.3 Assignment (https://www.lua.org/manual/5.4/manual.html#3.3.3)
* 3.4.10 Function Calls (https://www.lua.org/manual/5.4/manual.html#3.4.10)
* 3.4.11 Function Definitions (https://www.lua.org/manual/5.4/manual.html#3.4.11)
* 3.4.12 Lists of Expressions, Multiple Results, and Adjustment (https://www.lua.org/manual/5.4/manual.html#3.4.12)

Were Lua to require a fixed number of return variables per CFunction,
it would have been possible to treat that number as a constant and record that number
in, e.g., (an extended) luaL_Reg (the list of functions used by luaL_setfuncs)
rather than requiring the CFunction to generate that constant on each call --
freeing up the C-functions return value for some other use, or to declare them with
return type void.

> > @@ -187,11 +185,9 @@ static int ctrl_connect(lua_State *L) {
> >
> > p->socket = connfd;
> >
> > - lua_pop(L, 1);
> > lua_pushnumber(L, connfd);
> > - lua_pushnil(L);
> >
> > - return 2;
> > + return 1;
>
> But this is unrelated to the commit message - function (see prototype) returns the
> connection or in case of error (like here) two parms, nil + error string. You are
> changing the API here, or not ?

The first deleted line removes the input argument, which Lua does
automatically: all except the top-most n arguments are discarded.

The second deletion eliminates the second return value.
We now only return 1 value, hence the final change.

In assignment statements, Lua performs _adjustment_,
so, e.g., if an assignment statement has two variables on the left side,
and the CFunction has returned only one value,
Lua will pad the list on the right side with a nil, so we end up
with no change in the API.

In function call statements, the same situation arises:
if too few arguments are passed, the remaining parameters
are set to nil. Again, there is no change to the API.
(The call to our CFunction must be the final parameter
for this to be even relevant,
function calls in other positions insert only their first
return value in Lua).

Consider Lua's assert(v [, message]) basic function.
Its semantics are defined exactly for this situation:
if v != nil and v != false, then return v (and don't examine message),
otherwise raise an error with message as the error object.

Returning either one good value or false/nil followed by one or more error messages
is the idiomatic way of returning failure status in Lua. See e.g.
6.8 Input and Output Facilities (https://www.lua.org/manual/5.4/manual.html#6.8).

--

Best regards,

Konrad

[Storm, Christian]

Hi,

Have you verified this for *all* Lua versions SWUpdate supports back to 5.1 and including LuaJIT?

Well, in contrast to other languages, Lua is not opinionated – you can, e.g., choose which OOP model you implement (yourself) – and Lua gives you complete control and freedom. So, there may be (more?) value in having symmetry / explicitness in the Lua/C binding code over how it's done "usually". It's not just a technical (optimization) decision but also a stylistic one.


Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, FT RPD CED
Friedrich-Ludwig-Bauer-Str. 3, 85748 Garching, Germany