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

Re: [libvirt] [PATCH 04/10] Qemu arbitrary command-line arguments.



On Wed, Apr 21, 2010 at 12:01:18PM -0400, Chris Lalancette wrote:
> Implement the qemu hooks for XML namespace data.  This
> allows us to specify a qemu XML namespace, and then
> specify:
> 
> <qemu:commandline>
>  <qemu:arg>arg</qemu:arg>
>  <qemu:env name='name' value='value'/>
> </qemu:commandline>

Now I see it, it looks a little odd to use the element content
for qemu:arg, while having an element attributes for qemu:env.
Since changing qemu:env  to use content isn't practical, I
think we could do this instead

  <qemu:arg value='blah'/>

> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index b2820f0..ab0a4a4 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -158,6 +158,17 @@ struct qemud_driver {
>  typedef struct _qemuDomainPCIAddressSet qemuDomainPCIAddressSet;
>  typedef qemuDomainPCIAddressSet *qemuDomainPCIAddressSetPtr;
>  
> +typedef struct _qemuDomainCmdlineDef qemuDomainCmdlineDef;
> +typedef qemuDomainCmdlineDef *qemuDomainCmdlineDefPtr;
> +struct _qemuDomainCmdlineDef {
> +    unsigned int num_extra;
> +    char **extra;

Lets call these 'num_args' and 'args' instead.

> +
> +    unsigned int num_env;
> +    char **env_name;
> +    char **env_value;
> +};
> +
>  /* Port numbers used for KVM migration. */
>  # define QEMUD_MIGRATION_FIRST_PORT 49152
>  # define QEMUD_MIGRATION_NUM_PORTS 64
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 5f4adfd..0b297a7 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -47,6 +47,8 @@
>  #include <sys/ioctl.h>
>  #include <sys/un.h>
>  
> +#include <libxml/xpathInternals.h>
> +
>  #ifdef __linux__
>  # include <sys/vfs.h>
>  # ifndef NFS_SUPER_MAGIC
> @@ -88,6 +90,9 @@
>  
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  
> +#define QEMU_NAMESPACE_HREF "http://libvirt.org/schemas/domain/qemu/1.0";
> +
> +
>  /* Only 1 job is allowed at any time
>   * A job includes *all* monitor commands, even those just querying
>   * information, not merely actions */
> @@ -526,6 +531,144 @@ static void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, virD
>      }
>  }
>  
> +static void qemuDomainDefNamespaceFree(void *nsdata)
> +{
> +    qemuDomainCmdlineDefPtr cmd = nsdata;
> +    int i;
> +
> +    if (!cmd)
> +        return;
> +
> +    for (i = 0; i < cmd->num_extra; i++)
> +        VIR_FREE(cmd->extra[i]);
> +    for (i = 0; i < cmd->num_env; i++) {
> +        VIR_FREE(cmd->env_name[i]);
> +        VIR_FREE(cmd->env_value[i]);
> +    }
> +    VIR_FREE(cmd->extra);
> +    VIR_FREE(cmd->env_name);
> +    VIR_FREE(cmd->env_value);
> +    VIR_FREE(cmd);
> +}
> +
> +static int qemuDomainDefNamespaceParse(xmlDocPtr xml,
> +                                       xmlNodePtr root,
> +                                       xmlXPathContextPtr ctxt,
> +                                       void **data)
> +{
> +    qemuDomainCmdlineDefPtr cmd = NULL;
> +    xmlNsPtr ns;
> +    xmlNodePtr *nodes = NULL;
> +    int n, i;
> +    xmlNodePtr oldnode;
> +
> +    ns = xmlSearchNs(xml, root, BAD_CAST "qemu");
> +    if (!ns)
> +        /* this is fine; it just means there was no qemu namespace listed */
> +        return 0;
> +
> +    if (STRNEQ((const char *)ns->href, QEMU_NAMESPACE_HREF)) {
> +        qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                        _("Found namespace '%s' doesn't match expected '%s'"),
> +                        ns->href, QEMU_NAMESPACE_HREF);
> +        return -1;
> +    }
> +
> +    if (xmlXPathRegisterNs(ctxt, ns->prefix, ns->href) < 0) {
> +        qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                        _("Failed to register xml namespace '%s'"), ns->href);
> +        return -1;
> +    }
> +
> +    if (VIR_ALLOC(cmd) < 0) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +
> +    /* first handle the extra command-line arguments */
> +    n = virXPathNodeSet("./qemu:commandline/qemu:arg", ctxt, &nodes);
> +    if (n < 0)
> +        /* virXPathNodeSet already set the error */
> +        goto error;
> +
> +    if (n && VIR_ALLOC_N(cmd->extra, n) < 0) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +
> +    for (i = 0; i < n; i++) {
> +        oldnode = ctxt->node;
> +        ctxt->node = nodes[i];
> +        cmd->extra[cmd->num_extra] = virXPathString("string(.)", ctxt);
> +        if (cmd->extra[cmd->num_extra] == NULL)
> +            goto error;
> +        cmd->num_extra++;
> +        ctxt->node = oldnode;
> +    }
> +
> +    /* now handle the extra environment variables */
> +    n = virXPathNodeSet("./qemu:commandline/qemu:env", ctxt, &nodes);
> +    if (n < 0)
> +        /* virXPathNodeSet already set the error */
> +        goto error;
> +
> +    if (n && VIR_ALLOC_N(cmd->env_name, n) < 0) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +
> +    if (n && VIR_ALLOC_N(cmd->env_value, n) < 0) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +
> +    for (i = 0; i < n; i++) {
> +        oldnode = ctxt->node;
> +        ctxt->node = nodes[i];
> +        cmd->env_name[cmd->num_env] = virXPathString("string(./@name)", ctxt);
> +        if (cmd->env_name[cmd->num_env] == NULL)
> +            goto error;
> +        cmd->env_value[cmd->num_env] = virXPathString("string(./@value)", ctxt);
> +        /* a NULL value for command is allowed, since it might be empty */
> +        cmd->num_env++;
> +        ctxt->node = oldnode;
> +    }

