Re: [Qemu-devel] [PATCH for-1.4 02/12] qmp: Clean up design of memchar-read

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/05/2013 09:22 AM, Markus Armbruster wrote:
>> The data returned has a well-defined size, which makes the size
>> returned along with it redundant at best.  Drop it.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  hmp.c            | 11 ++++-------
>>  qapi-schema.json | 18 ++----------------
>>  qemu-char.c      | 21 +++++++++------------
>>  qmp-commands.hx  |  2 +-
>>  4 files changed, 16 insertions(+), 36 deletions(-)
>
> What happens if the data to be read includes a NUL byte, but the user
> didn't request base64 encoding?

Shit happens :)

I don't want to document its exact color and smell, because I want to
fix it for 1.5.  I made an attempt at documenting the issues, in [PATCH
for-1.4 12/12] QAPI QMP HMP: Fix and improve memchar-read/-write docs.

>> -    meminfo->count = cirmem_chr_read(chr, read_data, size);
>> +    cirmem_chr_read(chr, read_data, size);
>>  
>>      if (has_format && (format == DATA_FORMAT_BASE64)) {
>> -        meminfo->data = g_base64_encode(read_data, (size_t)meminfo->count);
>> +        data = g_base64_encode(read_data, count);
>>      } else {
>> -        meminfo->data = (char *)read_data;
>> +        data = (char *)read_data;
>>      }
>
> This makes it look like the user just gets a truncated read, but is the
> remainder of the buffer lost, or will the next attempt get it as well?
> Would it be better to instead return an error that the user's requested
> encoding is insufficient for the data that is trying to be read?

We need to figure out how we want to deal with problematic buffer
contents.  My current idea is:

* If the ring buffer ate bytes, silently drop initial continuation
  bytes, to make it behave as if it dropped characters rather than
  bytes.

* Normalize overlong sequences.  Except for U+0000, which becomes
  \xC0\x80.

* Replace other invalid sequences by a suitable replacement character.
  U+FFFD is a common choice.

I'm afraid actual coding needs to wait for JSON formatter fixes.  See
"[PATCH v2] check-qjson: More thorough testing of UTF-8 in strings" for
an idea on what's broken.

> At any rate, I agree with the idea of this patch in simplifying the API,
> and concur that we must solve it before 1.4 locks us in to a bad design.

Thanks for reviewing!