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

Re: [libvirt] [PATCH 1/3] bhyve: implement support for commandline args



  Ján Tomko wrote:

> On Fri, Jan 18, 2019 at 07:41:00PM +0400, Roman Bogorodskiy wrote:
> >Implement support for passing custom command line arguments
> >to bhyve using the 'bhyve:commandline' element:
> >
> >  <bhyve:commandline>
> >    <bhyve:arg value='-newarg'/>
> >  </bhyve:commandline>
> >
> > * Define virDomainXMLNamespace for the bhyve driver, which
> >   at this point supports only the 'commandline' element
> >   described above,
> > * Update command generation code to inject these command line
> >   arguments between driver-generated arguments and the vmname
> >   positional argument.
> >
> >Signed-off-by: Roman Bogorodskiy <bogorodskiy gmail com>
> >---
> > docs/schemas/domaincommon.rng                 |  17 +++
> > src/bhyve/bhyve_command.c                     |   9 ++
> > src/bhyve/bhyve_conf.c                        |  15 +++
> > src/bhyve/bhyve_conf.h                        |   9 ++
> > src/bhyve/bhyve_domain.c                      | 107 +++++++++++++++++-
> > src/bhyve/bhyve_domain.h                      |   1 +
> > .../bhyvexml2argv-commandline.args            |   9 ++
> > .../bhyvexml2argv-commandline.ldargs          |   3 +
> > .../bhyvexml2argv-commandline.xml             |  27 +++++
> > tests/bhyvexml2argvtest.c                     |   1 +
> > .../bhyvexml2xmlout-commandline.xml           |  37 ++++++
> > tests/bhyvexml2xmltest.c                      |   1 +
> > 12 files changed, 235 insertions(+), 1 deletion(-)
> > create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-commandline.args
> > create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-commandline.ldargs
> > create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-commandline.xml
> > create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-commandline.xml
> >
> >diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> >index aa50eac424..7672639cb6 100644
> >--- a/docs/schemas/domaincommon.rng
> >+++ b/docs/schemas/domaincommon.rng
> >@@ -81,6 +81,9 @@
> >         <optional>
> >           <ref name='launchSecurity'/>
> >         </optional>
> >+        <optional>
> >+          <ref name='bhyvecmdline'/>
> >+        </optional>
> >       </interleave>
> >     </element>
> >   </define>
> >@@ -6127,6 +6130,20 @@
> >     </element>
> >   </define>
> >
> >+  <!--
> >+       Optional hypervisor extensions in their own namespace:
> >+         Bhyve
> >+    -->
> >+  <define name="bhyvecmdline">
> >+    <element name="commandline" ns="http://libvirt.org/schemas/domain/bhyve/1.0";>
> >+      <zeroOrMore>
> >+        <element name="arg">
> >+          <attribute name='value'/>
> >+        </element>
> >+      </zeroOrMore>
> >+    </element>
> >+  </define>
> >+
> >   <!--
> >        Type library
> >     -->
> >diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> >index 84fda08943..a1ae2026a0 100644
> >--- a/src/bhyve/bhyve_command.c
> >+++ b/src/bhyve/bhyve_command.c
> >@@ -28,6 +28,7 @@
> > #include "bhyve_capabilities.h"
> > #include "bhyve_command.h"
> > #include "bhyve_domain.h"
> >+#include "bhyve_conf.h"
> > #include "bhyve_driver.h"
> > #include "datatypes.h"
> > #include "viralloc.h"
> >@@ -626,6 +627,14 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
> >     if (bhyveBuildConsoleArgStr(def, cmd) < 0)
> >         goto error;
> >
> >+    if (def->namespaceData) {
> >+        bhyveDomainCmdlineDefPtr bhyvecmd;
> >+
> >+        bhyvecmd = def->namespaceData;
> >+        for (i = 0; i < bhyvecmd->num_args; i++)
> >+            virCommandAddArg(cmd, bhyvecmd->args[i]);
> >+    }
> >+
> >     virCommandAddArg(cmd, def->name);
> >
> >     return cmd;
> >diff --git a/src/bhyve/bhyve_conf.c b/src/bhyve/bhyve_conf.c
> >index 60baa2e848..75709801c7 100644
> >--- a/src/bhyve/bhyve_conf.c
> >+++ b/src/bhyve/bhyve_conf.c
> >@@ -25,6 +25,7 @@
> > #include "virlog.h"
> > #include "virstring.h"
> > #include "bhyve_conf.h"
> >+#include "bhyve_domain.h"
> > #include "configmake.h"
> >
> > #define VIR_FROM_THIS VIR_FROM_BHYVE
> >@@ -107,3 +108,17 @@ virBhyveDriverConfigDispose(void *obj)
> >
> >     VIR_FREE(cfg->firmwareDir);
> > }
> >+
> >+void bhyveDomainCmdlineDefFree(bhyveDomainCmdlineDefPtr def)
> >+{
> >+    size_t i;
> >+
> >+    if (!def)
> >+        return;
> >+
> >+    for (i = 0; i < def->num_args; i++)
> >+        VIR_FREE(def->args[i]);
> >+
> >+    VIR_FREE(def->args);
> >+    VIR_FREE(def);
> >+}
> >diff --git a/src/bhyve/bhyve_conf.h b/src/bhyve/bhyve_conf.h
> >index 8da39fde7a..eb4a2e0fb8 100644
> >--- a/src/bhyve/bhyve_conf.h
> >+++ b/src/bhyve/bhyve_conf.h
> >@@ -29,4 +29,13 @@ virBhyveDriverConfigPtr virBhyveDriverGetConfig(bhyveConnPtr driver);
> > int virBhyveLoadDriverConfig(virBhyveDriverConfigPtr cfg,
> >                              const char *filename);
> >
> >+typedef struct _bhyveDomainCmdlineDef bhyveDomainCmdlineDef;
> >+typedef bhyveDomainCmdlineDef *bhyveDomainCmdlineDefPtr;
> >+struct _bhyveDomainCmdlineDef {
> >+    size_t num_args;
> >+    char **args;
> >+};
> >+
> >+void bhyveDomainCmdlineDefFree(bhyveDomainCmdlineDefPtr def);
> >+
> > #endif /* LIBVIRT_BHYVE_CONF_H */
> >diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
> >index e54af75f4d..554188ebeb 100644
> >--- a/src/bhyve/bhyve_domain.c
> >+++ b/src/bhyve/bhyve_domain.c
> >@@ -20,16 +20,21 @@
> >
> > #include <config.h>
> >
> >+#include "bhyve_conf.h"
> > #include "bhyve_device.h"
> > #include "bhyve_domain.h"
> > #include "bhyve_capabilities.h"
> > #include "viralloc.h"
> > #include "virlog.h"
> >
> >+#include <libxml/xpathInternals.h>
> >+
> > #define VIR_FROM_THIS VIR_FROM_BHYVE
> >
> > VIR_LOG_INIT("bhyve.bhyve_domain");
> >
> >+#define BHYVE_NAMESPACE_HREF "http://libvirt.org/schemas/domain/bhyve/1.0";
> >+
> > static void *
> > bhyveDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED)
> > {
> >@@ -157,7 +162,8 @@ virBhyveDriverCreateXMLConf(bhyveConnPtr driver)
> >     virBhyveDriverDomainDefParserConfig.priv = driver;
> >     return virDomainXMLOptionNew(&virBhyveDriverDomainDefParserConfig,
> >                                  &virBhyveDriverPrivateDataCallbacks,
> >-                                 NULL, NULL, NULL);
> >+                                 &virBhyveDriverDomainXMLNamespace,
> >+                                 NULL, NULL);
> > }
> >
> > virDomainDefParserConfig virBhyveDriverDomainDefParserConfig = {
> >@@ -165,3 +171,102 @@ virDomainDefParserConfig virBhyveDriverDomainDefParserConfig = {
> >     .domainPostParseCallback = bhyveDomainDefPostParse,
> >     .assignAddressesCallback = bhyveDomainDefAssignAddresses,
> > };
> >+
> >+static void
> >+bhyveDomainDefNamespaceFree(void *nsdata)
> >+{
> >+    bhyveDomainCmdlineDefPtr cmd = nsdata;
> >+
> >+    bhyveDomainCmdlineDefFree(cmd);
> >+}
> >+
> >+static int
> >+bhyveDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED,
> >+                             xmlNodePtr root ATTRIBUTE_UNUSED,
> >+                             xmlXPathContextPtr ctxt,
> >+                             void **data)
> >+{
> >+    bhyveDomainCmdlineDefPtr cmd = NULL;
> >+    bool uses_bhyve_ns = false;
> >+    xmlNodePtr *nodes = NULL;
> >+    int n;
> >+    size_t i;
> >+
> >+    if (xmlXPathRegisterNs(ctxt, BAD_CAST "bhyve", BAD_CAST BHYVE_NAMESPACE_HREF) < 0) {
> >+        virReportError(VIR_ERR_INTERNAL_ERROR,
> >+                       _("Failed to register xml namespace '%s'"),
> >+                       BHYVE_NAMESPACE_HREF);
> >+        return -1;
> >+    }
> >+
> >+    if (VIR_ALLOC(cmd) < 0)
> >+        return -1;
> >+
> >+    n = virXPathNodeSet("./bhyve:commandline/bhyve:arg", ctxt, &nodes);
> >+    if (n < 0)
> >+        goto error;
> >+    uses_bhyve_ns = n > 0;
> >+
> 
> This bool and the whole logic of this function looks overly complicated.

Yeah, I rewrote it without it as John suggested.

BTW, I used qemuDomainDefNamespaceParse() as a reference, so probably
it may be simplified as well.

> >+    if (n && VIR_ALLOC_N(cmd->args, n) < 0)
> >+        goto error;
> >+
> >+    for (i = 0; i < n; i++) {
> >+        cmd->args[cmd->num_args] = virXMLPropString(nodes[i], "value");
> >+        if (cmd->args[cmd->num_args] == NULL) {
> >+            virReportError(VIR_ERR_INTERNAL_ERROR,
> >+                           "%s", _("No bhyve command-line argument specified"));
> >+            goto error;
> >+        }
> >+        cmd->num_args++;
> >+    }
> >+
> >+    VIR_FREE(nodes);
> >+
> >+    if (uses_bhyve_ns)
> >+        *data = cmd;
> >+    else
> >+        VIR_FREE(cmd);
> 
> Strange to see a different free function here, but it looks there's no
> wayt to get here with anything but bare 'cmd' allocated'
> 
> >+
> >+    return 0;
> >+
> >+ error:
> >+    VIR_FREE(nodes);
> >+    bhyveDomainDefNamespaceFree(cmd);
> >+    return -1;
> >+}
> >+
> >+static int
> >+bhyveDomainDefNamespaceFormatXML(virBufferPtr buf ATTRIBUTE_UNUSED,
> 
> buf is used here

Indeed.

> >+                                void *nsdata)
> 
> Indentation is off

Indeed. As it's already pushed, I'll fix it in a follow-up series. I'll
have to send a follow-up series anyway to updates docs and add a warning
as Daniel suggested.

> Jano



Roman Bogorodskiy

Attachment: signature.asc
Description: PGP signature


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