[PATCH 003/103] virJSONValueObjectAddVArgs: Add new convertors for allocated strings

Peter Krempa posted 103 patches 4 years, 4 months ago
Only 102 patches received!
[PATCH 003/103] virJSONValueObjectAddVArgs: Add new convertors for allocated strings
Posted by Peter Krempa 4 years, 4 months ago
The 'f' and 'F' convertors add a string value to the JSON value object
and free the passed pointer. This is helpful in case we need to e.g. add
single use formatted values:

 virJSONValueObjectAdd(props,
                       "f:test1", g_strdup_printf("%s-test1", blah),
                       "F:test2", virGetStringAndReportErrorOnNull(),
                       NULL);

With 'F' if the passed string is NULL no additional error is reported so
the error is passed through.

To prevent leaking the strings on reasonable failures we add an 'err'
variable which skips the implementation of the conversions, but keeps
iterating through the arguments.

On hard programming errors (bad key format) the string is leaked but
that should not be a problem in practice.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/util/virjson.c | 82 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 68 insertions(+), 14 deletions(-)

diff --git a/src/util/virjson.c b/src/util/virjson.c
index d7e72af8f9..4d9cd5629a 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -112,6 +112,8 @@ virJSONValueGetType(const virJSONValue *value)
  * Explanation of type codes:
  * s: string value, must be non-null
  * S: string value, omitted if null
+ * f: string value, passed pointer is freed, error reported if NULL
+ * F: string value, passed pointer is freed, no error reported if NULL
  *
  * i: signed integer value
  * j: signed integer value, error if negative
@@ -155,6 +157,7 @@ int
 virJSONValueObjectAddVArgs(virJSONValue *obj,
                            va_list args)
 {
+    bool err = false;
     char type;
     char *key;
     int rc;
@@ -173,17 +176,30 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,

         /* This doesn't support maps, but no command uses those.  */
         switch (type) {
+        case 'f':
+        case 'F':
         case 'S':
         case 's': {
             char *val = va_arg(args, char *);
+            g_autofree char *valf = NULL;
+
+            if (type == 'f' || type == 'F')
+                valf = val;
+
+            if (err)
+                continue;
+
             if (!val) {
                 if (type == 'S')
                     continue;

-                virReportError(VIR_ERR_INTERNAL_ERROR,
-                               _("argument key '%s' must not have null value"),
-                               key);
-                return -1;
+                if (type != 'F') {
+                    virReportError(VIR_ERR_INTERNAL_ERROR,
+                                   _("argument key '%s' must not have null value"),
+                                   key);
+                }
+                err = true;
+                continue;
             }
             rc = virJSONValueObjectAppendString(obj, key, val);
         }   break;
@@ -195,11 +211,15 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,
         case 'i': {
             int val = va_arg(args, int);

+            if (err)
+                continue;
+
             if (val < 0 && (type == 'j' || type == 'y')) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("argument key '%s' must not be negative"),
                                key);
-                return -1;
+                err = true;
+                continue;
             }

             if (val == 0 && (type == 'z' || type == 'y'))
@@ -215,6 +235,9 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,
         case 'u': {
             unsigned int val = va_arg(args, unsigned int);

+            if (err)
+                continue;
+
             if (!val && type == 'p')
                 continue;

@@ -227,11 +250,15 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,
         case 'I': {
             long long val = va_arg(args, long long);

+            if (err)
+                continue;
+
             if (val < 0 && (type == 'J' || type == 'Y')) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("argument key '%s' must not be negative"),
                                key);
-                return -1;
+                err = true;
+                continue;
             }

             if (!val && (type == 'Z' || type == 'Y'))
@@ -249,6 +276,9 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,
              */
             long long val = va_arg(args, long long);

+            if (err)
+                continue;
+
             if (!val && type == 'P')
                 continue;

@@ -257,6 +287,10 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,

         case 'd': {
             double val = va_arg(args, double);
+
+            if (err)
+                continue;
+
             rc = virJSONValueObjectAppendNumberDouble(obj, key, val);
         }   break;

@@ -265,6 +299,9 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,
         case 'b': {
             int val = va_arg(args, int);

+            if (err)
+                continue;
+
             if (!val && type == 'B')
                 continue;

@@ -282,6 +319,9 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,
         }   break;

         case 'n': {
+            if (err)
+                continue;
+
             rc = virJSONValueObjectAppendNull(obj, key);
         }   break;

@@ -289,6 +329,9 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,
         case 'a': {
             virJSONValue **val = va_arg(args, virJSONValue **);

+            if (err)
+                continue;
+
             if (!(*val)) {
                 if (type == 'A')
                     continue;
@@ -296,7 +339,8 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("argument key '%s' must not have null value"),
                                key);
-                return -1;
+                err = true;
+                continue;
             }

             rc = virJSONValueObjectAppend(obj, key, val);
@@ -308,6 +352,9 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,
             g_autoptr(virJSONValue) jsonMap = virJSONValueNewArray();
             ssize_t pos = -1;

+            if (err)
+                continue;
+
             if (!map) {
                 if (type == 'M')
                     continue;
@@ -315,18 +362,20 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("argument key '%s' must not have null value"),
                                key);
-                return -1;
+                err = true;
+                continue;
             }

             while ((pos = virBitmapNextSetBit(map, pos)) > -1) {
                 g_autoptr(virJSONValue) newelem = virJSONValueNewNumberLong(pos);

-                if (virJSONValueArrayAppend(jsonMap, &newelem) < 0)
-                    return -1;
+                if (virJSONValueArrayAppend(jsonMap, &newelem) < 0) {
+                    err = true;
+                    continue;
+                }
             }

-            if ((rc = virJSONValueObjectAppend(obj, key, &jsonMap)) < 0)
-                return -1;
+            rc = virJSONValueObjectAppend(obj, key, &jsonMap);
         } break;

         default:
@@ -335,10 +384,15 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,
             return -1;
         }

-        if (rc < 0)
-            return -1;
+        if (rc < 0) {
+            err = true;
+            continue;
+        }
     }

+    if (err)
+        return -1;
+
     /* verify that we added at least one key-value pair */
     if (virJSONValueObjectKeysNumber(obj) == 0)
         return 0;
-- 
2.31.1

Re: [PATCH 003/103] virJSONValueObjectAddVArgs: Add new convertors for allocated strings
Posted by Ján Tomko 4 years, 4 months ago
On a Thursday in 2021, Peter Krempa wrote:
>The 'f' and 'F' convertors add a string value to the JSON value object
>and free the passed pointer. This is helpful in case we need to e.g. add
>single use formatted values:
>

I'm not a fan of conditionally freeing the passed pointer based on some
value. Especially now that we have g_auto, it should be easy enough to
take care of in the caller.

> virJSONValueObjectAdd(props,
>                       "f:test1", g_strdup_printf("%s-test1", blah),
>                       "F:test2", virGetStringAndReportErrorOnNull(),
>                       NULL);
>
>With 'F' if the passed string is NULL no additional error is reported so
>the error is passed through.
>
>To prevent leaking the strings on reasonable failures we add an 'err'
>variable which skips the implementation of the conversions, but keeps
>iterating through the arguments.
>

>On hard programming errors (bad key format) the string is leaked but
>that should not be a problem in practice.

This sentence does not help sell it either :)

Jano

>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/util/virjson.c | 82 ++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 68 insertions(+), 14 deletions(-)
>