[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 4/8] qemu_monitor: Introduce qemuMonitorAttachCharDev



On Thu, Jun 06, 2013 at 14:30:01 +0200, Michal Privoznik wrote:
> The function being introduced is responsible for preparing and
> executing 'chardev-add' qemu monitor command. Moreover, in case
> of PTY chardev, the corresponding pty path is updated.
> ---
>  src/qemu/qemu_monitor.c      |  21 +++++
>  src/qemu/qemu_monitor.h      |   3 +
>  src/qemu/qemu_monitor_json.c | 189 +++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor_json.h |   3 +
>  4 files changed, 216 insertions(+)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index ad326c5..d11b97e 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3626,3 +3626,24 @@ int qemuMonitorGetTPMTypes(qemuMonitorPtr mon,
>  
>      return qemuMonitorJSONGetTPMTypes(mon, tpmtypes);
>  }
> +
> +int qemuMonitorAttachCharDev(qemuMonitorPtr mon,
> +                             const char *chrID,
> +                             virDomainChrSourceDefPtr chr)
> +{
> +    VIR_DEBUG("mon=%p chrID=%s chr=%p", mon, chrID, chr);
> +
> +    if (!mon) {
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> +                       _("monitor must not be NULL"));
> +        return -1;
> +    }
> +
> +    if (!mon->json) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("JSON monitor is required"));
> +        return -1;
> +    }
> +
> +    return qemuMonitorJSONAttachCharDev(mon, chrID, chr);
> +}
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index a607712..3e7b4cb 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -696,6 +696,9 @@ int qemuMonitorGetTPMModels(qemuMonitorPtr mon,
>  int qemuMonitorGetTPMTypes(qemuMonitorPtr mon,
>                             char ***tpmtypes);
>  
> +int qemuMonitorAttachCharDev(qemuMonitorPtr mon,
> +                             const char *chrID,
> +                             virDomainChrSourceDefPtr chr);
>  /**
>   * When running two dd process and using <> redirection, we need a
>   * shell that will not truncate files.  These two strings serve that
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index b87b8b5..0399963 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -4760,6 +4760,30 @@ error:
>      return NULL;
>  }
>  
> +static virJSONValuePtr
> +qemuMonitorJSONBuildUnixSocketAddress(const char *path)
> +{
> +    virJSONValuePtr addr = NULL;
> +    virJSONValuePtr data = NULL;
> +
> +    if (!(data = virJSONValueNewObject()) ||
> +        !(addr = virJSONValueNewObject()))
> +        goto error;
> +
> +    /* port is really expected as a string here by qemu */

That's a bit surprising when unix sockets have no ports :-)

> +    if (virJSONValueObjectAppendString(data, "path", path) < 0 ||
> +        virJSONValueObjectAppendString(addr, "type", "unix") < 0 ||
> +        virJSONValueObjectAppend(addr, "data", data) < 0)
> +        goto error;
> +
> +    return addr;
> +error:
> +    virReportOOMError();
> +    virJSONValueFree(data);
> +    virJSONValueFree(addr);
> +    return NULL;
> +}
> +
>  int
>  qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon,
>                                const char *host,
> @@ -4939,3 +4963,168 @@ int qemuMonitorJSONGetTPMTypes(qemuMonitorPtr mon,
>  {
>      return qemuMonitorJSONGetStringArray(mon, "query-tpm-types", tpmtypes);
>  }
> +
> +static virJSONValuePtr
> +qemuMonitorJSONAttachCharDevCommand(const char *chrID,
> +                                    const virDomainChrSourceDefPtr chr)
> +{
> +    virJSONValuePtr ret;
> +    virJSONValuePtr backend;
> +    virJSONValuePtr data = NULL;
> +    virJSONValuePtr addr = NULL;
> +    const char *backend_type;
> +    bool telnet;
> +
> +    if (!(backend = virJSONValueNewObject()) ||
> +        !(data = virJSONValueNewObject())) {
> +        goto no_memory;
> +    }
> +
> +    switch ((enum virDomainChrType) chr->type) {
> +    case VIR_DOMAIN_CHR_TYPE_NULL:
> +    case VIR_DOMAIN_CHR_TYPE_VC:
> +        backend_type = "null";

To avoid adding an empty data object, I'd add:

    virJSONValueFree(data);
    data = NULL;

> +        break;
> +
> +    case VIR_DOMAIN_CHR_TYPE_PTY:
> +        backend_type = "pty";

and here as well:

    virJSONValueFree(data);
    data = NULL;

> +        break;
> +
> +    case VIR_DOMAIN_CHR_TYPE_FILE:
> +        backend_type = "file";
> +        if (virJSONValueObjectAppendString(data, "out", chr->data.file.path) < 0)
> +            goto no_memory;
> +        break;
> +
> +    case VIR_DOMAIN_CHR_TYPE_DEV:
> +        backend_type = STRPREFIX(chrID, "parallel") ? "parallel" : "serial";
> +        if (virJSONValueObjectAppendString(data, "device",
> +                                           chr->data.file.path) < 0)
> +            goto no_memory;
> +        break;
> +
> +    case VIR_DOMAIN_CHR_TYPE_TCP:
> +        backend_type = "socket";
> +        addr = qemuMonitorJSONBuildInetSocketAddress(chr->data.tcp.host,
> +                                                     chr->data.tcp.service);
> +        if (!addr ||
> +            virJSONValueObjectAppend(data, "addr", addr) < 0)
> +            goto no_memory;
> +        addr = NULL;
> +
> +        telnet = chr->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET;
> +
> +        if (virJSONValueObjectAppendBoolean(data, "wait", false) < 0 ||
> +            virJSONValueObjectAppendBoolean(data, "telnet", telnet) < 0 ||
> +            virJSONValueObjectAppendBoolean(data, "server", chr->data.tcp.listen) < 0)
> +            goto no_memory;
> +        break;
> +
> +    case VIR_DOMAIN_CHR_TYPE_UDP:
> +        backend_type = "socket";
> +        addr = qemuMonitorJSONBuildInetSocketAddress(chr->data.udp.connectHost,
> +                                                     chr->data.udp.connectService);
> +        if (!addr ||
> +            virJSONValueObjectAppend(data, "addr", addr) < 0)
> +            goto no_memory;
> +        addr = NULL;
> +        break;
> +
> +    case VIR_DOMAIN_CHR_TYPE_UNIX:
> +        backend_type = "socket";
> +        addr = qemuMonitorJSONBuildUnixSocketAddress(chr->data.nix.path);
> +
> +        if (!addr ||
> +            virJSONValueObjectAppend(data, "addr", addr) < 0)
> +            goto no_memory;
> +        addr = NULL;
> +
> +        if (virJSONValueObjectAppendBoolean(data, "wait", false) < 0 ||
> +            virJSONValueObjectAppendBoolean(data, "server", chr->data.nix.listen) < 0)
> +            goto no_memory;
> +        break;
> +
> +    case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
> +    case VIR_DOMAIN_CHR_TYPE_PIPE:
> +    case VIR_DOMAIN_CHR_TYPE_STDIO:
> +    case VIR_DOMAIN_CHR_TYPE_LAST:
> +    default:

Remove the "default:" line.

> +        virReportError(VIR_ERR_OPERATION_FAILED,
> +                       _("Unsupported char device type '%d'"),
> +                       chr->type);
> +        goto error;
> +    }
> +
> +    if (virJSONValueObjectAppendString(backend, "type", backend_type) < 0 ||
> +        virJSONValueObjectAppend(backend, "data", data) < 0)

Only append the data object if it's non-NULL (after freeing it when it's
not required).

> +        goto no_memory;
> +    data = NULL;
> +
> +    if (!(ret = qemuMonitorJSONMakeCommand("chardev-add",
> +                                           "s:id", chrID,
> +                                           "a:backend", backend,
> +                                           NULL)))
> +        goto error;
> +
> +    return ret;
> +
> +no_memory:
> +    virReportOOMError();
> +error:
> +    virJSONValueFree(addr);
> +    virJSONValueFree(data);
> +    virJSONValueFree(backend);
> +    return NULL;
> +}
> +
> +static int
> +qemuMonitorJSONAttachCharDevInfo(virJSONValuePtr reply,
> +                                 virDomainChrSourceDefPtr chr)

