Luiz Capitulino <lcapitul...@redhat.com> writes:
> On Tue, 5 Feb 2013 17:22:04 +0100
> Markus Armbruster <arm...@redhat.com> wrote:
>
>> Command memchar-write takes data and size parameter. Begs the
>> question what happens when data doesn't match size.
>>
>> With format base64, qmp_memchar_write() copies the full data argument,
>> regardless of size argument.
>>
>> With format utf8, qmp_memchar_write() copies size bytes from data,
>> happily reading beyond data. Copies crap from the heap or even
>> crashes.
>>
>> Drop the size parameter, and always copy the full data argument.
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> hmp.c | 4 +---
>> qapi-schema.json | 4 +---
>> qemu-char.c | 8 +++-----
>> qmp-commands.hx | 4 +---
>> 4 files changed, 6 insertions(+), 14 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index 1689e6f..9fdf1ce 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -664,13 +664,11 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict)
>>
>> void hmp_memchar_write(Monitor *mon, const QDict *qdict)
>> {
>> - uint32_t size;
>> const char *chardev = qdict_get_str(qdict, "device");
>> const char *data = qdict_get_str(qdict, "data");
>> Error *errp = NULL;
>>
>> - size = strlen(data);
>> - qmp_memchar_write(chardev, size, data, false, 0, &errp);
>> + qmp_memchar_write(chardev, data, false, 0, &errp);
>>
>> hmp_handle_error(mon, &errp);
>> }
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index cdd8384..9e2cbbd 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -346,8 +346,6 @@
>> #
>> # @device: the name of the memory char device.
>> #
>> -# @size: the size to write in bytes.
>> -#
>> # @data: the source data write to memchar.
>
> I agree with this change, but I have an observation.
>
> This is fine for QMP on the wire, as data will always be a string. But
> for QMP as a C interface, this means that we require data to be passed
> as a string (ie. null-terminated).
>
> I've just discussed this with Markus on IRC, and his suggestion is
> that the C interface should be different. I agree with him.
>
> But, should we at least make this explicity? Like changing the
> @DataFormat doc for @utf8 to:
>
> @utf8: The data format is a null-terminated utf8 string
But what does that mean? See below.
> or
>
> @utf8: The data format is a json string
That's trivially true for any json-string argument, including the data
argument with format base64.
Have a look at PATCH 12/12:
# @memchar-write:
[...]
+# @format: #optional data encoding (default 'utf8').
+# - base64: data must be base64 encoded text. It's binary
+# decoding gets written.
+# - utf8: data's UTF-8 encoding is written
+# - data itself is always Unicode regardless of format, like
+# any other string.
[...]
# @memchar-read:
[...]
+# @format: #optional data encoding (default 'utf8').
+# - base64: the data read is returned in base64 encoding.
+# - utf8: the data read is interpreted as UTF-8.
[Bug doc snipped for clarity...]
+# - The return value is always Unicode regardless of format,
+# like any other string.
Back to your question, namely how to document enumeration DataFormat.
Perhaps:
##
# @DataFormat:
#
# An enumeration of data format.
#
# @utf8: Data is a UTF-8 string (RFC 3629)
#
# @base64: Data is a Base64 encoded binary (RFC 3548)
#
# Since: 1.4
##
This omits the fact that data itself is a JSON string, because that's
"obvious". Hmm. Let me try -v:
##
# @DataFormat:
#
# An enumeration describing how a data string is to be converted.
#
# @utf8: The string is to be converted to UTF-8 (RFC 3629). Always
# possible, because the string is in Unicode.
#
# @base64: The string must be Base64 (RFC 3548) encoded binary, and is
# to be converted back to binary.
#
# Since: 1.4
##
