Re: [Qemu-devel] [PATCH 3/4] QAPI: Introduce memchar-read QMP command

[Lei Li <lilei@linux.vnet.ibm.com>]

On 10/27/2012 01:39 AM, Luiz Capitulino wrote:
> On Fri, 26 Oct 2012 19:21:51 +0800
> Lei Li <lilei@linux.vnet.ibm.com> wrote:
>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   hmp-commands.hx  |   19 ++++++++++++++++++
>>   hmp.c            |   19 ++++++++++++++++++
>>   hmp.h            |    1 +
>>   qapi-schema.json |   27 ++++++++++++++++++++++++++
>>   qemu-char.c      |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qmp-commands.hx  |   40 +++++++++++++++++++++++++++++++++++++++
>>   6 files changed, 161 insertions(+), 0 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index a37b8e9..df294eb 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -842,6 +842,25 @@ is full/empty, for now just assume a drop behaver in these two commands.
>>   ETEXI
>>   
>>       {
>> +        .name       = "memchar_read",
>> +        .args_type  = "chardev:s,size:i",
>> +        .params     = "chardev size",
>> +        .mhandler.cmd = hmp_memchar_read,
>> +    },
>> +
>> +STEXI
>> +@item memchar_read @var{chardev}
>> +@findex memchar_read
>> +Provide read interface for CirMemCharDriver. Read from cirmemchr
>> +char device and return @var{size} of the data.
>> +
>> +@var{size} is the size of data want to read from. Refer to unencoded
>> +size of the raw data, would adjust to the init size of the memchar
>> +if the requested size is larger than it.
>> +
>> +ETEXI
>> +
>> +    {
>>           .name       = "migrate",
>>           .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
>>           .params     = "[-d] [-b] [-i] uri",
>> diff --git a/hmp.c b/hmp.c
>> index 082985b..ef85736 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -698,6 +698,25 @@ void hmp_memchar_write(Monitor *mon, const QDict *qdict)
>>       hmp_handle_error(mon, &errp);
>>   }
>>   
>> +void hmp_memchar_read(Monitor *mon, const QDict *qdict)
>> +{
>> +    uint32_t size = qdict_get_int(qdict, "size");
>> +    const char *chardev = qdict_get_str(qdict, "chardev");
>> +    char *data;
>> +    enum DataFormat format;
> You don't need this variable.

ok.

>
>> +    Error *errp = NULL;
>> +
>> +    format = DATA_FORMAT_UTF8;
>> +    data = qmp_memchar_read(chardev, size, true, format, &errp);
>> +    if (errp) {
>> +        monitor_printf(mon, "%s\n", error_get_pretty(errp));
>> +        error_free(errp);
>> +        return;
>> +    }
>> +
>> +    monitor_printf(mon, "%s\n", data);
>> +}
>> +
>>   static void hmp_cont_cb(void *opaque, int err)
>>   {
>>       if (!err) {
>> diff --git a/hmp.h b/hmp.h
>> index 406ebb1..a5a0cfe 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -44,6 +44,7 @@ void hmp_cpu(Monitor *mon, const QDict *qdict);
>>   void hmp_memsave(Monitor *mon, const QDict *qdict);
>>   void hmp_pmemsave(Monitor *mon, const QDict *qdict);
>>   void hmp_memchar_write(Monitor *mon, const QDict *qdict);
>> +void hmp_memchar_read(Monitor *mon, const QDict *qdict);
>>   void hmp_cont(Monitor *mon, const QDict *qdict);
>>   void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
>>   void hmp_inject_nmi(Monitor *mon, const QDict *qdict);
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 43ef6bc..a8c9430 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -372,6 +372,33 @@
>>              '*format': 'DataFormat'} }
>>   
>>   ##
>> +# @memchar-read:
>> +#
>> +# Provide read interface for memchardev. Read from memchar
>> +# char device and return the data.
>> +#
>> +# @chardev: the name of the memchar char device.
>> +#
>> +# @size: the size to read in bytes.
>> +#
>> +# @format: #optional the format of the data want to read from
>> +#          memchardev, by default is 'utf8'.
>> +#
>> +# Returns: The data read from memchar as string
>> +#          If @chardev is not a valid memchr device, DeviceNotFound
>> +#
>> +# Notes: The option 'block' is not supported now due to the miss
>> +#        feature in qmp. Will add it later when we gain the necessary
>> +#        infrastructure enhancement. For now just assume 'drop' behaver
>> +#        for this command.
> Please, replace this note with an explanation of the current behavior. No
> need to talk about the future.

ok.

