[libvirt PATCH 0/4] remote: Random RPC fixes and cleanups
Jiri Denemark (4): remote: Propagate error from virDomainGetSecurityLabelList via RPC remote: Drop useless check in remoteDispatchDomainGetIOThreadInfo remote: Avoid leaking uri_out remote: Drop useless cleanup in remoteDispatchNodeGet{CPU,Memory}Stats src/remote/remote_daemon_dispatch.c | 48 +++++++++-------------------- 1 file changed, 15 insertions(+), 33 deletions(-) -- 2.39.1
The daemon side of this API has been broken ever since the API was introduced in 2012. Instead of sending the error from virDomainGetSecurityLabelList via RPC so that the client can see it, the dispatcher would just send a successful reply with return value set to -1 (and an empty array of labels). The client side would propagate this return value so the client can see the API failed, but the original error would be lost. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/remote/remote_daemon_dispatch.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 6c56e9ec3e..4d993afee6 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -2637,13 +2637,11 @@ remoteDispatchDomainGetSecurityLabelList(virNetServer *server G_GNUC_UNUSED, if (!(dom = get_nonnull_domain(conn, args->dom))) goto cleanup; - if ((len = virDomainGetSecurityLabelList(dom, &seclabels)) < 0) { - ret->ret = len; - ret->labels.labels_len = 0; - ret->labels.labels_val = NULL; - goto done; - } + if ((len = virDomainGetSecurityLabelList(dom, &seclabels)) < 0) + goto cleanup; + ret->ret = len; + ret->labels.labels_len = len; ret->labels.labels_val = g_new0(remote_domain_get_security_label_ret, len); for (i = 0; i < len; i++) { @@ -2653,9 +2651,7 @@ remoteDispatchDomainGetSecurityLabelList(virNetServer *server G_GNUC_UNUSED, cur->label.label_len = label_len; cur->enforcing = seclabels[i].enforcing; } - ret->labels.labels_len = ret->ret = len; - done: rv = 0; cleanup: -- 2.39.1
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/remote/remote_daemon_dispatch.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 4d993afee6..dbe6825fb8 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -2968,9 +2968,8 @@ remoteDispatchDomainGetIOThreadInfo(virNetServer *server G_GNUC_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dom); - if (ninfo >= 0) - for (i = 0; i < ninfo; i++) - virDomainIOThreadInfoFree(info[i]); + for (i = 0; i < ninfo; i++) + virDomainIOThreadInfoFree(info[i]); VIR_FREE(info); return rv; -- 2.39.1
On a Thursday in 2023, Jiri Denemark wrote:
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Can you elaborate on why you find this useless?
src/remote/remote_daemon_dispatch.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 4d993afee6..dbe6825fb8 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -2968,9 +2968,8 @@ remoteDispatchDomainGetIOThreadInfo(virNetServer *server G_GNUC_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dom); - if (ninfo >= 0)
If ninfo is less than zero
- for (i = 0; i < ninfo; i++)
it will get promoted to unsigned here and the loop will take a long time, freeing some fun stuff in the meantime. Jano
- virDomainIOThreadInfoFree(info[i]); + for (i = 0; i < ninfo; i++) + virDomainIOThreadInfoFree(info[i]); VIR_FREE(info);
return rv; -- 2.39.1
On Thu, Jan 26, 2023 at 13:51:26 +0100, Ján Tomko wrote:
On a Thursday in 2023, Jiri Denemark wrote:
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Can you elaborate on why you find this useless?
src/remote/remote_daemon_dispatch.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 4d993afee6..dbe6825fb8 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -2968,9 +2968,8 @@ remoteDispatchDomainGetIOThreadInfo(virNetServer *server G_GNUC_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dom); - if (ninfo >= 0)
If ninfo is less than zero
- for (i = 0; i < ninfo; i++)
it will get promoted to unsigned here and the loop will take a long time, freeing some fun stuff in the meantime.
Oops. Mostly because I didn't realize i is unsigned and I changed the function so that ninfo cannot ever be negative (although I dropped the change later anyway) :-) Jirka
In case the API returned success and a NULL pointer in uri_out, we would leak the preallocated buffer used for storing the uri_out pointer. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/remote/remote_daemon_dispatch.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index dbe6825fb8..6cf88e22c3 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -3061,15 +3061,14 @@ remoteDispatchDomainMigratePrepare2(virNetServer *server G_GNUC_UNUSED, */ ret->cookie.cookie_len = cookielen; ret->cookie.cookie_val = cookie; - ret->uri_out = *uri_out == NULL ? NULL : uri_out; + ret->uri_out = *uri_out == NULL ? NULL : g_steal_pointer(&uri_out); rv = 0; cleanup: - if (rv < 0) { + if (rv < 0) virNetMessageSaveError(rerr); - VIR_FREE(uri_out); - } + VIR_FREE(uri_out); return rv; } @@ -4776,15 +4775,14 @@ remoteDispatchDomainMigratePrepare3(virNetServer *server G_GNUC_UNUSED, */ ret->cookie_out.cookie_out_len = cookieoutlen; ret->cookie_out.cookie_out_val = cookieout; - ret->uri_out = *uri_out == NULL ? NULL : uri_out; + ret->uri_out = *uri_out == NULL ? NULL : g_steal_pointer(&uri_out); rv = 0; cleanup: - if (rv < 0) { + if (rv < 0) virNetMessageSaveError(rerr); - VIR_FREE(uri_out); - } + VIR_FREE(uri_out); return rv; } @@ -5572,16 +5570,15 @@ remoteDispatchDomainMigratePrepare3Params(virNetServer *server G_GNUC_UNUSED, ret->cookie_out.cookie_out_len = cookieoutlen; ret->cookie_out.cookie_out_val = cookieout; - ret->uri_out = !*uri_out ? NULL : uri_out; + ret->uri_out = !*uri_out ? NULL : g_steal_pointer(&uri_out); rv = 0; cleanup: virTypedParamsFree(params, nparams); - if (rv < 0) { + if (rv < 0) virNetMessageSaveError(rerr); - VIR_FREE(uri_out); - } + VIR_FREE(uri_out); return rv; } -- 2.39.1
The function cannot fail once it starts populating ret->params.params_val[i].field. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/remote/remote_daemon_dispatch.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 6cf88e22c3..16bb23f049 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -3309,11 +3309,6 @@ remoteDispatchNodeGetCPUStats(virNetServer *server G_GNUC_UNUSED, cleanup: if (rv < 0) { virNetMessageSaveError(rerr); - if (ret->params.params_val) { - for (i = 0; i < nparams; i++) - VIR_FREE(ret->params.params_val[i].field); - VIR_FREE(ret->params.params_val); - } } VIR_FREE(params); return rv; @@ -3376,11 +3371,6 @@ remoteDispatchNodeGetMemoryStats(virNetServer *server G_GNUC_UNUSED, cleanup: if (rv < 0) { virNetMessageSaveError(rerr); - if (ret->params.params_val) { - for (i = 0; i < nparams; i++) - VIR_FREE(ret->params.params_val[i].field); - VIR_FREE(ret->params.params_val); - } } VIR_FREE(params); return rv; -- 2.39.1
On 1/26/23 11:29, Jiri Denemark wrote:
Jiri Denemark (4): remote: Propagate error from virDomainGetSecurityLabelList via RPC remote: Drop useless check in remoteDispatchDomainGetIOThreadInfo remote: Avoid leaking uri_out remote: Drop useless cleanup in remoteDispatchNodeGet{CPU,Memory}Stats
src/remote/remote_daemon_dispatch.c | 48 +++++++++-------------------- 1 file changed, 15 insertions(+), 33 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Jiri Denemark -
Ján Tomko -
Michal Prívozník