Laszlo Ersek <ler...@redhat.com> writes:
> 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é <phi...@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.
hexlify-buffer is an interactive autoloaded Lisp function in
‘hexl.el’.
(hexlify-buffer)
Convert a binary buffer to hexl format.
This discards the buffer’s undo information.
;-P
> 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
Similar parameters elsewhere in this header are called @buf.
>> + * @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);
Avoid the silly GLib types, please.
>> +
>> +/**
>> + * 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)
Naming the parameter @ch would be more idiomatic, I think.
>> +{
>> + 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.
Yup. Let's abort().
>> +
>> +gchar *qemu_strdup_hexlify(gconstpointer ptr, gsize len)
>> +{
>> + guchar *data = (guchar *)ptr;
>> + gchar *hex_string;
>> +
>> + if (!ptr || !len) {
>> + return g_strdup("");
>> + }
A null pointer is not the same as the empty string. Replace this by
assert(ptr);
and ...
>> +
>> + hex_string = g_malloc(2 * len + 1);
>
> (3) Should check against integer overflow in the g_malloc() argument
> (multiplication and addition).
E.g.
assert(len <= (SIZE_MAX - 1) / 2);
>> + for (gsize i = 0; i < len; i++) {
>> + g_snprintf(&hex_string[2 * i], 3, "%02x", data[i]);
>> + }
>> +
... esnure termination here
hex_string[2 * i] = 0;
What does g_snprintf() buy us over plain snprintf()? I count 400+ uses
of the latter, and none of the former.
>> + return hex_string;
>> +}
>> +
>> +gpointer qemu_strdup_unhexlify(const gchar *hex_string, gsize *out_size)
>> +{
>> + size_t size = 0;
>> + guchar *data = NULL;
>> +
>> + if (hex_string) {
A null pointer is not the same as the empty string. assert(hex_string)
and make the conversion unconditional.
>> + 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;
This maps "" to null. I think it shold return "". It naturally does if
you make the if (size) code unconditional.
>> +}
>> +
>> /*
>> * 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".
Yes. I wish you had wrote that first, saving me the trouble of looking
at the patch.
