4:27 p.m.
From: Michal Privoznik <mprivozn(a)redhat.com>
There's a problem with glib: what we might consider functions are
in fact macros and to make things worse - they do declare local
variables. For instance here's the declaration of
g_clear_pointer() macro:
#define g_clear_pointer(pp, destroy) \
G_STMT_START \
{ \
G_STATIC_ASSERT (sizeof *(pp) == sizeof (gpointer)); \
glib_typeof ((pp)) _pp = (pp); \
glib_typeof (*(pp)) _ptr = *_pp; \
*_pp = NULL; \
if (_ptr) \
(destroy) (_ptr); \
} \
G_STMT_END \
Now, as of v6.2.0-rc1~267 our VIR_FREE() macro is in fact a
redeclaration of g_clear_pointer(). Thus, calling VIR_FREE()
increases stack size!
Ideally, this wouldn't be a problem, because those variables
(_pp, _ptr) live in their own block. And clever compiler can just
reuse space created for one block.
But then there's clang where we are hitting this exact problem in
functions like doRemoteOpen() where either g_clear_pointer() is
called directly, or there are macros like EXTRACT_URI_ARG_STR()
which hide the call away.
That's why despite our previous efforts decreasing stack size we
still needed v9.8.0-rc1~208.
Well, moving URI argument extraction (those calls to
EXTRACT_URI_ARG_* macros) into a separate function helps us
decrease stack size from 2296 bytes to 1320.
Even after this there are still more possibilities for
improvements, but those will be addressed in future commits.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/remote/remote_driver.c | 113 +++++++++++++++++++++++++++----------
1 file changed, 82 insertions(+), 31 deletions(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 745cd34f83..681594e406 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -758,13 +758,74 @@ remoteConnectFormatURI(virURI *uri,
virReportError(VIR_ERR_INVALID_ARG, \
_("Failed to parse value of URI component %1$s"), \
var->name); \
- goto error; \
+ return -1; \
} \
ARG_VAR = tmp == 0; \
var->ignore = 1; \
continue; \
}
+static int
+doRemoteOpenExtractURIArgs(virConnectPtr conn,
+ char **name,
+ char **command,
+ char **sockname,
+ char **authtype,
+ char **sshauth,
+ char **netcat,
+ char **keyfile,
+ char **pkipath,
+ char **knownHosts,
+ char **knownHostsVerify,
+ char **tls_priority,
+ char **mode_str,
+ char **proxy_str,
+#ifndef WIN32
+ bool *tty,
+#endif
+ bool *sanity,
+ bool *verify)
+{
+ size_t i;
+
+ for (i = 0; i < conn->uri->paramsCount; i++) {
+ virURIParam *var = &conn->uri->params[i];
+
+ EXTRACT_URI_ARG_STR("name", *name);
+ EXTRACT_URI_ARG_STR("command", *command);
+ EXTRACT_URI_ARG_STR("socket", *sockname);
+ EXTRACT_URI_ARG_STR("auth", *authtype);
+ EXTRACT_URI_ARG_STR("sshauth", *sshauth);
+ EXTRACT_URI_ARG_STR("netcat", *netcat);
+ EXTRACT_URI_ARG_STR("keyfile", *keyfile);
+ EXTRACT_URI_ARG_STR("pkipath", *pkipath);
+ EXTRACT_URI_ARG_STR("known_hosts", *knownHosts);
+ EXTRACT_URI_ARG_STR("known_hosts_verify", *knownHostsVerify);
+ EXTRACT_URI_ARG_STR("tls_priority", *tls_priority);
+ EXTRACT_URI_ARG_STR("mode", *mode_str);
+ EXTRACT_URI_ARG_STR("proxy", *proxy_str);
+ EXTRACT_URI_ARG_BOOL("no_sanity", *sanity);
+ EXTRACT_URI_ARG_BOOL("no_verify", *verify);
+#ifndef WIN32
+ EXTRACT_URI_ARG_BOOL("no_tty", *tty);
+#endif
+
+ if (STRCASEEQ(var->name, "authfile")) {
+ /* Strip this param, used by virauth.c */
+ var->ignore = 1;
+ continue;
+ }
+
+ VIR_DEBUG("passing through variable '%s' ('%s') to remote
end",
+ var->name, var->value);
+ }
+
+ return 0;
+}
+
+#undef EXTRACT_URI_ARG_STR
+#undef EXTRACT_URI_ARG_BOOL
+
/*
* URIs that this driver needs to handle:
@@ -818,7 +879,6 @@ doRemoteOpen(virConnectPtr conn,
bool tty = true;
#endif
int mode;
- size_t i;
int proxy;
/* We handle *ALL* URIs here. The caller has rejected any
@@ -844,35 +904,28 @@ doRemoteOpen(virConnectPtr conn,
* although that won't be the case for now).
*/
if (conn->uri) {
- for (i = 0; i < conn->uri->paramsCount; i++) {
- virURIParam *var = &conn->uri->params[i];
- EXTRACT_URI_ARG_STR("name", name);
- EXTRACT_URI_ARG_STR("command", command);
- EXTRACT_URI_ARG_STR("socket", sockname);
- EXTRACT_URI_ARG_STR("auth", authtype);
- EXTRACT_URI_ARG_STR("sshauth", sshauth);
- EXTRACT_URI_ARG_STR("netcat", netcat);
- EXTRACT_URI_ARG_STR("keyfile", keyfile);
- EXTRACT_URI_ARG_STR("pkipath", pkipath);
- EXTRACT_URI_ARG_STR("known_hosts", knownHosts);
- EXTRACT_URI_ARG_STR("known_hosts_verify", knownHostsVerify);
- EXTRACT_URI_ARG_STR("tls_priority", tls_priority);
- EXTRACT_URI_ARG_STR("mode", mode_str);
- EXTRACT_URI_ARG_STR("proxy", proxy_str);
- EXTRACT_URI_ARG_BOOL("no_sanity", sanity);
- EXTRACT_URI_ARG_BOOL("no_verify", verify);
+ /* This really needs to be a separate function to keep
+ * the stack size at sane levels. */
+ if (doRemoteOpenExtractURIArgs(conn,
+ &name,
+ &command,
+ &sockname,
+ &authtype,
+ &sshauth,
+ &netcat,
+ &keyfile,
+ &pkipath,
+ &knownHosts,
+ &knownHostsVerify,
+ &tls_priority,
+ &mode_str,
+ &proxy_str,
#ifndef WIN32
- EXTRACT_URI_ARG_BOOL("no_tty", tty);
+ &tty,
#endif
-
- if (STRCASEEQ(var->name, "authfile")) {
- /* Strip this param, used by virauth.c */
- var->ignore = 1;
- continue;
- }
-
- VIR_DEBUG("passing through variable '%s' ('%s') to
remote end",
- var->name, var->value);
+ &sanity,
+ &verify) < 0) {
+ goto error;
}
/* Construct the original name. */
@@ -1248,8 +1301,6 @@ doRemoteOpen(virConnectPtr conn,
VIR_FREE(priv->hostname);
return VIR_DRV_OPEN_ERROR;
}
-#undef EXTRACT_URI_ARG_STR
-#undef EXTRACT_URI_ARG_BOOL
static struct private_data *
remoteAllocPrivateData(void)
--
2.49.0