Re: [Qemu-devel] [PATCH v2 03/18] cutils: Add qemu_strdup_hexlify() and qemu_strdup_unhexlify()

[Laszlo Ersek <lersek@redhat.com>]

Hi Phil,

most important comment at the bottom.

On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
> Add two helpers: one to represent a binary data as a string of
> hexadecimal values, and one to restore a such string into its
> original binary data.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/cutils.h | 33 ++++++++++++++++++++++++++
>  util/cutils.c         | 55 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 88 insertions(+)
> 
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index d2dad3057c..375a5508b0 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -171,6 +171,39 @@ bool test_buffer_is_zero_next_accel(void);
>  int uleb128_encode_small(uint8_t *out, uint32_t n);
>  int uleb128_decode_small(const uint8_t *in, uint32_t *n);
>  
> +/**
> + * qemu_strdup_hexlify:

(1) I think the name "hexlify" is unusual. I think we should use
encode/decode terminology, or hex/unhex, or, if we want to stick with
the "stringify" pattern, hexify/unhexify. (No "l".)

> + *
> + * Encode a sequence of binary data into its hexadecimal stringified
> + * representation.
> + *
> + * @ptr: Buffer to hexlify
> + * @size: Length of the buffer
> + *
> + * Use qemu_strdup_unhexlify() to convert the hex string to original data.
> + *
> + * Returns: A newly allocated, zero-terminated hex encoded string representing
> + * the data. The returned string must be freed with g_free().
> + */
> +gchar *qemu_strdup_hexlify(gconstpointer ptr, gsize size);
> +
> +/**
> + * qemu_strdup_unhexlify:
> + *
> + * Decode a sequence of hexadecimal encoded text into binary data.
> + *
> + * @hex_string: String to unhexlify
> + * @out_size: if not NULL: gsize to be written with the data length
> + *
> + * This function is the opposite of qemu_strdup_hexlify().
> + *
> + * Returns: A newly allocated buffer containing the binary data that text
> + * represents. The returned buffer must be freed with g_free().
> + * Note that the returned binary data is not necessarily zero-terminated,
> + * so it should not be used as a character string.
> + */
> +gpointer qemu_strdup_unhexlify(const gchar *hex_string, gsize *out_size);
> +
>  /**
>   * qemu_pstrcmp0:
>   * @str1: a non-NULL pointer to a C string (*str1 can be NULL)
> diff --git a/util/cutils.c b/util/cutils.c
> index e098debdc0..bf324c0d8b 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -779,6 +779,61 @@ int uleb128_decode_small(const uint8_t *in, uint32_t *n)
>      }
>  }
>  
> +static guchar hexval(const gchar v)
> +{
> +    switch (v) {
> +    case '0' ... '9':
> +        return v - '0';
> +    case 'A' ... 'F':
> +        return v - 'A' + 10;
> +    case 'a' ... 'f':
> +        return v - 'a' + 10;
> +    default:
> +        return 0;
> +    }
> +}

(2) I don't think that we should silently translate invalid characters
to zero, in any hexadecimal decoder.

> +
> +gchar *qemu_strdup_hexlify(gconstpointer ptr, gsize len)
> +{
> +    guchar *data = (guchar *)ptr;
> +    gchar *hex_string;
> +
> +    if (!ptr || !len) {
> +        return g_strdup("");
> +    }
> +
> +    hex_string = g_malloc(2 * len + 1);

(3) Should check against integer overflow in the g_malloc() argument
(multiplication and addition).

> +    for (gsize i = 0; i < len; i++) {
> +        g_snprintf(&hex_string[2 * i], 3, "%02x", data[i]);
> +    }
> +
> +    return hex_string;
> +}
> +
> +gpointer qemu_strdup_unhexlify(const gchar *hex_string, gsize *out_size)
> +{
> +    size_t size = 0;
> +    guchar *data = NULL;
> +
> +    if (hex_string) {
> +        size = strlen(hex_string) / 2;

(4) Should likely check that the length of the string is an even integer.

> +        if (size) {
> +            size_t i;
> +
> +            data = g_new(guchar, size + 1);
> +            for (i = 0; i < size; i++) {
> +                data[i]  = hexval(*hex_string++) << 4;
> +                data[i] |= hexval(*hex_string++);
> +            }
> +            data[i] = '\0';
> +        }
> +    }
> +    if (out_size) {
> +        *out_size = size;
> +    }
> +    return data;
> +}
> +
>  /*
>   * helper to parse debug environment variables
>   */
> 

(5) Most importantly: I don't think we need this patch.

First, AFAICS, the unhex function is never used in the series, and no
unit test is being added for it. That makes it a bad candidate for
"include/qemu/cutils.h".

Second, while the hex function is used in PATCH v2 13/18
("hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command"), the documentation in
that patch and the logic in the patch are inconsistent. The
documentation -- i.e. both the commit message and the "misc.json" change
-- say that "FirmwareConfigurationItem.data" is unused (not populated).
However, that's exactly what create_qmp_fw_cfg_item() uses the hex
function for.

Third, if we do decide that the QMP command should output the fw_cfg
binary data, then the QMP tradition (to my knowledge) has been to use
base64 encoding. GLib provides helpers for base64:

https://developer.gnome.org/glib/stable/glib-Base64-Encoding.html

and you can see examples of it being used in e.g.

(a) qmp_ringbuf_read() [chardev/char-ringbuf.c] -- the  @ringbuf-read
command is defined in "qapi/char.json"

(b) qmp_guest_exec_status() [qga/commands.c] -- the @guest-exec-status
command is defined in "qga/qapi-schema.json".

Thanks
Laszlo