The name of this function is quite confusing since it doesn't really
attach anything, it just parses the result of previous chardev-add
command. I think it could actually be inlined in
qemuMonitorJSONAttachCharDev to avoid the need for coming up with a
better name :-)

> +{
> +    virJSONValuePtr data;
> +    const char *path;
> +
> +    if (chr->type != VIR_DOMAIN_CHR_TYPE_PTY)
> +        return 0;
> +
> +    if (!(data = virJSONValueObjectGet(reply, "return"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("chardev-add reply was missing return data"));
> +        return -1;
> +    }
> +
> +    if (!(path = virJSONValueObjectGetString(data, "pty"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("chardev-add reply was missing pty path"));
> +        return -1;
> +    }
> +
> +    if (VIR_STRDUP(chr->data.file.path, path) < 0)
> +        return -1;
> +    return 0;
> +}
> +
> +int
> +qemuMonitorJSONAttachCharDev(qemuMonitorPtr mon,
> +                             const char *chrID,
> +                             virDomainChrSourceDefPtr chr)
> +{
> +    int ret = -1;
> +    virJSONValuePtr cmd;
> +    virJSONValuePtr reply = NULL;
> +
> +    if (!(cmd = qemuMonitorJSONAttachCharDevCommand(chrID, chr)))
> +        return ret;
> +
> +    ret = qemuMonitorJSONCommand(mon, cmd, &reply);
> +
> +    if (ret == 0)
> +        ret = qemuMonitorJSONCheckError(cmd, reply);
> +
> +    if (ret == 0)
> +        ret = qemuMonitorJSONAttachCharDevInfo(reply, chr);
> +
> +    virJSONValueFree(cmd);
> +    virJSONValueFree(reply);
> +    return ret;
> +}

ACK with the small issues addressed.

Jirka


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]