Eric Blake <ebl...@redhat.com> 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 <arm...@redhat.com> >> --- >> 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!