Re: [PATCH] pk_mi_json convert functions between a pk_val and a json_object.

[Jose E. Marchesi]

    > Thanks for the new version of the patch.
    > See comments below.
    
    Hello, this is the newest version of the patch where I applied the changes
    discussed in the previous thread.

Very nice!
Just a few more little comments :)

    diff --git a/poke/pk-mi-json.c b/poke/pk-mi-json.c
    index 3ec81924..31d2132c 100644
    --- a/poke/pk-mi-json.c
    +++ b/poke/pk-mi-json.c
    @@ -17,8 +17,8 @@
      */
     
     #include <config.h>
    -
     #include <assert.h>
    +#include <config.h>
     #include <string.h>
     #include <json.h>

Please do not remove the empty line between the include of config.h and
the other includes.  It is customary to do that.
     
    @@ -429,11 +429,882 @@ pk_mi_json_to_msg (const char *str)
         return NULL;
     
       json = json_tokener_parse_ex (tokener, str, strlen (str));
    +  assert (json);
    +
       json_tokener_free (tokener);
     
    -  if (json)
    -    msg = pk_mi_json_object_to_msg (json);
    +  msg = pk_mi_json_object_to_msg (json);
     
       /* XXX: free the json object  */
       return msg;
     }

While you are on it... could you free the json object before returning
the string as the XXX comment says?

    +#define PK_MI_CHECK(errmsg, A, M, ...) \
    +   do                                  \
    +    {                                  \
    +      if (!(A))                        \
    +        {                              \
    +          if (errmsg == NULL) goto error;  \
    +          asprintf (errmsg, "[ERROR] " M , ##__VA_ARGS__); \
    +        }                              \
    +    }                                  \
    +    while (0)

Please assert in case asprintf returns -1, which happens when there is
an out of memory condition.


    +#define PK_MI_DEBUG(M, ...)                  \
    +      fprintf (stderr, "DEBUG %s:%d: " M "\n",\
    +        __FILE__, __LINE__, ##__VA_ARGS__)
    +
     
    +/* Given a Poke value, return the json object associated with this val
    +
    +   VAL is the pk_val to be converted.
    +
    +   ERRMSG is a buffer to be allocated that contains any error messages.
    +
    +   If ERRMSG is NULL, nothing happens on the buffer.
    +
    +   In case of error returns NULL and sets errmsg appropriately.  */
    +
    +const char *pk_mi_val_to_json (pk_val val, char **errmsg);
    +
    +/* Given a json object, create the Poke value associated with it.
    +
    +   VALUE is the Poke value to be created.
    +
    +   STR is the string value of a pk_val to be converted.
    +
    +   ERRMSG is a buffer to be allocated that contains any error messages.
    +
    +   If ERRMSG is NULL, nothing happens on the buffer.
    +
    +   In case of error returns -1 and sets errmsg appropriately.  */
    +
    +int pk_mi_json_to_val (pk_val *value, const char *str, char **errmsg);

In the comment blocks of both pk_mi_val_to_json and pk_mi_json_to_val,
please indicate explicitly that it is up to the user to deallocate the
string returned in ERRMSG.

Given the above little points are fixed, this is OK for master.
Thanks Kostas!