| Message ID | 1360081335-6594-3-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <armbru@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? > - 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? 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.
Eric Blake <eblake@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 <armbru@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!
On Tue, 5 Feb 2013 17:22:05 +0100 Markus Armbruster <armbru@redhat.com> wrote: > The data returned has a well-defined size, which makes the size > returned along with it redundant at best. Drop it. It was me who asked Lei to add it and the reasons were: 1. qemu-ga does it too, so it's good to be consistent and 2. I believe this is good for the QMP as a C interface. I agree with this change as long as we agree that for now we won't return a size for returned strings.
[Forgot to add Michael, re-sending] On Tue, 5 Feb 2013 17:22:05 +0100 Markus Armbruster <armbru@redhat.com> wrote: > The data returned has a well-defined size, which makes the size > returned along with it redundant at best. Drop it. It was me who asked Lei to add it and the reasons were: 1. qemu-ga does it too, so it's good to be consistent and 2. I believe this is good for the QMP as a C interface. I agree with this change as long as we agree that for now we won't return a size for returned strings.
diff --git a/hmp.c b/hmp.c index 9fdf1ce..f6bb767 100644 --- a/hmp.c +++ b/hmp.c @@ -677,21 +677,18 @@ void hmp_memchar_read(Monitor *mon, const QDict *qdict) { uint32_t size = qdict_get_int(qdict, "size"); const char *chardev = qdict_get_str(qdict, "device"); - MemCharRead *meminfo; + char *data; Error *errp = NULL; - meminfo = qmp_memchar_read(chardev, size, false, 0, &errp); + data = qmp_memchar_read(chardev, size, false, 0, &errp); if (errp) { monitor_printf(mon, "%s\n", error_get_pretty(errp)); error_free(errp); return; } - if (meminfo->count > 0) { - monitor_printf(mon, "%s\n", meminfo->data); - } - - qapi_free_MemCharRead(meminfo); + monitor_printf(mon, "%s\n", data); + g_free(data); } static void hmp_cont_cb(void *opaque, int err) diff --git a/qapi-schema.json b/qapi-schema.json index 9e2cbbd..d8fa1c3 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -361,20 +361,6 @@ '*format': 'DataFormat'} } ## -# @MemCharRead -# -# Result of QMP command memchar-read. -# -# @data: The data read from memchar as string. -# -# @count: The numbers of bytes read from. -# -# Since: 1.4 -## -{ 'type': 'MemCharRead', - 'data': { 'data': 'str', 'count': 'int' } } - -## # @memchar-read: # # Provide read interface for memchardev. Read from the char @@ -387,14 +373,14 @@ # @format: #optional the format of the data want to read from # memchardev, by default is 'utf8'. # -# Returns: @MemCharRead +# Returns: data read from the device # If @device is not a valid memchr device, DeviceNotFound # # Since: 1.4 ## { 'command': 'memchar-read', 'data': {'device': 'str', 'size': 'int', '*format': 'DataFormat'}, - 'returns': 'MemCharRead' } + 'returns': 'str' } ## # @CommandInfo: diff --git a/qemu-char.c b/qemu-char.c index 9c1dd13..6de8aff 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -2788,14 +2788,14 @@ void qmp_memchar_write(const char *device, const char *data, } } -MemCharRead *qmp_memchar_read(const char *device, int64_t size, - bool has_format, enum DataFormat format, - Error **errp) +char *qmp_memchar_read(const char *device, int64_t size, + bool has_format, enum DataFormat format, + Error **errp) { CharDriverState *chr; guchar *read_data; - MemCharRead *meminfo; size_t count; + char *data; chr = qemu_chr_find(device); if (!chr) { @@ -2813,26 +2813,23 @@ MemCharRead *qmp_memchar_read(const char *device, int64_t size, return NULL; } - meminfo = g_malloc0(sizeof(MemCharRead)); - count = qemu_chr_cirmem_count(chr); if (count == 0) { - meminfo->data = g_strdup(""); - return meminfo; + return g_strdup(""); } size = size > count ? count : size; read_data = g_malloc0(size + 1); - 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; } - return meminfo; + return data; } QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) diff --git a/qmp-commands.hx b/qmp-commands.hx index 8468f10..51ce2e6 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -525,7 +525,7 @@ Example: "arguments": { "device": foo, "size": 1000, "format": "utf8" } } -<- { "return": { "data": "data string...", "count": 1000 } } +<- {"return": "abcdefgh"} EQMP
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 <armbru@redhat.com> --- hmp.c | 11 ++++------- qapi-schema.json | 18 ++---------------- qemu-char.c | 21 +++++++++------------ qmp-commands.hx | 2 +- 4 files changed, 16 insertions(+), 36 deletions(-)