>
>> +#
>> +# Since: 1.3
>> +##
>> +{ 'command': 'memchar-read',
>> +  'data': {'chardev': 'str', 'size': 'int', '*format': 'DataFormat'},
>> +  'returns': 'str' }
>> +
>> +##
>>   # @CommandInfo:
>>   #
>>   # Information about a QMP command
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 6114e29..cf88f71 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2761,6 +2761,61 @@ void qmp_memchar_write(const char *chardev, int64_t size,
>>       }
>>   }
>>   
>> +char *qmp_memchar_read(const char *chardev, int64_t size,
>> +                       bool has_format, enum DataFormat format,
>> +                       Error **errp)
>> +{
>> +    CharDriverState *chr;
>> +    guchar *read_data;
>> +    char *data = NULL;
>> +    int ret;
>> +
>> +    read_data = g_malloc0(size);
> This is unsafe, as qmp clients could pass any value here. You should check
> the number of available bytes and allocate only that.

This size is not the init size of ring buffer, it's the size user want to read
from the cirmem backend. And it'll be checked within cirmem_chr_write.

>
>> +
>> +    chr = qemu_chr_find(chardev);
>> +    if (!chr) {
>> +        error_set(errp, QERR_DEVICE_NOT_FOUND, chardev);
>> +        goto fail;
>> +    }
>> +
>> +    if (strcmp(chr->filename, "memchr") != 0) {
>> +        error_setg(errp,"The %s is not memory char device\n", chardev);
>> +        goto fail;
>> +    }
> The same comment I made for the write operation applies here.
>
>> +
>> +    /* XXX: For the sync command as 'block', waiting for the qmp
>> +     * to support necessary feature. Now just act as 'drop'. */
> Here too.
>
>> +    if (cirmem_chr_is_empty(chr)) {
>> +        error_setg(errp, "Failed to read from memchr %s", chardev);
>> +        goto fail;
>> +    }
>> +
>> +    if (size == 0) {
>> +        size = CBUFF_SIZE;
>> +    }
> IMO, you should refuse size=0.

Do you think it's better to refuse it than giving a default size?

>
>> +
>> +    ret = cirmem_chr_read(chr, read_data, size);
>> +
>> +    if (ret < 0) {
>> +        error_setg(errp, "Failed to read from memchr %s", chardev);
>> +        goto fail;
>> +    }
>> +
>> +    if (has_format && (format == DATA_FORMAT_BASE64)) {
>> +       if (read_data) {
>> +           data = g_base64_encode(read_data, (size_t)size);
>> +       }
>> +    } else {
>> +        data = (char *)read_data;
>> +    }
>> +
>> +    return data;
>> +
>> +fail:
>> +    g_free(read_data);
>> +    return NULL;
>> +}
>> +
>>   QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>>   {
>>       char host[65], port[33], width[8], height[8];
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 7548b9b..7729fb0 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -500,6 +500,46 @@ Example:
>>   EQMP
>>   
>>       {
>> +        .name       = "memchar-read",
>> +        .args_type  = "chardev:s,size:i,format:s?",
>> +        .help       = "return the size of data from memchar chardev",
>> +        .mhandler.cmd_new = qmp_marshal_input_memchar_read,
>> +    },
>> +
>> +SQMP
>> +memchar-read
>> +-------------
>> +
>> +Provide read interface for memchardev. Read from memchar
>> +char device and return the data.
>> +
>> +Arguments:
>> +
>> +- "chardev": the name of the char device, must be unique (json-string)
>> +- "size": the memory size wanted to read in bytes(refer to unencoded
>> +          size of the raw data), would adjust to the init size of the
>> +          memchar if the requested size is larger than it. (json-int)
>> +- "format": the data format write to memchardev, default is
>> +            utf8. (json-string, optional)
>> +          - Possible values: "utf8", "base64"
>> +
>> +Example:
>> +
>> +-> { "execute": "memchar-read",
>> +                "arguments": { "chardev": foo,
>> +                               "size": 1000,
>> +                               "format": "utf8" } }
>> +<- { "return": "data string..." }
>> +
>> +Notes:
>> +
>> +We will add 'control' options for read and write command that specifies
>> +behavior when the queue is full/empty, for now just assume a drop
>> +behaver in these two commands.
> Please drop this.

Sure.

>
>> +
>> +EQMP
>> +
>> +    {
>>           .name       = "xen-save-devices-state",
>>           .args_type  = "filename:F",
>>       .mhandler.cmd_new = qmp_marshal_input_xen_save_devices_state,
>


-- 
Lei