Re: [Qemu-devel] [PATCH 3/5] QAPI: Introduce memchar-write QMP command

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 22 Oct 2012 00:47:59 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  hmp-commands.hx  |   22 +++++++++++++++++
>  hmp.c            |   19 +++++++++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-char.c      |   46 ++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |   40 +++++++++++++++++++++++++++++++
>  6 files changed, 197 insertions(+), 0 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e0b537d..753aab3 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -825,6 +825,28 @@ Inject an NMI on the given CPU (x86 only).
>  ETEXI
>  
>      {
> +        .name       = "memchar_write",
> +        .args_type  = "chardev:s,control:-b,data:s",
> +        .params     = "chardev [-b] data",
> +        .help       = "Provide writing interface for CirMemCharDriver. Write"
> +                      "'data' to it. (Use -b for 'block' option)",
> +        .mhandler.cmd = hmp_memchar_write,

Honest question: is this (and memchr_read) really useful? Isn't
the console command alone good enough?

> +    },
> +
> +STEXI
> +@item memchar_write @var{chardev} [-b] @var{data}
> +@findex memchar_write
> +Provide writing interface for CirMemCharDriver. Write @var{data}
> +to cirmemchr char device. The size of the data write to the driver
> +should better be power of 2.

As this is a human interface, it makes sense to round-up automatically.
Actually, you don't even accept a size parameter :)

