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

[Ján Tomko]

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 <pkre...@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 <jto...@redhat.com>

Jano

signature.asc
Description: PGP signature