Using xpath for getting immediate attributes is somewhat overkill. You can
just avoid touching ctxt at all, and use  

  virXMLPropString(nodes[i], "name");
  virXMLPropString(nodes[i], "value");

> +static int qemuDomainDefNamespaceFormatXML(virBufferPtr buf,
> +                                           void *nsdata)
> +{
> +    qemuDomainCmdlineDefPtr cmd = nsdata;
> +    int i;
> +
> +    if (cmd->num_extra || cmd->num_env)
> +        virBufferAddLit(buf, "  <qemu:commandline>\n");
> +    for (i = 0; i < cmd->num_extra; i++)
> +        virBufferVSprintf(buf, "    <qemu:arg>%s</qemu:arg>\n", cmd->extra[i]);
> +    for (i = 0; i < cmd->num_env; i++) {
> +        virBufferVSprintf(buf, "    <qemu:env name='%s'", cmd->env_name[i]);
> +        if (cmd->env_value[i])
> +            virBufferVSprintf(buf, " value='%s'", cmd->env_value[i]);
> +        virBufferAddLit(buf, "/>\n");
> +    }

Should use escaping for all the attributes here

> +    if (cmd->num_extra || cmd->num_env)
> +        virBufferAddLit(buf, "  </qemu:commandline>\n");
> +
> +    return 0;
> +}
> +
> +static const char *qemuDomainDefNamespaceHref(void)
> +{
> +    return "xmlns:qemu='" QEMU_NAMESPACE_HREF "'";
> +}
>  
>  static int qemuCgroupControllerActive(struct qemud_driver *driver,
>                                        int controller)
> @@ -1316,6 +1459,14 @@ qemuCreateCapabilities(virCapsPtr oldcaps,
>      caps->privateDataXMLFormat = qemuDomainObjPrivateXMLFormat;
>      caps->privateDataXMLParse = qemuDomainObjPrivateXMLParse;
>  
> +    /* Domain Namespace XML parser hooks */
> +    if (VIR_ALLOC(caps->ns) < 0)
> +        goto no_memory;
> +
> +    caps->ns->parse = qemuDomainDefNamespaceParse;
> +    caps->ns->free = qemuDomainDefNamespaceFree;
> +    caps->ns->format = qemuDomainDefNamespaceFormatXML;
> +    caps->ns->href = qemuDomainDefNamespaceHref;

Could avoid needing to allocate this, by just statically 
declaring the callback table.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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