Patchwork [for-1.4,02/12] qmp: Clean up design of memchar-read

login
register
mail settings
Submitter Markus Armbruster
Date Feb. 5, 2013, 4:22 p.m.
Message ID <1360081335-6594-3-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/218291/
State New
Headers show

Comments

Markus Armbruster - Feb. 5, 2013, 4:22 p.m.
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(-)
Eric Blake - Feb. 5, 2013, 5:10 p.m.
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.
Markus Armbruster - Feb. 5, 2013, 5:36 p.m.
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!
Luiz Capitulino - Feb. 6, 2013, 1:44 p.m.
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.
Luiz Capitulino - Feb. 6, 2013, 1:45 p.m.
[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.

Patch

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