> +
> +@var{control} is option('block', 'drop') for read and write command
> +that specifies behavior when the queue is full/empty. By default is
> +'drop'. Note that the 'block' option is not supported now.
> +        -b for 'block' option. None for 'drop' option.
> +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 70bdec2..18ca61b 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -671,6 +671,25 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &errp);
>  }
>  
> +void hmp_memchar_write(Monitor *mon, const QDict *qdict)
> +{
> +    uint32_t size;
> +    const char *chardev = qdict_get_str(qdict, "chardev");
> +    const char *data = qdict_get_str(qdict, "data");
> +    enum DataFormat format;
> +    int con = qdict_get_try_bool(qdict, "block", 0);
> +    enum CongestionControl control;
> +    Error *errp = NULL;
> +
> +    size = strlen(data);
> +    format = DATA_FORMAT_UTF8;
> +    control = con ? CONGESTION_CONTROL_BLOCK : CONGESTION_CONTROL_DROP;
> +    qmp_memchar_write(chardev, size, data, true, format,
> +                      true, control, &errp);
> +
> +    hmp_handle_error(mon, &errp);
> +}
> +
>  static void hmp_cont_cb(void *opaque, int err)
>  {
>      if (!err) {
> diff --git a/hmp.h b/hmp.h
> index 71ea384..406ebb1 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -43,6 +43,7 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
>  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_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 f9dbdae..a908aa6 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -325,6 +325,75 @@
>  { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
>  
>  ##
> +# @DataFormat:

DataEncoding?

> +#
> +# An enumeration of data format write to or read from
> +# memchardev. The default value would be utf8.

This is generic enough, don't need to mention memchardev.

> +#
> +# @utf8: The data format is 'utf8'.
> +#
> +# @base64: The data format is 'base64'.
> +#
> +# Note: The data format start with 'utf8' and 'base64', will support
> +#       other data format as well.
> +#
> +# Since: 1.3
> +##
> +{ 'enum': 'DataFormat'
> +  'data': [ 'utf8', 'base64' ] }
> +
> +##
> +# @CongestionControl
> +#
> +# An enumeration of options for the read and write command that
> +# specifies behavior when the queue is full/empty. The default
> +# option would be drop.
> +#
> +# @drop: Would result in reads returning empty strings and writes
> +#        dropping queued data.
> +#
> +# @block: Would make the session block until data was available
> +#         or the queue had space available.
> +#
> +# Note: 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.

This is not good, as an app would have to try and error 'block' to see
if it's supported.

My suggestion is to drop all CongestionControl bits and assume a dropping
behavior for this command. Later, when we add async support to QMP we can
add an async version of this command.

> +#
> +# Since: 1.3
> +##
> +{'enum': 'CongestionControl'
> + 'data': [ 'drop', 'block' ] }
> +
> +##
> +# @memchar-write:
> +#
> +# Provide writing interface for memchardev. Write data to memchar
> +# char device.
> +#
> +# @chardev: the name of the memchar char device.

I wonder if "device" is better than "chardev".

> +#
> +# @size: the size to write in bytes. Should be power of 2.
> +#
> +# @data: the source data write to memchar.
> +#
> +# @format: #optional the format of the data write to memchardev, by
> +#          default is 'utf8'.
> +#
> +# @control: #optional options for read and write command that specifies
> +#           behavior when the queue is full/empty.
> +#
> +# Returns: Nothing on success
> +#          If @chardev is not a valid memchr device, DeviceNotFound
> +#          If an I/O error occurs while writing, IOError

Please, drop the IOError line as this error doesn't exist anymore. The
DeviceNotFound one is fine.

> +#
> +# Since: 1.3
> +##
> +{ 'command': 'memchar-write',
> +  'data': {'chardev': 'str', 'size': 'int', 'data': 'str',
> +           '*format': 'DataFormat',
> +           '*control': 'CongestionControl'} }
> +
> +##
>  # @CommandInfo:
>  #
>  # Information about a QMP command
> diff --git a/qemu-char.c b/qemu-char.c
> index 381bf60..133d282 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2690,6 +2690,52 @@ static CharDriverState *qemu_chr_open_cirmemchr(QemuOpts *opts)
>      return chr;
>  }
>  
> +void qmp_memchar_write(const char *chardev, int64_t size,
> +                       const char *data, bool has_format,
> +                       enum DataFormat format, bool has_control,
> +                       enum CongestionControl control,
> +                       Error **errp)
> +{
> +    CharDriverState *chr;
> +    guchar *write_data;
> +    int ret;
> +    gsize write_count;
> +
> +    chr = qemu_chr_find(chardev);
> +
> +    if (!chr) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, chardev);
> +        return;
> +    }
> +
> +    /* XXX: For the sync command as 'block', waiting for the qmp
> + *      * to support necessary feature. Now just act as 'drop'. */
> +    if (cirmem_chr_is_full(chr)) {
> +        if (has_control && (control == CONGESTION_CONTROL_BLOCK)) {
> +            error_setg(errp, "Failed to write to memchr %s", chardev);
> +            return;
> +        } else {
> +            error_setg(errp, "Failed to write to memchr %s", chardev);
> +            return;
> +        }
> +    }

As I said above, I think you should drop all this stuff.

> +
> +    write_count = (gsize)size;
> +
> +    if (has_format && (format == DATA_FORMAT_BASE64)) {
> +        write_data = g_base64_decode(data, &write_count);
> +    } else {
> +        write_data = (uint8_t *)data;
> +    }
> +
> +    ret = cirmem_chr_write(chr, write_data, write_count);

What if chr is not a memory device?

> +
> +    if (ret < 0) {
> +        error_setg(errp, "Failed to write to memchr %s", chardev);
> +        return;
> +    }

Would be nice to return -errno codes from cirmem_chr_write() and use
error_setg_errno() (not in tree yet). Considering we're allowed to return
-errno, of course.

> +}
> +
>  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 2f8477e..502ed57 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -466,6 +466,46 @@ Note: inject-nmi fails when the guest doesn't support injecting.
>  EQMP
>  
>      {
> +        .name       = "memchar-write",
> +        .args_type  = "chardev:s,size:i,data:s,format:s?,control:s?",
> +        .help       = "write size of data to memchar chardev",
> +        .mhandler.cmd_new = qmp_marshal_input_memchar_write,
> +    },
> +
> +SQMP
> +memchar-write
> +-------------
> +
> +Provide writing interface for memchardev. Write data to memchar
> +char device.
> +
> +Arguments:
> +
> +- "chardev": the name of the char device, must be unique (json-string)
> +- "size": the memory size, in bytes, should be power of 2 (json-int)
> +- "data": the source data writed to memchar (json-string)
> +- "format": the data format write to memchardev, default is
> +            utf8. (json-string, optional)
> +          - Possible values: "utf8", "base64"
> +
> +- "control": options for read and write command that specifies
> +             behavior when the queue is full/empty, default is
> +             drop. (json-string, optional)
> +          - Possible values: "drop", "block"
> +
> +Example:
> +
> +-> { "execute": "memchar-write",
> +                "arguments": { "chardev": foo,
> +                               "size": 8,
> +                               "data": "abcdefgh",
> +                               "format": "utf8",
> +                               "control": "drop" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "xen-save-devices-state",
>          .args_type  = "filename:F",
>      .mhandler.cmd_new = qmp_marshal_input_xen_save_devices_state,