[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-1.4 02/12] qmp: Clean up design of memchar-r
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH for-1.4 02/12] qmp: Clean up design of memchar-read |
Date: |
Tue, 05 Feb 2013 18:36:45 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
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!
- [Qemu-devel] [PATCH for-1.4 00/12] Fix memchar-read/-write before API gets released, Markus Armbruster, 2013/02/05
- [Qemu-devel] [PATCH for-1.4 04/12] qmp: Plug memory leaks in memchar-write, memchar-read, Markus Armbruster, 2013/02/05
- [Qemu-devel] [PATCH for-1.4 03/12] qmp: Clean up type usage in qmp_memchar_write(), qmp_memchar_read(), Markus Armbruster, 2013/02/05
- [Qemu-devel] [PATCH for-1.4 02/12] qmp: Clean up design of memchar-read, Markus Armbruster, 2013/02/05
- [Qemu-devel] [PATCH for-1.4 06/12] qmp: Drop wasteful zero-initialization in qmp_memchar_read(), Markus Armbruster, 2013/02/05
- [Qemu-devel] [PATCH for-1.4 07/12] qemu-char: Fix chardev "memory" not to drop IAC characters, Markus Armbruster, 2013/02/05
- [Qemu-devel] [PATCH for-1.4 05/12] qmp: Drop superfluous special case "empty" in qmp_memchar_read(), Markus Armbruster, 2013/02/05
- [Qemu-devel] [PATCH for-1.4 01/12] qmp: Fix design bug and read beyond buffer in memchar-write, Markus Armbruster, 2013/02/05