[PATCH 06/16] qemuMonitorJSONAttachCharDevGetProps: Modernize construction of JSON objects

Ján Tomko jtomko at redhat.com
Fri Nov 19 10:31:54 UTC 2021


On a Thursday in 2021, Peter Krempa wrote:
>Use 'virJSONValueObjectAdd' instead of the step-by-step manual JSON
>object building.
>
>Signed-off-by: Peter Krempa <pkrempa at redhat.com>
>---
> src/qemu/qemu_monitor_json.c | 195 ++++++++++++++++++-----------------
> 1 file changed, 99 insertions(+), 96 deletions(-)
>
>diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>index 1ced942161..a9e87de4d3 100644
>--- a/src/qemu/qemu_monitor_json.c
>+++ b/src/qemu/qemu_monitor_json.c
> qemuMonitorJSONAttachCharDevGetProps(const char *chrID,
>                                      const virDomainChrSourceDef *chr)
> {
>     g_autoptr(virJSONValue) props = NULL;
>-    g_autoptr(virJSONValue) backend = virJSONValueNewObject();
>-    g_autoptr(virJSONValue) data = virJSONValueNewObject();
>-    g_autoptr(virJSONValue) addr = NULL;
>-    const char *backend_type = NULL;
>-    const char *host;
>-    const char *port;
>-    g_autofree char *tlsalias = NULL;
>-    bool telnet;
>+    g_autoptr(virJSONValue) backend = NULL;
>+    g_autoptr(virJSONValue) backendData = virJSONValueNewObject();
>+    const char *backendType = NULL;

The backendType and backendData renames could've been done upfront to
simplify this patch, at the cost of changing the same lines multiple
times.

>
>     switch ((virDomainChrType)chr->type) {
>     case VIR_DOMAIN_CHR_TYPE_NULL:
>-        backend_type = "null";
>-        break;
>-
>     case VIR_DOMAIN_CHR_TYPE_VC:
>-        backend_type = "vc";
>-        break;
>-
>     case VIR_DOMAIN_CHR_TYPE_PTY:
>-        backend_type = "pty";
>+        backendType = virDomainChrTypeToString(chr->type);

[...]

>-        if (qemuMonitorJSONBuildChrChardevReconnect(data, &chr->data.tcp.reconnect) < 0)
>-            return NULL;
>-        break;
>
>-    case VIR_DOMAIN_CHR_TYPE_UDP:
>-        backend_type = "udp";
>-        host = chr->data.udp.connectHost;
>-        if (!host)
>-            host = "";
>-        addr = qemuMonitorJSONBuildInetSocketAddress(host,
>-                                                     chr->data.udp.connectService);
>-        if (!addr ||
>-            virJSONValueObjectAppend(data, "remote", &addr) < 0)
>-            return NULL;
>+            if (chr->data.tcp.reconnect.enabled == VIR_TRISTATE_BOOL_YES)
>+                reconnect = chr->data.tcp.reconnect.timeout;
>+            else if (chr->data.tcp.reconnect.enabled == VIR_TRISTATE_BOOL_NO)
>+                reconnect = 0;

Before, qemuMonitorJSONBuildChrChardevReconnect only formatted the
timeout value if enabled was set to BOOL_YES.

[A] Please split out the == BOOL_NO additions that unify it with what we do
in the cold start commandline generator.

>+        } else {
>+            if (chr->data.nix.listen) {
>+                server = true;
>+                waitval = VIR_TRISTATE_BOOL_NO;
>+            }
>

[...]

>@@ -6748,15 +6748,18 @@ qemuMonitorJSONAttachCharDevGetProps(const char *chrID,
>         return NULL;
>     }
>
>-    if (chr->logfile &&
>-        virJSONValueObjectAdd(&data,
>-                              "s:logfile", chr->logfile,
>-                              "T:logappend", chr->logappend,
>-                              NULL) < 0)
>-        return NULL;
>+    if (chr->logfile) {
>+        if (virJSONValueObjectAdd(&backendData,
>+                                  "S:logfile", chr->logfile,
>+                                  "T:logappend", chr->logappend,
>+                                  NULL) < 0)
>+            return NULL;
>+    }

Apart from the style change [B] There's no need to change 's' to 'S'
here.

>
>-    if (virJSONValueObjectAppendString(backend, "type", backend_type) < 0 ||
>-        virJSONValueObjectAppend(backend, "data", &data) < 0)
>+    if (virJSONValueObjectAdd(&backend,
>+                              "s:type", backendType,
>+                              "A:data", &backendData,
>+                              NULL) < 0)
>         return NULL;
>

With [A][B] addressed,

Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 484 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20211119/28b6b7ee/attachment-0001.sig>


More information about the libvir-list mailing list