1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] Qemu driver: Support network-backed pflash disks.

Prerna Saxena posted 1 patch 5 years, 12 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180420085902.17661-2-saxenap.ltc@gmail.com
Test syntax-check failed
docs/schemas/domaincommon.rng   | 108 +++++++++++++++++++----
src/conf/domain_conf.c          | 188 ++++++++++++++++++++++++++++++++++++----
src/conf/domain_conf.h          |   7 +-
src/qemu/qemu_cgroup.c          |  13 ++-
src/qemu/qemu_command.c         |  21 +++--
src/qemu/qemu_domain.c          |  16 ++--
src/qemu/qemu_driver.c          |   7 +-
src/qemu/qemu_parse_command.c   |  30 ++++++-
src/qemu/qemu_process.c         |  33 ++++---
src/security/security_dac.c     |   6 +-
src/security/security_selinux.c |   6 +-
11 files changed, 361 insertions(+), 74 deletions(-)
[libvirt] [PATCH] Qemu driver: Support network-backed pflash disks.
Posted by Prerna Saxena 5 years, 12 months ago 
So far libvirt domain XML only allows local filepaths that can be
used to specify a loader element or its matching NVRAM disk.
Given that Vms may themselves move across hypervisor hosts, it should be
possible to allocate loaders/NVRAM disks on network storage for
uninterrupted access.

Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
---
 docs/schemas/domaincommon.rng   | 108 +++++++++++++++++++----
 src/conf/domain_conf.c          | 188 ++++++++++++++++++++++++++++++++++++----
 src/conf/domain_conf.h          |   7 +-
 src/qemu/qemu_cgroup.c          |  13 ++-
 src/qemu/qemu_command.c         |  21 +++--
 src/qemu/qemu_domain.c          |  16 ++--
 src/qemu/qemu_driver.c          |   7 +-
 src/qemu/qemu_parse_command.c   |  30 ++++++-
 src/qemu/qemu_process.c         |  33 ++++---
 src/security/security_dac.c     |   6 +-
 src/security/security_selinux.c |   6 +-
 11 files changed, 361 insertions(+), 74 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4cab55f..32db395 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -276,7 +276,42 @@
                 </choice>
               </attribute>
             </optional>
-            <ref name="absFilePath"/>
+            <optional>
+              <attribute name="backing">
+                <choice>
+                  <value>file</value>
+                  <value>network</value>
+                </choice>
+              </attribute>
+            </optional>
+            <optional>
+              <choice>
+                 <group>
+                  <ref name="absFilePath"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceFileElement"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolNBD"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolGluster"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolRBD"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolISCSI"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolHTTP"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolSimple"/>
+                 </group>
+              </choice>
+            </optional>
           </element>
         </optional>
         <optional>
@@ -287,7 +322,40 @@
               </attribute>
             </optional>
             <optional>
-              <ref name="absFilePath"/>
+              <attribute name="backing">
+                <choice>
+                  <value>file</value>
+                  <value>network</value>
+                </choice>
+              </attribute>
+            </optional>
+            <optional>
+              <choice>
+                 <group>
+                  <ref name="absFilePath"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceFileElement"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolNBD"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolGluster"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolRBD"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolISCSI"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolHTTP"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolSimple"/>
+                 </group>
+              </choice>
             </optional>
           </element>
         </optional>
@@ -1494,25 +1562,29 @@
       </attribute>
     </optional>
     <optional>
-      <element name="source">
-        <optional>
-          <attribute name="file">
-            <ref name="absFilePath"/>
-          </attribute>
-        </optional>
-        <optional>
-          <ref name="storageStartupPolicy"/>
-        </optional>
-        <optional>
-          <ref name="encryption"/>
-        </optional>
-        <zeroOrMore>
-          <ref name='devSeclabel'/>
-        </zeroOrMore>
-      </element>
+      <ref name='diskSourceFileElement'/>
     </optional>
   </define>
 
+  <define name="diskSourceFileElement">
+    <element name="source">
+      <optional>
+        <attribute name="file">
+          <ref name="absFilePath"/>
+        </attribute>
+      </optional>
+      <optional>
+        <ref name="storageStartupPolicy"/>
+      </optional>
+      <optional>
+        <ref name="encryption"/>
+      </optional>
+      <zeroOrMore>
+        <ref name='devSeclabel'/>
+      </zeroOrMore>
+    </element>
+  </define>
+
   <define name="diskSourceBlock">
     <attribute name="type">
       <value>block</value>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 35666c1..c80f9d9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2883,8 +2883,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
     if (!loader)
         return;
 
-    VIR_FREE(loader->path);
-    VIR_FREE(loader->nvram);
+    virStorageSourceFree(loader->loader_src);
+    virStorageSourceFree(loader->nvram);
     VIR_FREE(loader->templt);
     VIR_FREE(loader);
 }
@@ -17961,17 +17961,59 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
 
 static int
 virDomainLoaderDefParseXML(xmlNodePtr node,
+                           xmlXPathContextPtr ctxt,
                            virDomainLoaderDefPtr loader)
 {
     int ret = -1;
     char *readonly_str = NULL;
     char *secure_str = NULL;
     char *type_str = NULL;
+    char *tmp = NULL;
+    xmlNodePtr cur;
+    xmlXPathContextPtr cur_ctxt = ctxt;
+
+    if (VIR_ALLOC(loader->loader_src)) {
+        goto cleanup;
+    }
+    loader->loader_src->type = VIR_STORAGE_TYPE_LAST;
 
     readonly_str = virXMLPropString(node, "readonly");
     secure_str = virXMLPropString(node, "secure");
     type_str = virXMLPropString(node, "type");
-    loader->path = (char *) xmlNodeGetContent(node);
+
+    if ((tmp = virXMLPropString(node, "backing")) &&
+        (loader->loader_src->type = virStorageTypeFromString(tmp)) <= 0) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("unknown loader type '%s'"), tmp);
+        goto cleanup;
+    }
+    VIR_FREE(tmp);
+
+    for (cur = node->children; cur != NULL; cur = cur->next) {
+        if (cur->type != XML_ELEMENT_NODE) {
+            continue;
+        }
+
+        if (virXMLNodeNameEqual(cur, "source")) {
+            if (virDomainStorageSourceParse(cur, cur_ctxt, loader->loader_src, 0) < 0) {
+                virReportError(VIR_ERR_XML_DETAIL,
+                               _("Error parsing Loader source element"));
+                goto cleanup;
+            }
+            break;
+        }
+    }
+
+    /* Old-style absolute path found ? */
+    if (loader->loader_src->type == VIR_STORAGE_TYPE_LAST) {
+        if (!(loader->loader_src->path = (char *) xmlNodeGetContent(node))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("missing loader source"));
+            goto cleanup;
+        } else {
+            loader->loader_src->type = VIR_STORAGE_TYPE_FILE;
+        }
+    }
 
     if (readonly_str &&
         (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) {
@@ -17998,13 +18040,78 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
     }
 
     ret = 0;
- cleanup:
+    goto exit;
+cleanup:
+    if (loader->loader_src)
+      VIR_FREE(loader->loader_src);
+exit:
     VIR_FREE(readonly_str);
     VIR_FREE(secure_str);
     VIR_FREE(type_str);
+
     return ret;
 }
 
+static int
+virDomainLoaderNvramDefParseXML(xmlNodePtr node,
+                           xmlXPathContextPtr ctxt,
+                           virDomainLoaderDefPtr loader)
+{
+    int ret = -1;
+    char *tmp = NULL;
+    xmlNodePtr cur;
+
+    if (VIR_ALLOC(loader->nvram)) {
+        goto cleanup;
+    }
+
+    loader->nvram->type = VIR_STORAGE_TYPE_LAST;
+
+    if ((tmp = virXMLPropString(node, "backing")) &&
+        (loader->nvram->type = virStorageTypeFromString(tmp)) <= 0) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("unknown nvram type '%s'"), tmp);
+        goto cleanup;
+    }
+    VIR_FREE(tmp);
+
+    for (cur = node->children; cur != NULL; cur = cur->next) {
+        if (cur->type != XML_ELEMENT_NODE) {
+            continue;
+        }
+
+        if (virXMLNodeNameEqual(cur, "template")) {
+            loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
+            continue;
+        }
+
+        if (virXMLNodeNameEqual(cur, "source")) {
+            if (virDomainStorageSourceParse(cur, ctxt, loader->nvram, 0) < 0) {
+                virReportError(VIR_ERR_XML_DETAIL,
+                               _("Error parsing nvram source element"));
+                goto cleanup;
+            }
+            ret = 0;
+        }
+    }
+
+    if (loader->nvram->type == VIR_STORAGE_TYPE_LAST) {
+        if (!(loader->nvram->path = (char *) xmlNodeGetContent(node))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("missing nvram source"));
+            goto cleanup;
+        } else {
+            loader->nvram->type = VIR_STORAGE_TYPE_FILE;
+            ret = 0;
+        }
+    }
+    return ret;
+
+cleanup:
+    if (loader->nvram)
+      VIR_FREE(loader->nvram);
+    return ret;
+}
 
 static virBitmapPtr
 virDomainSchedulerParse(xmlNodePtr node,
@@ -18397,11 +18504,15 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
             if (VIR_ALLOC(def->os.loader) < 0)
                 goto error;
 
-            if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0)
+            def->os.loader->loader_src = NULL;
+            def->os.loader->nvram = NULL;
+            if (virDomainLoaderDefParseXML(loader_node, ctxt, def->os.loader) < 0)
                 goto error;
 
-            def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt);
-            def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
+            if ((loader_node = virXPathNode("./os/nvram[1]", ctxt)) &&
+                (virDomainLoaderNvramDefParseXML(loader_node, ctxt,
+                                                 def->os.loader) < 0))
+                    goto error;
         }
     }
 
@@ -26070,11 +26181,19 @@ virDomainHugepagesFormat(virBufferPtr buf,
 
 static void
 virDomainLoaderDefFormat(virBufferPtr buf,
-                         virDomainLoaderDefPtr loader)
+                         virDomainLoaderDefPtr loader,
+                         unsigned int flags)
 {
     const char *readonly = virTristateBoolTypeToString(loader->readonly);
     const char *secure = virTristateBoolTypeToString(loader->secure);
     const char *type = virDomainLoaderTypeToString(loader->type);
+    const char *backing = NULL;
+
+    virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
+    virBuffer childBuf = VIR_BUFFER_INITIALIZER;
+
+    virBufferSetChildIndent(&childBuf, buf);
+
 
     virBufferAddLit(buf, "<loader");
 
@@ -26084,17 +26203,54 @@ virDomainLoaderDefFormat(virBufferPtr buf,
     if (loader->secure)
         virBufferAsprintf(buf, " secure='%s'", secure);
 
-    virBufferAsprintf(buf, " type='%s'>", type);
+    virBufferAsprintf(buf, " type='%s'", type);
+    if (loader->loader_src &&
+        loader->loader_src->type != VIR_STORAGE_TYPE_LAST) {
+        if (virDomainStorageSourceFormat(&attrBuf, &childBuf, loader->loader_src,
+                                     flags, 0) < 0)
+            goto cleanup;
+
+        backing = virStorageTypeToString(loader->loader_src->type);
+        virBufferAsprintf(buf, " backing='%s'>", backing);
+
+        if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0)
+            goto cleanup;
+    } else {
+        virBufferAddLit(buf, ">\n");
+    }
+
+    virBufferAddLit(buf, "</loader>\n");
 
-    virBufferEscapeString(buf, "%s</loader>\n", loader->path);
     if (loader->nvram || loader->templt) {
-        virBufferAddLit(buf, "<nvram");
-        virBufferEscapeString(buf, " template='%s'", loader->templt);
+        ignore_value(virBufferContentAndReset(&attrBuf));
+        ignore_value(virBufferContentAndReset(&childBuf));
+        virBufferSetChildIndent(&childBuf, buf);
+
         if (loader->nvram)
-            virBufferEscapeString(buf, ">%s</nvram>\n", loader->nvram);
-        else
-            virBufferAddLit(buf, "/>\n");
+            backing = virStorageTypeToString(loader->nvram->type);
+
+        virBufferAddLit(buf, "<nvram");
+        if (loader->templt) {
+            virBufferEscapeString(buf, " template='%s'", loader->templt);
+        }
+        if (loader->nvram &&
+            (virDomainStorageSourceFormat(&attrBuf, &childBuf,
+                                         loader->nvram, flags, 0) < 0)) {
+            virBufferAddLit(buf, ">\n</nvram>\n");
+            goto cleanup;
+        }
+
+        virBufferEscapeString(buf, " backing='%s'>", backing);
+        if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) {
+            virBufferAddLit(buf, "</nvram>\n");
+            goto cleanup;
+        }
+        virBufferAddLit(buf, "</nvram>\n");
     }
+cleanup:
+    virBufferFreeAndReset(&attrBuf);
+    virBufferFreeAndReset(&childBuf);
+    return;
 }
 
 static void
@@ -26757,7 +26913,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
         virBufferAsprintf(buf, "<initgroup>%s</initgroup>\n", def->os.initgroup);
 
     if (def->os.loader)
-        virDomainLoaderDefFormat(buf, def->os.loader);
+        virDomainLoaderDefFormat(buf, def->os.loader, flags);
     virBufferEscapeString(buf, "<kernel>%s</kernel>\n",
                           def->os.kernel);
     virBufferEscapeString(buf, "<initrd>%s</initrd>\n",
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 3c7eccb..50d5ac3 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1857,14 +1857,17 @@ typedef enum {
 
 VIR_ENUM_DECL(virDomainLoader)
 
+struct _virStorageSource;
+typedef struct _virStorageSource* virStorageSourcePtr;
+
 typedef struct _virDomainLoaderDef virDomainLoaderDef;
 typedef virDomainLoaderDef *virDomainLoaderDefPtr;
 struct _virDomainLoaderDef {
-    char *path;
+    virStorageSourcePtr loader_src;
     int readonly;   /* enum virTristateBool */
     virDomainLoader type;
     int secure;     /* enum virTristateBool */
-    char *nvram;    /* path to non-volatile RAM */
+    virStorageSourcePtr nvram;  /* path to non-voliatile RAM */
     char *templt;   /* user override of path to master nvram */
 };
 
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index d88eb78..aa5d071 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -580,16 +580,21 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm)
 static int
 qemuSetupFirmwareCgroup(virDomainObjPtr vm)
 {
+    virStorageSourcePtr src = NULL;
+
     if (!vm->def->os.loader)
         return 0;
 
-    if (vm->def->os.loader->path &&
-        qemuSetupImagePathCgroup(vm, vm->def->os.loader->path,
-                                 vm->def->os.loader->readonly == VIR_TRISTATE_BOOL_YES) < 0)
+    src = vm->def->os.loader->loader_src;
+    if (src->path &&
+        src->type == VIR_STORAGE_TYPE_FILE &&
+        qemuSetupImagePathCgroup(vm, src->path,
+                                 src->readonly == VIR_TRISTATE_BOOL_YES) < 0)
         return -1;
 
     if (vm->def->os.loader->nvram &&
-        qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram, false) < 0)
+        vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+        qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram->path, false) < 0)
         return -1;
 
     return 0;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 238c6ed..279a06c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9293,6 +9293,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
     virDomainLoaderDefPtr loader = def->os.loader;
     virBuffer buf = VIR_BUFFER_INITIALIZER;
     int unit = 0;
+    char *source = NULL;
 
     if (!loader)
         return;
@@ -9300,7 +9301,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
     switch ((virDomainLoader) loader->type) {
     case VIR_DOMAIN_LOADER_TYPE_ROM:
         virCommandAddArg(cmd, "-bios");
-        virCommandAddArg(cmd, loader->path);
+        virCommandAddArg(cmd, loader->loader_src->path);
         break;
 
     case VIR_DOMAIN_LOADER_TYPE_PFLASH:
@@ -9312,9 +9313,14 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
                                  NULL);
         }
 
+        if (qemuGetDriveSourceString(loader->loader_src, NULL, &source) < 0)
+            break;
+
         virBufferAddLit(&buf, "file=");
-        virQEMUBuildBufferEscapeComma(&buf, loader->path);
-        virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d", unit);
+        virQEMUBuildBufferEscapeComma(&buf, source);
+        free(source);
+        virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d",
+                          unit);
         unit++;
 
         if (loader->readonly) {
@@ -9327,9 +9333,14 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
 
         if (loader->nvram) {
             virBufferFreeAndReset(&buf);
+            if (qemuGetDriveSourceString(loader->nvram, NULL, &source) < 0)
+                break;
+
             virBufferAddLit(&buf, "file=");
-            virQEMUBuildBufferEscapeComma(&buf, loader->nvram);
-            virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d", unit);
+            virQEMUBuildBufferEscapeComma(&buf, source);
+            virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d",
+                              unit);
+            unit++;
 
             virCommandAddArg(cmd, "-drive");
             virCommandAddArgBuffer(cmd, &buf);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 21897cb..c1cb751 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3312,8 +3312,10 @@ qemuDomainDefPostParse(virDomainDefPtr def,
     if (def->os.loader &&
         def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
         def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON &&
-        !def->os.loader->nvram) {
-        if (virAsprintf(&def->os.loader->nvram, "%s/%s_VARS.fd",
+        def->os.loader->nvram &&
+        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+        !def->os.loader->nvram->path) {
+        if (virAsprintf(&def->os.loader->nvram->path, "%s/%s_VARS.fd",
                         cfg->nvramDir, def->name) < 0)
             goto cleanup;
     }
@@ -10477,19 +10479,21 @@ qemuDomainSetupLoader(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
 
     VIR_DEBUG("Setting up loader");
 
-    if (loader) {
+    if (loader && loader->loader_src &&
+        loader->loader_src->type == VIR_STORAGE_TYPE_FILE) {
         switch ((virDomainLoader) loader->type) {
         case VIR_DOMAIN_LOADER_TYPE_ROM:
-            if (qemuDomainCreateDevice(loader->path, data, false) < 0)
+            if (qemuDomainCreateDevice(loader->loader_src->path, data, false) < 0)
                 goto cleanup;
             break;
 
         case VIR_DOMAIN_LOADER_TYPE_PFLASH:
-            if (qemuDomainCreateDevice(loader->path, data, false) < 0)
+            if (qemuDomainCreateDevice(loader->loader_src->path, data, false) < 0)
                 goto cleanup;
 
             if (loader->nvram &&
-                qemuDomainCreateDevice(loader->nvram, data, false) < 0)
+                loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+                qemuDomainCreateDevice(loader->nvram->path, data, false) < 0)
                 goto cleanup;
             break;
 
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5673d9f..ce6339d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7542,12 +7542,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
 
     if (vm->def->os.loader &&
         vm->def->os.loader->nvram &&
-        virFileExists(vm->def->os.loader->nvram)) {
+        vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+        virFileExists(vm->def->os.loader->nvram->path)) {
         if ((flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
-            if (unlink(vm->def->os.loader->nvram) < 0) {
+            if (unlink(vm->def->os.loader->nvram->path) < 0) {
                 virReportSystemError(errno,
                                      _("failed to remove nvram: %s"),
-                                     vm->def->os.loader->nvram);
+                                     vm->def->os.loader->nvram->path);
                 goto endjob;
             }
         } else if (!(flags & VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)) {
diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
index 0ce9632..2a0b200 100644
--- a/src/qemu/qemu_parse_command.c
+++ b/src/qemu/qemu_parse_command.c
@@ -650,6 +650,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
     int idx = -1;
     int busid = -1;
     int unitid = -1;
+    bool is_firmware = false;
 
     if (qemuParseKeywords(val,
                           &keywords,
@@ -772,6 +773,9 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
                 def->bus = VIR_DOMAIN_DISK_BUS_VIRTIO;
             } else if (STREQ(values[i], "xen")) {
                 def->bus = VIR_DOMAIN_DISK_BUS_XEN;
+            } else if (STREQ(values[i], "pflash")) {
+                def->bus = VIR_DOMAIN_DISK_BUS_LAST;
+                is_firmware = true;
             } else if (STREQ(values[i], "sd")) {
                 def->bus = VIR_DOMAIN_DISK_BUS_SD;
             }
@@ -943,8 +947,25 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
         ignore_value(VIR_STRDUP(def->dst, "hda"));
     }
 
-    if (!def->dst)
-        goto error;
+    if (!def->dst) {
+        if (is_firmware && def->bus == VIR_DOMAIN_DISK_BUS_LAST) {
+            if (!dom->os.loader && (VIR_ALLOC(dom->os.loader) < 0))
+                goto error;
+            if (def->src->readonly) {
+                /* Loader spec */
+                dom->os.loader->loader_src = def->src;
+                dom->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH;
+            } else {
+                /* NVRAM Spec */
+                if (!dom->os.loader->nvram && (VIR_ALLOC(dom->os.loader->nvram) < 0))
+                    goto error;
+                dom->os.loader->nvram = def->src;
+            }
+        } else {
+            goto error;
+        }
+    }
+
     if (STREQ(def->dst, "xvda"))
         def->dst[3] = 'a' + idx;
     else
@@ -2215,8 +2236,11 @@ qemuParseCommandLine(virCapsPtr caps,
         } else if (STREQ(arg, "-bios")) {
             WANT_VALUE();
             if (VIR_ALLOC(def->os.loader) < 0 ||
-                VIR_STRDUP(def->os.loader->path, val) < 0)
+                VIR_ALLOC(def->os.loader->loader_src) < 0 ||
+                VIR_STRDUP(def->os.loader->loader_src->path, val) < 0)
                 goto error;
+            def->os.loader->loader_src->type = VIR_STORAGE_TYPE_FILE;
+            def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
         } else if (STREQ(arg, "-initrd")) {
             WANT_VALUE();
             if (VIR_STRDUP(def->os.initrd, val) < 0)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 6a5262a..725dd6e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3994,25 +3994,32 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
     const char *master_nvram_path;
     ssize_t r;
 
-    if (!loader || !loader->nvram || virFileExists(loader->nvram))
+    if (!loader || !loader->loader_src || !loader->nvram ||
+        loader->nvram->type == VIR_STORAGE_TYPE_NETWORK)
         return 0;
 
     master_nvram_path = loader->templt;
-    if (!loader->templt) {
+    /* Even if a template is not specified, we associate "known" EFI firmware
+     * to their NVRAM templates.
+     * Ofcourse this only applies to local firmware paths, as it is diffcult
+     * for libvirt to parse all network paths.
+     */
+    if (!loader->templt && loader->loader_src->type == VIR_STORAGE_TYPE_FILE) {
         size_t i;
         for (i = 0; i < cfg->nfirmwares; i++) {
-            if (STREQ(cfg->firmwares[i]->name, loader->path)) {
+            if (STREQ(cfg->firmwares[i]->name, loader->loader_src->path)) {
                 master_nvram_path = cfg->firmwares[i]->nvram;
                 break;
             }
         }
     }
 
-    if (!master_nvram_path) {
-        virReportError(VIR_ERR_OPERATION_FAILED,
-                       _("unable to find any master var store for "
-                         "loader: %s"), loader->path);
-        goto cleanup;
+    if (!master_nvram_path && loader->nvram) {
+        /* There is no template description, but an NVRAM spec
+         * has already been provided.
+         * Trust the client to have generated the right spec here
+         */
+        return 0;
     }
 
     if ((srcFD = virFileOpenAs(master_nvram_path, O_RDONLY,
@@ -4022,13 +4029,13 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
                              master_nvram_path);
         goto cleanup;
     }
-    if ((dstFD = virFileOpenAs(loader->nvram,
+    if ((dstFD = virFileOpenAs(loader->nvram->path,
                                O_WRONLY | O_CREAT | O_EXCL,
                                S_IRUSR | S_IWUSR,
                                cfg->user, cfg->group, 0)) < 0) {
         virReportSystemError(-dstFD,
                              _("Failed to create file '%s'"),
-                             loader->nvram);
+                             loader->nvram->path);
         goto cleanup;
     }
     created = true;
@@ -4046,7 +4053,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
         if (safewrite(dstFD, buf, r) < 0) {
             virReportSystemError(errno,
                                  _("Unable to write to file '%s'"),
-                                 loader->nvram);
+                                 loader->nvram->path);
             goto cleanup;
         }
     } while (r);
@@ -4060,7 +4067,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
     if (VIR_CLOSE(dstFD) < 0) {
         virReportSystemError(errno,
                              _("Unable to close file '%s'"),
-                             loader->nvram);
+                             loader->nvram->path);
         goto cleanup;
     }
 
@@ -4070,7 +4077,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
      * copy the file content. Roll back. */
     if (ret < 0) {
         if (created)
-            unlink(loader->nvram);
+            unlink(loader->nvram->path);
     }
 
     VIR_FORCE_CLOSE(srcFD);
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 663c8c9..fce4204 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1604,7 +1604,8 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
     }
 
     if (def->os.loader && def->os.loader->nvram &&
-        virSecurityDACRestoreFileLabel(priv, def->os.loader->nvram) < 0)
+        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+        virSecurityDACRestoreFileLabel(priv, def->os.loader->nvram->path) < 0)
         rc = -1;
 
     return rc;
@@ -1732,8 +1733,9 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr,
         return -1;
 
     if (def->os.loader && def->os.loader->nvram &&
+        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
         virSecurityDACSetOwnership(priv, NULL,
-                                   def->os.loader->nvram, user, group) < 0)
+                                   def->os.loader->nvram->path, user, group) < 0)
         return -1;
 
     if (def->os.kernel &&
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index c26cdac..396e7fc 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2459,7 +2459,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr,
         rc = -1;
 
     if (def->os.loader && def->os.loader->nvram &&
-        virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram) < 0)
+        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+        virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram->path) < 0)
         rc = -1;
 
     return rc;
@@ -2851,8 +2852,9 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr,
     /* This is different than kernel or initrd. The nvram store
      * is really a disk, qemu can read and write to it. */
     if (def->os.loader && def->os.loader->nvram &&
+        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
         secdef && secdef->imagelabel &&
-        virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram,
+        virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram->path,
                                      secdef->imagelabel) < 0)
         return -1;
 
-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Qemu driver: Support network-backed pflash disks.
Posted by John Ferlan 5 years, 11 months ago 

On 04/20/2018 04:59 AM, Prerna Saxena wrote:
> So far libvirt domain XML only allows local filepaths that can be
> used to specify a loader element or its matching NVRAM disk.
> Given that Vms may themselves move across hypervisor hosts, it should be
> possible to allocate loaders/NVRAM disks on network storage for
> uninterrupted access.
> 
> Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
> ---
>  docs/schemas/domaincommon.rng   | 108 +++++++++++++++++++----
>  src/conf/domain_conf.c          | 188 ++++++++++++++++++++++++++++++++++++----
>  src/conf/domain_conf.h          |   7 +-
>  src/qemu/qemu_cgroup.c          |  13 ++-
>  src/qemu/qemu_command.c         |  21 +++--
>  src/qemu/qemu_domain.c          |  16 ++--
>  src/qemu/qemu_driver.c          |   7 +-
>  src/qemu/qemu_parse_command.c   |  30 ++++++-
>  src/qemu/qemu_process.c         |  33 ++++---
>  src/security/security_dac.c     |   6 +-
>  src/security/security_selinux.c |   6 +-
>  11 files changed, 361 insertions(+), 74 deletions(-)
> 

I know on IRC you asked Peter or Michal to review (and they're CC'd
here), but before they get a chance next week some time - I'll give you
a quick look. You do understand Peter, Michal, myself, and other Red Hat
libvirt developers follow libvir-list anyway - so CC'ing any of us
doesn't do much since we filter into a mail folder anyway...

This will need a v2 anyway because the patch has too much going on and
needs to be split up more.

You need to break out the domain_conf, docs, etc. changes into one
patch. Much of what you put into the cover that describes the XML should
go into this patch's commit message. There should be xml2xml test
changes as well as adjustments to formatdomain.html.in to describe the
new options. The part of the cover that says automatically reformatting
to use the storage source cannot happen. There's precedence for that
when the <encryption> and <auth> moved *into* the storage source we
still have to accommodate for them outside. Internally, it can be placed
as expected, but when formatting, we have to format as we read. After
that, perhaps add the security changes in another, the cgroup in
another, and finally the qemu adjustments in the last.  Not even sure if
you need a capability as well.

Finally this doesn't even compile for me.  You removed @path from
_virDomainLoaderDef but neglected to adjust all consumers. I suggest
using cscope and egrep'ing on "os.*loader->path" as well as ->nvram
since you changed it's type.

I assume you've considered that network storage types need to deal with
authentication and encryption passphrases, right?  What about using a
srcpool storage source?

I'll briefly scan the rest.

> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 4cab55f..32db395 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -276,7 +276,42 @@
>                  </choice>
>                </attribute>
>              </optional>
> -            <ref name="absFilePath"/>
> +            <optional>
> +              <attribute name="backing">
> +                <choice>
> +                  <value>file</value>
> +                  <value>network</value>
> +                </choice>
> +              </attribute>
> +            </optional>
> +            <optional>
> +              <choice>
> +                 <group>
> +                  <ref name="absFilePath"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceFileElement"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolNBD"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolGluster"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolRBD"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolISCSI"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolHTTP"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolSimple"/>
> +                 </group>
> +              </choice>
> +            </optional>
>            </element>
>          </optional>
>          <optional>
> @@ -287,7 +322,40 @@
>                </attribute>
>              </optional>
>              <optional>
> -              <ref name="absFilePath"/>
> +              <attribute name="backing">
> +                <choice>
> +                  <value>file</value>
> +                  <value>network</value>
> +                </choice>
> +              </attribute>
> +            </optional>
> +            <optional>
> +              <choice>
> +                 <group>
> +                  <ref name="absFilePath"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceFileElement"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolNBD"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolGluster"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolRBD"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolISCSI"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolHTTP"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolSimple"/>
> +                 </group>
> +              </choice>
>              </optional>
>            </element>
>          </optional>
> @@ -1494,25 +1562,29 @@
>        </attribute>
>      </optional>
>      <optional>
> -      <element name="source">
> -        <optional>
> -          <attribute name="file">
> -            <ref name="absFilePath"/>
> -          </attribute>
> -        </optional>
> -        <optional>
> -          <ref name="storageStartupPolicy"/>
> -        </optional>
> -        <optional>
> -          <ref name="encryption"/>
> -        </optional>
> -        <zeroOrMore>
> -          <ref name='devSeclabel'/>
> -        </zeroOrMore>
> -      </element>
> +      <ref name='diskSourceFileElement'/>
>      </optional>
>    </define>
>  
> +  <define name="diskSourceFileElement">
> +    <element name="source">
> +      <optional>
> +        <attribute name="file">
> +          <ref name="absFilePath"/>
> +        </attribute>
> +      </optional>
> +      <optional>
> +        <ref name="storageStartupPolicy"/>
> +      </optional>
> +      <optional>
> +        <ref name="encryption"/>
> +      </optional>
> +      <zeroOrMore>
> +        <ref name='devSeclabel'/>
> +      </zeroOrMore>
> +    </element>
> +  </define>
> +
>    <define name="diskSourceBlock">
>      <attribute name="type">
>        <value>block</value>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 35666c1..c80f9d9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2883,8 +2883,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
>      if (!loader)
>          return;
>  
> -    VIR_FREE(loader->path);
> -    VIR_FREE(loader->nvram);
> +    virStorageSourceFree(loader->loader_src);

loader_src is redundant to loader isn't it?

> +    virStorageSourceFree(loader->nvram);
>      VIR_FREE(loader->templt);
>      VIR_FREE(loader);
>  }
> @@ -17961,17 +17961,59 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>  
>  static int
>  virDomainLoaderDefParseXML(xmlNodePtr node,
> +                           xmlXPathContextPtr ctxt,
>                             virDomainLoaderDefPtr loader)
>  {
>      int ret = -1;
>      char *readonly_str = NULL;
>      char *secure_str = NULL;
>      char *type_str = NULL;
> +    char *tmp = NULL;
> +    xmlNodePtr cur;
> +    xmlXPathContextPtr cur_ctxt = ctxt;
> +
> +    if (VIR_ALLOC(loader->loader_src)) {
> +        goto cleanup;
> +    }

syntax-check would choke here with the unnecessary { }

> +    loader->loader_src->type = VIR_STORAGE_TYPE_LAST;

ug, ah, no.

Consider my comment from above - you have either "path" or "source" and
deal with it from there. When you format you need to format as you read.

>  
>      readonly_str = virXMLPropString(node, "readonly");
>      secure_str = virXMLPropString(node, "secure");
>      type_str = virXMLPropString(node, "type");
> -    loader->path = (char *) xmlNodeGetContent(node);
> +
> +    if ((tmp = virXMLPropString(node, "backing")) &&
> +        (loader->loader_src->type = virStorageTypeFromString(tmp)) <= 0) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("unknown loader type '%s'"), tmp);
> +        goto cleanup;
> +    }
> +    VIR_FREE(tmp);
> +
> +    for (cur = node->children; cur != NULL; cur = cur->next) {
> +        if (cur->type != XML_ELEMENT_NODE) {
> +            continue;
> +        }
> +
> +        if (virXMLNodeNameEqual(cur, "source")) {
> +            if (virDomainStorageSourceParse(cur, cur_ctxt, loader->loader_src, 0) < 0) {
> +                virReportError(VIR_ERR_XML_DETAIL,
> +                               _("Error parsing Loader source element"));
> +                goto cleanup;
> +            }
> +            break;
> +        }
> +    }
> +
> +    /* Old-style absolute path found ? */
> +    if (loader->loader_src->type == VIR_STORAGE_TYPE_LAST) {
> +        if (!(loader->loader_src->path = (char *) xmlNodeGetContent(node))) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("missing loader source"));
> +            goto cleanup;
> +        } else {
> +            loader->loader_src->type = VIR_STORAGE_TYPE_FILE;
> +        }
> +    }

You need to find a different way.  If you find <source>, then parse it.
If you find <path> then you parse it instead.

>  
>      if (readonly_str &&
>          (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) {
> @@ -17998,13 +18040,78 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>      }
>  
>      ret = 0;
> - cleanup:
> +    goto exit;
> +cleanup:
> +    if (loader->loader_src)
> +      VIR_FREE(loader->loader_src);
> +exit:

This is not the way we handle this.

>      VIR_FREE(readonly_str);
>      VIR_FREE(secure_str);
>      VIR_FREE(type_str);
> +

extraneous

>      return ret;
>  }
>  

Two blank lines.

> +static int
> +virDomainLoaderNvramDefParseXML(xmlNodePtr node,
> +                           xmlXPathContextPtr ctxt,
> +                           virDomainLoaderDefPtr loader)
> +{
> +    int ret = -1;
> +    char *tmp = NULL;
> +    xmlNodePtr cur;
> +
> +    if (VIR_ALLOC(loader->nvram)) {
> +        goto cleanup;
> +    }

Again { }

Similar problem to before w/r/t the XML processing here.

> +
> +    loader->nvram->type = VIR_STORAGE_TYPE_LAST;
> +
> +    if ((tmp = virXMLPropString(node, "backing")) &&
> +        (loader->nvram->type = virStorageTypeFromString(tmp)) <= 0) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("unknown nvram type '%s'"), tmp);
> +        goto cleanup;
> +    }
> +    VIR_FREE(tmp);
> +
> +    for (cur = node->children; cur != NULL; cur = cur->next) {
> +        if (cur->type != XML_ELEMENT_NODE) {
> +            continue;
> +        }
> +
> +        if (virXMLNodeNameEqual(cur, "template")) {
> +            loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
> +            continue;
> +        }
> +
> +        if (virXMLNodeNameEqual(cur, "source")) {
> +            if (virDomainStorageSourceParse(cur, ctxt, loader->nvram, 0) < 0) {
> +                virReportError(VIR_ERR_XML_DETAIL,
> +                               _("Error parsing nvram source element"));
> +                goto cleanup;
> +            }
> +            ret = 0;
> +        }
> +    }
> +
> +    if (loader->nvram->type == VIR_STORAGE_TYPE_LAST) {
> +        if (!(loader->nvram->path = (char *) xmlNodeGetContent(node))) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("missing nvram source"));
> +            goto cleanup;
> +        } else {
> +            loader->nvram->type = VIR_STORAGE_TYPE_FILE;
> +            ret = 0;
> +        }
> +    }
> +    return ret;
> +
> +cleanup:
> +    if (loader->nvram)
> +      VIR_FREE(loader->nvram);
> +    return ret;
> +}
>  
>  static virBitmapPtr
>  virDomainSchedulerParse(xmlNodePtr node,
> @@ -18397,11 +18504,15 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
>              if (VIR_ALLOC(def->os.loader) < 0)
>                  goto error;
>  
> -            if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0)
> +            def->os.loader->loader_src = NULL;
> +            def->os.loader->nvram = NULL;
> +            if (virDomainLoaderDefParseXML(loader_node, ctxt, def->os.loader) < 0)
>                  goto error;
>  
> -            def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt);
> -            def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
> +            if ((loader_node = virXPathNode("./os/nvram[1]", ctxt)) &&
> +                (virDomainLoaderNvramDefParseXML(loader_node, ctxt,
> +                                                 def->os.loader) < 0))
> +                    goto error;

don't reuse "loader_node" for "nvram_node".

I think you need to separate loader patches from nvram patches too.
It'll perhaps make things easier to review again eventually.

>          }
>      }
>  
> @@ -26070,11 +26181,19 @@ virDomainHugepagesFormat(virBufferPtr buf,
>  
>  static void
>  virDomainLoaderDefFormat(virBufferPtr buf,
> -                         virDomainLoaderDefPtr loader)
> +                         virDomainLoaderDefPtr loader,
> +                         unsigned int flags)

As noted earlier reading in one format means we write out in the same
format. If someone wants to use the new format, then they can.

>  {
>      const char *readonly = virTristateBoolTypeToString(loader->readonly);
>      const char *secure = virTristateBoolTypeToString(loader->secure);
>      const char *type = virDomainLoaderTypeToString(loader->type);
> +    const char *backing = NULL;
> +
> +    virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
> +    virBuffer childBuf = VIR_BUFFER_INITIALIZER;
> +
> +    virBufferSetChildIndent(&childBuf, buf);
> +
>  
>      virBufferAddLit(buf, "<loader");
>  
> @@ -26084,17 +26203,54 @@ virDomainLoaderDefFormat(virBufferPtr buf,
>      if (loader->secure)
>          virBufferAsprintf(buf, " secure='%s'", secure);
>  
> -    virBufferAsprintf(buf, " type='%s'>", type);
> +    virBufferAsprintf(buf, " type='%s'", type);
> +    if (loader->loader_src &&
> +        loader->loader_src->type != VIR_STORAGE_TYPE_LAST) {
> +        if (virDomainStorageSourceFormat(&attrBuf, &childBuf, loader->loader_src,
> +                                     flags, 0) < 0)
> +            goto cleanup;
> +
> +        backing = virStorageTypeToString(loader->loader_src->type);
> +        virBufferAsprintf(buf, " backing='%s'>", backing);
> +
> +        if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0)
> +            goto cleanup;
> +    } else {
> +        virBufferAddLit(buf, ">\n");
> +    }
> +
> +    virBufferAddLit(buf, "</loader>\n");
>  
> -    virBufferEscapeString(buf, "%s</loader>\n", loader->path);
>      if (loader->nvram || loader->templt) {
> -        virBufferAddLit(buf, "<nvram");
> -        virBufferEscapeString(buf, " template='%s'", loader->templt);
> +        ignore_value(virBufferContentAndReset(&attrBuf));
> +        ignore_value(virBufferContentAndReset(&childBuf));
> +        virBufferSetChildIndent(&childBuf, buf);
> +
>          if (loader->nvram)
> -            virBufferEscapeString(buf, ">%s</nvram>\n", loader->nvram);
> -        else
> -            virBufferAddLit(buf, "/>\n");
> +            backing = virStorageTypeToString(loader->nvram->type);
> +
> +        virBufferAddLit(buf, "<nvram");
> +        if (loader->templt) {
> +            virBufferEscapeString(buf, " template='%s'", loader->templt);
> +        }
> +        if (loader->nvram &&
> +            (virDomainStorageSourceFormat(&attrBuf, &childBuf,
> +                                         loader->nvram, flags, 0) < 0)) {
> +            virBufferAddLit(buf, ">\n</nvram>\n");
> +            goto cleanup;
> +        }
> +
> +        virBufferEscapeString(buf, " backing='%s'>", backing);
> +        if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) {
> +            virBufferAddLit(buf, "</nvram>\n");
> +            goto cleanup;
> +        }
> +        virBufferAddLit(buf, "</nvram>\n");
>      }
> +cleanup:
> +    virBufferFreeAndReset(&attrBuf);
> +    virBufferFreeAndReset(&childBuf);
> +    return;
>  }
>  
>  static void
> @@ -26757,7 +26913,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>          virBufferAsprintf(buf, "<initgroup>%s</initgroup>\n", def->os.initgroup);
>  
>      if (def->os.loader)
> -        virDomainLoaderDefFormat(buf, def->os.loader);
> +        virDomainLoaderDefFormat(buf, def->os.loader, flags);
>      virBufferEscapeString(buf, "<kernel>%s</kernel>\n",
>                            def->os.kernel);
>      virBufferEscapeString(buf, "<initrd>%s</initrd>\n",
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 3c7eccb..50d5ac3 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1857,14 +1857,17 @@ typedef enum {
>  
>  VIR_ENUM_DECL(virDomainLoader)
>  
> +struct _virStorageSource;
> +typedef struct _virStorageSource* virStorageSourcePtr;
> +
>  typedef struct _virDomainLoaderDef virDomainLoaderDef;
>  typedef virDomainLoaderDef *virDomainLoaderDefPtr;
>  struct _virDomainLoaderDef {
> -    char *path;
> +    virStorageSourcePtr loader_src;

You'll probably need a way to booleanize which format was read. I forget
how things were done for encryption and auth changes that I made.

>      int readonly;   /* enum virTristateBool */
>      virDomainLoader type;
>      int secure;     /* enum virTristateBool */
> -    char *nvram;    /* path to non-volatile RAM */
> +    virStorageSourcePtr nvram;  /* path to non-voliatile RAM */
>      char *templt;   /* user override of path to master nvram */
>  };
>  
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index d88eb78..aa5d071 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -580,16 +580,21 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm)
>  static int
>  qemuSetupFirmwareCgroup(virDomainObjPtr vm)
>  {
> +    virStorageSourcePtr src = NULL;
> +
>      if (!vm->def->os.loader)
>          return 0;
>  
> -    if (vm->def->os.loader->path &&
> -        qemuSetupImagePathCgroup(vm, vm->def->os.loader->path,
> -                                 vm->def->os.loader->readonly == VIR_TRISTATE_BOOL_YES) < 0)
> +    src = vm->def->os.loader->loader_src;
> +    if (src->path &&
> +        src->type == VIR_STORAGE_TYPE_FILE &&
> +        qemuSetupImagePathCgroup(vm, src->path,
> +                                 src->readonly == VIR_TRISTATE_BOOL_YES) < 0)
>          return -1;
>  
>      if (vm->def->os.loader->nvram &&
> -        qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram, false) < 0)
> +        vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> +        qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram->path, false) < 0)
>          return -1;
>  
>      return 0;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 238c6ed..279a06c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9293,6 +9293,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
>      virDomainLoaderDefPtr loader = def->os.loader;
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      int unit = 0;
> +    char *source = NULL;
>  
>      if (!loader)
>          return;
> @@ -9300,7 +9301,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
>      switch ((virDomainLoader) loader->type) {
>      case VIR_DOMAIN_LOADER_TYPE_ROM:
>          virCommandAddArg(cmd, "-bios");
> -        virCommandAddArg(cmd, loader->path);
> +        virCommandAddArg(cmd, loader->loader_src->path);
>          break;
>  
>      case VIR_DOMAIN_LOADER_TYPE_PFLASH:
> @@ -9312,9 +9313,14 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
>                                   NULL);
>          }
>  
> +        if (qemuGetDriveSourceString(loader->loader_src, NULL, &source) < 0)
> +            break;
> +
>          virBufferAddLit(&buf, "file=");
> -        virQEMUBuildBufferEscapeComma(&buf, loader->path);
> -        virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d", unit);
> +        virQEMUBuildBufferEscapeComma(&buf, source);
> +        free(source);
> +        virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d",
> +                          unit);

What happens if there's authentication or encryption? is that not supported?

>          unit++;
>  
>          if (loader->readonly) {
> @@ -9327,9 +9333,14 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
>  
>          if (loader->nvram) {
>              virBufferFreeAndReset(&buf);
> +            if (qemuGetDriveSourceString(loader->nvram, NULL, &source) < 0)
> +                break;
> +
>              virBufferAddLit(&buf, "file=");
> -            virQEMUBuildBufferEscapeComma(&buf, loader->nvram);
> -            virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d", unit);
> +            virQEMUBuildBufferEscapeComma(&buf, source);
> +            virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d",
> +                              unit);
> +            unit++;
>  
>              virCommandAddArg(cmd, "-drive");
>              virCommandAddArgBuffer(cmd, &buf);
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 21897cb..c1cb751 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3312,8 +3312,10 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>      if (def->os.loader &&
>          def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
>          def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON &&
> -        !def->os.loader->nvram) {
> -        if (virAsprintf(&def->os.loader->nvram, "%s/%s_VARS.fd",
> +        def->os.loader->nvram &&
> +        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> +        !def->os.loader->nvram->path) {
> +        if (virAsprintf(&def->os.loader->nvram->path, "%s/%s_VARS.fd",
>                          cfg->nvramDir, def->name) < 0)
>              goto cleanup;
>      }
> @@ -10477,19 +10479,21 @@ qemuDomainSetupLoader(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
>  
>      VIR_DEBUG("Setting up loader");
>  
> -    if (loader) {
> +    if (loader && loader->loader_src &&
> +        loader->loader_src->type == VIR_STORAGE_TYPE_FILE) {
>          switch ((virDomainLoader) loader->type) {
>          case VIR_DOMAIN_LOADER_TYPE_ROM:
> -            if (qemuDomainCreateDevice(loader->path, data, false) < 0)
> +            if (qemuDomainCreateDevice(loader->loader_src->path, data, false) < 0)
>                  goto cleanup;
>              break;
>  
>          case VIR_DOMAIN_LOADER_TYPE_PFLASH:
> -            if (qemuDomainCreateDevice(loader->path, data, false) < 0)
> +            if (qemuDomainCreateDevice(loader->loader_src->path, data, false) < 0)
>                  goto cleanup;
>  
>              if (loader->nvram &&
> -                qemuDomainCreateDevice(loader->nvram, data, false) < 0)
> +                loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> +                qemuDomainCreateDevice(loader->nvram->path, data, false) < 0)
>                  goto cleanup;
>              break;
>  
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 5673d9f..ce6339d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7542,12 +7542,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
>  
>      if (vm->def->os.loader &&
>          vm->def->os.loader->nvram &&
> -        virFileExists(vm->def->os.loader->nvram)) {
> +        vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> +        virFileExists(vm->def->os.loader->nvram->path)) {
>          if ((flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
> -            if (unlink(vm->def->os.loader->nvram) < 0) {
> +            if (unlink(vm->def->os.loader->nvram->path) < 0) {
>                  virReportSystemError(errno,
>                                       _("failed to remove nvram: %s"),
> -                                     vm->def->os.loader->nvram);
> +                                     vm->def->os.loader->nvram->path);
>                  goto endjob;
>              }
>          } else if (!(flags & VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)) {
> diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
> index 0ce9632..2a0b200 100644
> --- a/src/qemu/qemu_parse_command.c
> +++ b/src/qemu/qemu_parse_command.c
> @@ -650,6 +650,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
>      int idx = -1;
>      int busid = -1;
>      int unitid = -1;
> +    bool is_firmware = false;

wow - changing this code too...  not everyone does this!  I really doubt
the code even really works very well any more.

>  
>      if (qemuParseKeywords(val,
>                            &keywords,
> @@ -772,6 +773,9 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
>                  def->bus = VIR_DOMAIN_DISK_BUS_VIRTIO;
>              } else if (STREQ(values[i], "xen")) {
>                  def->bus = VIR_DOMAIN_DISK_BUS_XEN;
> +            } else if (STREQ(values[i], "pflash")) {
> +                def->bus = VIR_DOMAIN_DISK_BUS_LAST;
> +                is_firmware = true;
>              } else if (STREQ(values[i], "sd")) {
>                  def->bus = VIR_DOMAIN_DISK_BUS_SD;
>              }
> @@ -943,8 +947,25 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
>          ignore_value(VIR_STRDUP(def->dst, "hda"));
>      }
>  
> -    if (!def->dst)
> -        goto error;
> +    if (!def->dst) {
> +        if (is_firmware && def->bus == VIR_DOMAIN_DISK_BUS_LAST) {
> +            if (!dom->os.loader && (VIR_ALLOC(dom->os.loader) < 0))
> +                goto error;
> +            if (def->src->readonly) {
> +                /* Loader spec */
> +                dom->os.loader->loader_src = def->src;
> +                dom->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH;
> +            } else {
> +                /* NVRAM Spec */
> +                if (!dom->os.loader->nvram && (VIR_ALLOC(dom->os.loader->nvram) < 0))
> +                    goto error;
> +                dom->os.loader->nvram = def->src;
> +            }
> +        } else {
> +            goto error;
> +        }
> +    }
> +
>      if (STREQ(def->dst, "xvda"))
>          def->dst[3] = 'a' + idx;
>      else
> @@ -2215,8 +2236,11 @@ qemuParseCommandLine(virCapsPtr caps,
>          } else if (STREQ(arg, "-bios")) {
>              WANT_VALUE();
>              if (VIR_ALLOC(def->os.loader) < 0 ||
> -                VIR_STRDUP(def->os.loader->path, val) < 0)
> +                VIR_ALLOC(def->os.loader->loader_src) < 0 ||
> +                VIR_STRDUP(def->os.loader->loader_src->path, val) < 0)
>                  goto error;
> +            def->os.loader->loader_src->type = VIR_STORAGE_TYPE_FILE;
> +            def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
>          } else if (STREQ(arg, "-initrd")) {
>              WANT_VALUE();
>              if (VIR_STRDUP(def->os.initrd, val) < 0)
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 6a5262a..725dd6e 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3994,25 +3994,32 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
>      const char *master_nvram_path;
>      ssize_t r;
>  
> -    if (!loader || !loader->nvram || virFileExists(loader->nvram))
> +    if (!loader || !loader->loader_src || !loader->nvram ||
> +        loader->nvram->type == VIR_STORAGE_TYPE_NETWORK)
>          return 0;
>  
>      master_nvram_path = loader->templt;
> -    if (!loader->templt) {
> +    /* Even if a template is not specified, we associate "known" EFI firmware
> +     * to their NVRAM templates.
> +     * Ofcourse this only applies to local firmware paths, as it is diffcult
> +     * for libvirt to parse all network paths.
> +     */
> +    if (!loader->templt && loader->loader_src->type == VIR_STORAGE_TYPE_FILE) {
>          size_t i;
>          for (i = 0; i < cfg->nfirmwares; i++) {
> -            if (STREQ(cfg->firmwares[i]->name, loader->path)) {
> +            if (STREQ(cfg->firmwares[i]->name, loader->loader_src->path)) {
>                  master_nvram_path = cfg->firmwares[i]->nvram;
>                  break;
>              }
>          }
>      }
>  
> -    if (!master_nvram_path) {
> -        virReportError(VIR_ERR_OPERATION_FAILED,
> -                       _("unable to find any master var store for "
> -                         "loader: %s"), loader->path);
> -        goto cleanup;
> +    if (!master_nvram_path && loader->nvram) {
> +        /* There is no template description, but an NVRAM spec
> +         * has already been provided.
> +         * Trust the client to have generated the right spec here
> +         */
> +        return 0;
>      }
>  
>      if ((srcFD = virFileOpenAs(master_nvram_path, O_RDONLY,
> @@ -4022,13 +4029,13 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
>                               master_nvram_path);
>          goto cleanup;
>      }
> -    if ((dstFD = virFileOpenAs(loader->nvram,
> +    if ((dstFD = virFileOpenAs(loader->nvram->path,
>                                 O_WRONLY | O_CREAT | O_EXCL,
>                                 S_IRUSR | S_IWUSR,
>                                 cfg->user, cfg->group, 0)) < 0) {
>          virReportSystemError(-dstFD,
>                               _("Failed to create file '%s'"),
> -                             loader->nvram);
> +                             loader->nvram->path);
>          goto cleanup;
>      }
>      created = true;
> @@ -4046,7 +4053,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
>          if (safewrite(dstFD, buf, r) < 0) {
>              virReportSystemError(errno,
>                                   _("Unable to write to file '%s'"),
> -                                 loader->nvram);
> +                                 loader->nvram->path);
>              goto cleanup;
>          }
>      } while (r);
> @@ -4060,7 +4067,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
>      if (VIR_CLOSE(dstFD) < 0) {
>          virReportSystemError(errno,
>                               _("Unable to close file '%s'"),
> -                             loader->nvram);
> +                             loader->nvram->path);
>          goto cleanup;
>      }
>  
> @@ -4070,7 +4077,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
>       * copy the file content. Roll back. */
>      if (ret < 0) {
>          if (created)
> -            unlink(loader->nvram);
> +            unlink(loader->nvram->path);
>      }
>  
>      VIR_FORCE_CLOSE(srcFD);
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 663c8c9..fce4204 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -1604,7 +1604,8 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
>      }
>  
>      if (def->os.loader && def->os.loader->nvram &&
> -        virSecurityDACRestoreFileLabel(priv, def->os.loader->nvram) < 0)
> +        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> +        virSecurityDACRestoreFileLabel(priv, def->os.loader->nvram->path) < 0)
>          rc = -1;
>  
>      return rc;
> @@ -1732,8 +1733,9 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr,
>          return -1;
>  
>      if (def->os.loader && def->os.loader->nvram &&
> +        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
>          virSecurityDACSetOwnership(priv, NULL,
> -                                   def->os.loader->nvram, user, group) < 0)
> +                                   def->os.loader->nvram->path, user, group) < 0)
>          return -1;
>  
>      if (def->os.kernel &&
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index c26cdac..396e7fc 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -2459,7 +2459,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr,
>          rc = -1;
>  
>      if (def->os.loader && def->os.loader->nvram &&
> -        virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram) < 0)
> +        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> +        virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram->path) < 0)
>          rc = -1;
>  
>      return rc;
> @@ -2851,8 +2852,9 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr,
>      /* This is different than kernel or initrd. The nvram store
>       * is really a disk, qemu can read and write to it. */
>      if (def->os.loader && def->os.loader->nvram &&
> +        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
>          secdef && secdef->imagelabel &&
> -        virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram,
> +        virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram->path,
>                                       secdef->imagelabel) < 0)
>          return -1;
>  
> 

There's still far too much missing... even in the qemu side - there
needs to be xml2argv test changes... *.args output...

Perhaps best to generate a v2 with things separated better and more
complete changes and take it from there.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Qemu driver: Support network-backed pflash disks.
Posted by Prerna 5 years, 11 months ago 
On Sat, Apr 28, 2018 at 1:11 AM, John Ferlan <jferlan@redhat.com> wrote:

>
>
> On 04/20/2018 04:59 AM, Prerna Saxena wrote:
> > So far libvirt domain XML only allows local filepaths that can be
> > used to specify a loader element or its matching NVRAM disk.
> > Given that Vms may themselves move across hypervisor hosts, it should be
> > possible to allocate loaders/NVRAM disks on network storage for
> > uninterrupted access.
> >
> > Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
> > ---
> >  docs/schemas/domaincommon.rng   | 108 +++++++++++++++++++----
> >  src/conf/domain_conf.c          | 188 ++++++++++++++++++++++++++++++
> ++++++----
> >  src/conf/domain_conf.h          |   7 +-
> >  src/qemu/qemu_cgroup.c          |  13 ++-
> >  src/qemu/qemu_command.c         |  21 +++--
> >  src/qemu/qemu_domain.c          |  16 ++--
> >  src/qemu/qemu_driver.c          |   7 +-
> >  src/qemu/qemu_parse_command.c   |  30 ++++++-
> >  src/qemu/qemu_process.c         |  33 ++++---
> >  src/security/security_dac.c     |   6 +-
> >  src/security/security_selinux.c |   6 +-
> >  11 files changed, 361 insertions(+), 74 deletions(-)
> >
>
> I know on IRC you asked Peter or Michal to review (and they're CC'd
> here), but before they get a chance next week some time - I'll give you
>

Thank you for taking a look.


> a quick look. You do understand Peter, Michal, myself, and other Red Hat
> libvirt developers follow libvir-list anyway - so CC'ing any of us
> doesn't do much since we filter into a mail folder anyway...
>
>
I understand. Peter & Michal had participated in the original IRC
discussion around the need for extending loader as a network backed device,
and I had cc'ed them for context.


> This will need a v2 anyway because the patch has too much going on and
> needs to be split up more.
>

Will do. I should have properly mentioned this was an RFC rather than a
ready-to-merge patch, and that this currently excluded test and
documentation fixes.


>
> You need to break out the domain_conf, docs, etc. changes into one
> patch. Much of what you put into the cover that describes the XML should


Got it.


> go into this patch's commit message. There should be xml2xml test
> changes as well as adjustments to formatdomain.html.in to describe the
> new options. The part of the cover that says automatically reformatting
> to use the storage source cannot happen. There's precedence for that

when the <encryption> and <auth> moved *into* the storage source we
> still have to accommodate for them outside. Internally, it can be placed
> as expected, but when formatting, we have to format as we read. After
>

Ok. I had explicitly asked around on IRC if it was okay "breaking" the
existing XML  semantics. Peter had mentioned it was okay to have the XML
read as its old semantic. The formatter could later "translate" the old
-style absolute filepaths into the "new-style" source-description that is
introduced.
I had kept that in mind while implementing this patch. If that is not to be
done, I will need to redo parts of this patch.


> that, perhaps add the security changes in another, the cgroup in
> another, and finally the qemu adjustments in the last.  Not even sure if
> you need a capability as well.
>
>
Why do you think we'd need a capability for this?  I'd be keen to
understand why changes to XML spec  is not enough.


> Finally this doesn't even compile for me.  You removed @path from
> _virDomainLoaderDef but neglected to adjust all consumers. I suggest
> using cscope and egrep'ing on "os.*loader->path" as well as ->nvram
> since you changed it's type.
>

I know why. I had run and tested this to work, but my build configuration
included the qemu driver and excluded every other driver. Given that this
element extends to other drivers, I can understand the build issues. Again,
should have mentioned this was an RFC.


>
> I assume you've considered that network storage types need to deal with
> authentication and encryption passphrases, right?  What about using a
> srcpool storage source?
>
>
Erm, no. This patch does not include support for encryption/authenticaton.
I will need to add those.


> I'll briefly scan the rest.
>
> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.
> rng
> > index 4cab55f..32db395 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -276,7 +276,42 @@
> >                  </choice>
> >                </attribute>
> >              </optional>
> > -            <ref name="absFilePath"/>
> > +            <optional>
> > +              <attribute name="backing">
> > +                <choice>
> > +                  <value>file</value>
> > +                  <value>network</value>
> > +                </choice>
> > +              </attribute>
> > +            </optional>
> > +            <optional>
> > +              <choice>
> > +                 <group>
> > +                  <ref name="absFilePath"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceFileElement"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolNBD"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolGluster"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolRBD"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolISCSI"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolHTTP"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolSimple"/>
> > +                 </group>
> > +              </choice>
> > +            </optional>
> >            </element>
> >          </optional>
> >          <optional>
> > @@ -287,7 +322,40 @@
> >                </attribute>
> >              </optional>
> >              <optional>
> > -              <ref name="absFilePath"/>
> > +              <attribute name="backing">
> > +                <choice>
> > +                  <value>file</value>
> > +                  <value>network</value>
> > +                </choice>
> > +              </attribute>
> > +            </optional>
> > +            <optional>
> > +              <choice>
> > +                 <group>
> > +                  <ref name="absFilePath"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceFileElement"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolNBD"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolGluster"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolRBD"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolISCSI"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolHTTP"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolSimple"/>
> > +                 </group>
> > +              </choice>
> >              </optional>
> >            </element>
> >          </optional>
> > @@ -1494,25 +1562,29 @@
> >        </attribute>
> >      </optional>
> >      <optional>
> > -      <element name="source">
> > -        <optional>
> > -          <attribute name="file">
> > -            <ref name="absFilePath"/>
> > -          </attribute>
> > -        </optional>
> > -        <optional>
> > -          <ref name="storageStartupPolicy"/>
> > -        </optional>
> > -        <optional>
> > -          <ref name="encryption"/>
> > -        </optional>
> > -        <zeroOrMore>
> > -          <ref name='devSeclabel'/>
> > -        </zeroOrMore>
> > -      </element>
> > +      <ref name='diskSourceFileElement'/>
> >      </optional>
> >    </define>
> >
> > +  <define name="diskSourceFileElement">
> > +    <element name="source">
> > +      <optional>
> > +        <attribute name="file">
> > +          <ref name="absFilePath"/>
> > +        </attribute>
> > +      </optional>
> > +      <optional>
> > +        <ref name="storageStartupPolicy"/>
> > +      </optional>
> > +      <optional>
> > +        <ref name="encryption"/>
> > +      </optional>
> > +      <zeroOrMore>
> > +        <ref name='devSeclabel'/>
> > +      </zeroOrMore>
> > +    </element>
> > +  </define>
> > +
> >    <define name="diskSourceBlock">
> >      <attribute name="type">
> >        <value>block</value>
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 35666c1..c80f9d9 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -2883,8 +2883,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr
> loader)
> >      if (!loader)
> >          return;
> >
> > -    VIR_FREE(loader->path);
> > -    VIR_FREE(loader->nvram);
> > +    virStorageSourceFree(loader->loader_src);
>
> loader_src is redundant to loader isn't it?
>

Should this just be called "src" ? I was not sure if this sounded ambiguous.


>
> > +    virStorageSourceFree(loader->nvram);
> >      VIR_FREE(loader->templt);
> >      VIR_FREE(loader);
> >  }
> > @@ -17961,17 +17961,59 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr
> def)
> >
> >  static int
> >  virDomainLoaderDefParseXML(xmlNodePtr node,
> > +                           xmlXPathContextPtr ctxt,
> >                             virDomainLoaderDefPtr loader)
> >  {
> >      int ret = -1;
> >      char *readonly_str = NULL;
> >      char *secure_str = NULL;
> >      char *type_str = NULL;
> > +    char *tmp = NULL;
> > +    xmlNodePtr cur;
> > +    xmlXPathContextPtr cur_ctxt = ctxt;
> > +
> > +    if (VIR_ALLOC(loader->loader_src)) {
> > +        goto cleanup;
> > +    }
>
> syntax-check would choke here with the unnecessary { }
>

Will remove.


>
> > +    loader->loader_src->type = VIR_STORAGE_TYPE_LAST;
>
> ug, ah, no.
>
> Consider my comment from above - you have either "path" or "source" and
> deal with it from there. When you format you need to format as you read.
>
>
As I mentioned above, I had understood that we deprecate the old format in
favour of the new. I also believe that the new is a superset of the old:
Eg, the old spec said :
<loader>/usr/share/OVMF/OVMF_CODE.fd</loader>

The new one should expect the same thing as:
<loader backing='file'><source
file='/usr/share/OVMF/OVMF_CODE.fd'/></loader>

So, if you notice, the two are not really distinct but rather the new spec
is a better description of the old.
So personally, I would prefer deprecating the old style format in favour of
new.
However, I'd go with community recommendation.

>
> >      readonly_str = virXMLPropString(node, "readonly");
> >      secure_str = virXMLPropString(node, "secure");
> >      type_str = virXMLPropString(node, "type");
> > -    loader->path = (char *) xmlNodeGetContent(node);
> > +
> > +    if ((tmp = virXMLPropString(node, "backing")) &&
> > +        (loader->loader_src->type = virStorageTypeFromString(tmp)) <=
> 0) {
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                       _("unknown loader type '%s'"), tmp);
> > +        goto cleanup;
> > +    }
> > +    VIR_FREE(tmp);
> > +
> > +    for (cur = node->children; cur != NULL; cur = cur->next) {
> > +        if (cur->type != XML_ELEMENT_NODE) {
> > +            continue;
> > +        }
> > +
> > +        if (virXMLNodeNameEqual(cur, "source")) {
> > +            if (virDomainStorageSourceParse(cur, cur_ctxt,
> loader->loader_src, 0) < 0) {
> > +                virReportError(VIR_ERR_XML_DETAIL,
> > +                               _("Error parsing Loader source
> element"));
> > +                goto cleanup;
> > +            }
> > +            break;
> > +        }
> > +    }
> > +
> > +    /* Old-style absolute path found ? */
> > +    if (loader->loader_src->type == VIR_STORAGE_TYPE_LAST) {
> > +        if (!(loader->loader_src->path = (char *)
> xmlNodeGetContent(node))) {
> > +            virReportError(VIR_ERR_XML_ERROR, "%s",
> > +                           _("missing loader source"));
> > +            goto cleanup;
> > +        } else {
> > +            loader->loader_src->type = VIR_STORAGE_TYPE_FILE;
> > +        }
> > +    }
>
> You need to find a different way.  If you find <source>, then parse it.
> If you find <path> then you parse it instead.
>
>
Pls see my comment above on why the two should not be treated as distinct
options.


> >
> >      if (readonly_str &&
> >          (loader->readonly = virTristateBoolTypeFromString(readonly_str))
> <= 0) {
> > @@ -17998,13 +18040,78 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
> >      }
> >
> >      ret = 0;
> > - cleanup:
> > +    goto exit;
> > +cleanup:
> > +    if (loader->loader_src)
> > +      VIR_FREE(loader->loader_src);
> > +exit:
>
> This is not the way we handle this.
>
>
Will fix.


> >      VIR_FREE(readonly_str);
> >      VIR_FREE(secure_str);
> >      VIR_FREE(type_str);
> > +
>
> extraneous
>

Will remove.


>
> >      return ret;
> >  }
> >
>
> Two blank lines.
>

Will remove.


>
> > +static int
> > +virDomainLoaderNvramDefParseXML(xmlNodePtr node,
> > +                           xmlXPathContextPtr ctxt,
> > +                           virDomainLoaderDefPtr loader)
> > +{
> > +    int ret = -1;
> > +    char *tmp = NULL;
> > +    xmlNodePtr cur;
> > +
> > +    if (VIR_ALLOC(loader->nvram)) {
> > +        goto cleanup;
> > +    }
>
> Again { }
>

Sorry.


>
> Similar problem to before w/r/t the XML processing here.
>
> > +
> > +    loader->nvram->type = VIR_STORAGE_TYPE_LAST;
> > +
> > +    if ((tmp = virXMLPropString(node, "backing")) &&
> > +        (loader->nvram->type = virStorageTypeFromString(tmp)) <= 0) {
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                       _("unknown nvram type '%s'"), tmp);
> > +        goto cleanup;
> > +    }
> > +    VIR_FREE(tmp);
> > +
> > +    for (cur = node->children; cur != NULL; cur = cur->next) {
> > +        if (cur->type != XML_ELEMENT_NODE) {
> > +            continue;
> > +        }
> > +
> > +        if (virXMLNodeNameEqual(cur, "template")) {
> > +            loader->templt = virXPathString("string(./os/nvram[1]/@template)",
> ctxt);
> > +            continue;
> > +        }
> > +
> > +        if (virXMLNodeNameEqual(cur, "source")) {
> > +            if (virDomainStorageSourceParse(cur, ctxt, loader->nvram,
> 0) < 0) {
> > +                virReportError(VIR_ERR_XML_DETAIL,
> > +                               _("Error parsing nvram source element"));
> > +                goto cleanup;
> > +            }
> > +            ret = 0;
> > +        }
> > +    }
> > +
> > +    if (loader->nvram->type == VIR_STORAGE_TYPE_LAST) {
> > +        if (!(loader->nvram->path = (char *) xmlNodeGetContent(node))) {
> > +            virReportError(VIR_ERR_XML_ERROR, "%s",
> > +                           _("missing nvram source"));
> > +            goto cleanup;
> > +        } else {
> > +            loader->nvram->type = VIR_STORAGE_TYPE_FILE;
> > +            ret = 0;
> > +        }
> > +    }
> > +    return ret;
> > +
> > +cleanup:
> > +    if (loader->nvram)
> > +      VIR_FREE(loader->nvram);
> > +    return ret;
> > +}
> >
> >  static virBitmapPtr
> >  virDomainSchedulerParse(xmlNodePtr node,
> > @@ -18397,11 +18504,15 @@ virDomainDefParseBootOptions(virDomainDefPtr
> def,
> >              if (VIR_ALLOC(def->os.loader) < 0)
> >                  goto error;
> >
> > -            if (virDomainLoaderDefParseXML(loader_node,
> def->os.loader) < 0)
> > +            def->os.loader->loader_src = NULL;
> > +            def->os.loader->nvram = NULL;
> > +            if (virDomainLoaderDefParseXML(loader_node, ctxt,
> def->os.loader) < 0)
> >                  goto error;
> >
> > -            def->os.loader->nvram = virXPathString("string(./os/nvram[1])",
> ctxt);
> > -            def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)",
> ctxt);
> > +            if ((loader_node = virXPathNode("./os/nvram[1]", ctxt)) &&
> > +                (virDomainLoaderNvramDefParseXML(loader_node, ctxt,
> > +                                                 def->os.loader) < 0))
> > +                    goto error;
>
> don't reuse "loader_node" for "nvram_node".
>

Will add a separate ptr.


>
> I think you need to separate loader patches from nvram patches too.
> It'll perhaps make things easier to review again eventually.
>

Will do.


>
> >          }
> >      }
> >
> > @@ -26070,11 +26181,19 @@ virDomainHugepagesFormat(virBufferPtr buf,
> >
> >  static void
> >  virDomainLoaderDefFormat(virBufferPtr buf,
> > -                         virDomainLoaderDefPtr loader)
> > +                         virDomainLoaderDefPtr loader,
> > +                         unsigned int flags)
>
> As noted earlier reading in one format means we write out in the same
> format. If someone wants to use the new format, then they can.
>

Hmm, as mentioned, this was not the assumption on which this patch was
based.


>
> >  {
> >      const char *readonly = virTristateBoolTypeToString(
> loader->readonly);
> >      const char *secure = virTristateBoolTypeToString(loader->secure);
> >      const char *type = virDomainLoaderTypeToString(loader->type);
> > +    const char *backing = NULL;
> > +
> > +    virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
> > +    virBuffer childBuf = VIR_BUFFER_INITIALIZER;
> > +
> > +    virBufferSetChildIndent(&childBuf, buf);
> > +
> >
> >      virBufferAddLit(buf, "<loader");
> >
> > @@ -26084,17 +26203,54 @@ virDomainLoaderDefFormat(virBufferPtr buf,
> >      if (loader->secure)
> >          virBufferAsprintf(buf, " secure='%s'", secure);
> >
> > -    virBufferAsprintf(buf, " type='%s'>", type);
> > +    virBufferAsprintf(buf, " type='%s'", type);
> > +    if (loader->loader_src &&
> > +        loader->loader_src->type != VIR_STORAGE_TYPE_LAST) {
> > +        if (virDomainStorageSourceFormat(&attrBuf, &childBuf,
> loader->loader_src,
> > +                                     flags, 0) < 0)
> > +            goto cleanup;
> > +
> > +        backing = virStorageTypeToString(loader->loader_src->type);
> > +        virBufferAsprintf(buf, " backing='%s'>", backing);
> > +
> > +        if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0)
> > +            goto cleanup;
> > +    } else {
> > +        virBufferAddLit(buf, ">\n");
> > +    }
> > +
> > +    virBufferAddLit(buf, "</loader>\n");
> >
> > -    virBufferEscapeString(buf, "%s</loader>\n", loader->path);
> >      if (loader->nvram || loader->templt) {
> > -        virBufferAddLit(buf, "<nvram");
> > -        virBufferEscapeString(buf, " template='%s'", loader->templt);
> > +        ignore_value(virBufferContentAndReset(&attrBuf));
> > +        ignore_value(virBufferContentAndReset(&childBuf));
> > +        virBufferSetChildIndent(&childBuf, buf);
> > +
> >          if (loader->nvram)
> > -            virBufferEscapeString(buf, ">%s</nvram>\n", loader->nvram);
> > -        else
> > -            virBufferAddLit(buf, "/>\n");
> > +            backing = virStorageTypeToString(loader->nvram->type);
> > +
> > +        virBufferAddLit(buf, "<nvram");
> > +        if (loader->templt) {
> > +            virBufferEscapeString(buf, " template='%s'",
> loader->templt);
> > +        }
> > +        if (loader->nvram &&
> > +            (virDomainStorageSourceFormat(&attrBuf, &childBuf,
> > +                                         loader->nvram, flags, 0) < 0))
> {
> > +            virBufferAddLit(buf, ">\n</nvram>\n");
> > +            goto cleanup;
> > +        }
> > +
> > +        virBufferEscapeString(buf, " backing='%s'>", backing);
> > +        if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) <
> 0) {
> > +            virBufferAddLit(buf, "</nvram>\n");
> > +            goto cleanup;
> > +        }
> > +        virBufferAddLit(buf, "</nvram>\n");
> >      }
> > +cleanup:
> > +    virBufferFreeAndReset(&attrBuf);
> > +    virBufferFreeAndReset(&childBuf);
> > +    return;
> >  }
> >
> >  static void
> > @@ -26757,7 +26913,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> >          virBufferAsprintf(buf, "<initgroup>%s</initgroup>\n",
> def->os.initgroup);
> >
> >      if (def->os.loader)
> > -        virDomainLoaderDefFormat(buf, def->os.loader);
> > +        virDomainLoaderDefFormat(buf, def->os.loader, flags);
> >      virBufferEscapeString(buf, "<kernel>%s</kernel>\n",
> >                            def->os.kernel);
> >      virBufferEscapeString(buf, "<initrd>%s</initrd>\n",
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index 3c7eccb..50d5ac3 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -1857,14 +1857,17 @@ typedef enum {
> >
> >  VIR_ENUM_DECL(virDomainLoader)
> >
> > +struct _virStorageSource;
> > +typedef struct _virStorageSource* virStorageSourcePtr;
> > +
> >  typedef struct _virDomainLoaderDef virDomainLoaderDef;
> >  typedef virDomainLoaderDef *virDomainLoaderDefPtr;
> >  struct _virDomainLoaderDef {
> > -    char *path;
> > +    virStorageSourcePtr loader_src;
>
> You'll probably need a way to booleanize which format was read. I forget
> how things were done for encryption and auth changes that I made.
>

Will check.


>
> >      int readonly;   /* enum virTristateBool */
> >      virDomainLoader type;
> >      int secure;     /* enum virTristateBool */
> > -    char *nvram;    /* path to non-volatile RAM */
> > +    virStorageSourcePtr nvram;  /* path to non-voliatile RAM */
> >      char *templt;   /* user override of path to master nvram */
> >  };
> >
> > diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> > index d88eb78..aa5d071 100644
> > --- a/src/qemu/qemu_cgroup.c
> > +++ b/src/qemu/qemu_cgroup.c
> > @@ -580,16 +580,21 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm)
> >  static int
> >  qemuSetupFirmwareCgroup(virDomainObjPtr vm)
> >  {
> > +    virStorageSourcePtr src = NULL;
> > +
> >      if (!vm->def->os.loader)
> >          return 0;
> >
> > -    if (vm->def->os.loader->path &&
> > -        qemuSetupImagePathCgroup(vm, vm->def->os.loader->path,
> > -                                 vm->def->os.loader->readonly ==
> VIR_TRISTATE_BOOL_YES) < 0)
> > +    src = vm->def->os.loader->loader_src;
> > +    if (src->path &&
> > +        src->type == VIR_STORAGE_TYPE_FILE &&
> > +        qemuSetupImagePathCgroup(vm, src->path,
> > +                                 src->readonly ==
> VIR_TRISTATE_BOOL_YES) < 0)
> >          return -1;
> >
> >      if (vm->def->os.loader->nvram &&
> > -        qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram, false)
> < 0)
> > +        vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> > +        qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram->path,
> false) < 0)
> >          return -1;
> >
> >      return 0;
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 238c6ed..279a06c 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -9293,6 +9293,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr
> cmd,
> >      virDomainLoaderDefPtr loader = def->os.loader;
> >      virBuffer buf = VIR_BUFFER_INITIALIZER;
> >      int unit = 0;
> > +    char *source = NULL;
> >
> >      if (!loader)
> >          return;
> > @@ -9300,7 +9301,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr
> cmd,
> >      switch ((virDomainLoader) loader->type) {
> >      case VIR_DOMAIN_LOADER_TYPE_ROM:
> >          virCommandAddArg(cmd, "-bios");
> > -        virCommandAddArg(cmd, loader->path);
> > +        virCommandAddArg(cmd, loader->loader_src->path);
> >          break;
> >
> >      case VIR_DOMAIN_LOADER_TYPE_PFLASH:
> > @@ -9312,9 +9313,14 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr
> cmd,
> >                                   NULL);
> >          }
> >
> > +        if (qemuGetDriveSourceString(loader->loader_src, NULL,
> &source) < 0)
> > +            break;
> > +
> >          virBufferAddLit(&buf, "file=");
> > -        virQEMUBuildBufferEscapeComma(&buf, loader->path);
> > -        virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d", unit);
> > +        virQEMUBuildBufferEscapeComma(&buf, source);
> > +        free(source);
> > +        virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d",
> > +                          unit);
>
> What happens if there's authentication or encryption? is that not
> supported?
>

Not supported in this patch, as I had not scoped for it. Will add.


>
> >          unit++;
> >
> >          if (loader->readonly) {
> > @@ -9327,9 +9333,14 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr
> cmd,
> >
> >          if (loader->nvram) {
> >              virBufferFreeAndReset(&buf);
> > +            if (qemuGetDriveSourceString(loader->nvram, NULL, &source)
> < 0)
> > +                break;
> > +
> >              virBufferAddLit(&buf, "file=");
> > -            virQEMUBuildBufferEscapeComma(&buf, loader->nvram);
> > -            virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d",
> unit);
> > +            virQEMUBuildBufferEscapeComma(&buf, source);
> > +            virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d",
> > +                              unit);
> > +            unit++;
> >
> >              virCommandAddArg(cmd, "-drive");
> >              virCommandAddArgBuffer(cmd, &buf);
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 21897cb..c1cb751 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -3312,8 +3312,10 @@ qemuDomainDefPostParse(virDomainDefPtr def,
> >      if (def->os.loader &&
> >          def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
> >          def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON &&
> > -        !def->os.loader->nvram) {
> > -        if (virAsprintf(&def->os.loader->nvram, "%s/%s_VARS.fd",
> > +        def->os.loader->nvram &&
> > +        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> > +        !def->os.loader->nvram->path) {
> > +        if (virAsprintf(&def->os.loader->nvram->path, "%s/%s_VARS.fd",
> >                          cfg->nvramDir, def->name) < 0)
> >              goto cleanup;
> >      }
> > @@ -10477,19 +10479,21 @@ qemuDomainSetupLoader(virQEMUDriverConfigPtr
> cfg ATTRIBUTE_UNUSED,
> >
> >      VIR_DEBUG("Setting up loader");
> >
> > -    if (loader) {
> > +    if (loader && loader->loader_src &&
> > +        loader->loader_src->type == VIR_STORAGE_TYPE_FILE) {
> >          switch ((virDomainLoader) loader->type) {
> >          case VIR_DOMAIN_LOADER_TYPE_ROM:
> > -            if (qemuDomainCreateDevice(loader->path, data, false) < 0)
> > +            if (qemuDomainCreateDevice(loader->loader_src->path, data,
> false) < 0)
> >                  goto cleanup;
> >              break;
> >
> >          case VIR_DOMAIN_LOADER_TYPE_PFLASH:
> > -            if (qemuDomainCreateDevice(loader->path, data, false) < 0)
> > +            if (qemuDomainCreateDevice(loader->loader_src->path, data,
> false) < 0)
> >                  goto cleanup;
> >
> >              if (loader->nvram &&
> > -                qemuDomainCreateDevice(loader->nvram, data, false) < 0)
> > +                loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> > +                qemuDomainCreateDevice(loader->nvram->path, data,
> false) < 0)
> >                  goto cleanup;
> >              break;
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 5673d9f..ce6339d 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -7542,12 +7542,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
> >
> >      if (vm->def->os.loader &&
> >          vm->def->os.loader->nvram &&
> > -        virFileExists(vm->def->os.loader->nvram)) {
> > +        vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> > +        virFileExists(vm->def->os.loader->nvram->path)) {
> >          if ((flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
> > -            if (unlink(vm->def->os.loader->nvram) < 0) {
> > +            if (unlink(vm->def->os.loader->nvram->path) < 0) {
> >                  virReportSystemError(errno,
> >                                       _("failed to remove nvram: %s"),
> > -                                     vm->def->os.loader->nvram);
> > +                                     vm->def->os.loader->nvram->path);
> >                  goto endjob;
> >              }
> >          } else if (!(flags & VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)) {
> > diff --git a/src/qemu/qemu_parse_command.c
> b/src/qemu/qemu_parse_command.c
> > index 0ce9632..2a0b200 100644
> > --- a/src/qemu/qemu_parse_command.c
> > +++ b/src/qemu/qemu_parse_command.c
> > @@ -650,6 +650,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr
> xmlopt,
> >      int idx = -1;
> >      int busid = -1;
> >      int unitid = -1;
> > +    bool is_firmware = false;
>
> wow - changing this code too...  not everyone does this!  I really doubt
> the code even really works very well any more.
>

Added for completeness. Pls let me know if you would want me to drop this.


>
> >
> >      if (qemuParseKeywords(val,
> >                            &keywords,
> > @@ -772,6 +773,9 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr
> xmlopt,
> >                  def->bus = VIR_DOMAIN_DISK_BUS_VIRTIO;
> >              } else if (STREQ(values[i], "xen")) {
> >                  def->bus = VIR_DOMAIN_DISK_BUS_XEN;
> > +            } else if (STREQ(values[i], "pflash")) {
> > +                def->bus = VIR_DOMAIN_DISK_BUS_LAST;
> > +                is_firmware = true;
> >              } else if (STREQ(values[i], "sd")) {
> >                  def->bus = VIR_DOMAIN_DISK_BUS_SD;
> >              }
> > @@ -943,8 +947,25 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr
> xmlopt,
> >          ignore_value(VIR_STRDUP(def->dst, "hda"));
> >      }
> >
> > -    if (!def->dst)
> > -        goto error;
> > +    if (!def->dst) {
> > +        if (is_firmware && def->bus == VIR_DOMAIN_DISK_BUS_LAST) {
> > +            if (!dom->os.loader && (VIR_ALLOC(dom->os.loader) < 0))
> > +                goto error;
> > +            if (def->src->readonly) {
> > +                /* Loader spec */
> > +                dom->os.loader->loader_src = def->src;
> > +                dom->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH;
> > +            } else {
> > +                /* NVRAM Spec */
> > +                if (!dom->os.loader->nvram &&
> (VIR_ALLOC(dom->os.loader->nvram) < 0))
> > +                    goto error;
> > +                dom->os.loader->nvram = def->src;
> > +            }
> > +        } else {
> > +            goto error;
> > +        }
> > +    }
> > +
> >      if (STREQ(def->dst, "xvda"))
> >          def->dst[3] = 'a' + idx;
> >      else
> > @@ -2215,8 +2236,11 @@ qemuParseCommandLine(virCapsPtr caps,
> >          } else if (STREQ(arg, "-bios")) {
> >              WANT_VALUE();
> >              if (VIR_ALLOC(def->os.loader) < 0 ||
> > -                VIR_STRDUP(def->os.loader->path, val) < 0)
> > +                VIR_ALLOC(def->os.loader->loader_src) < 0 ||
> > +                VIR_STRDUP(def->os.loader->loader_src->path, val) < 0)
> >                  goto error;
> > +            def->os.loader->loader_src->type = VIR_STORAGE_TYPE_FILE;
> > +            def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
> >          } else if (STREQ(arg, "-initrd")) {
> >              WANT_VALUE();
> >              if (VIR_STRDUP(def->os.initrd, val) < 0)
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 6a5262a..725dd6e 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -3994,25 +3994,32 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
> >      const char *master_nvram_path;
> >      ssize_t r;
> >
> > -    if (!loader || !loader->nvram || virFileExists(loader->nvram))
> > +    if (!loader || !loader->loader_src || !loader->nvram ||
> > +        loader->nvram->type == VIR_STORAGE_TYPE_NETWORK)
> >          return 0;
> >
> >      master_nvram_path = loader->templt;
> > -    if (!loader->templt) {
> > +    /* Even if a template is not specified, we associate "known" EFI
> firmware
> > +     * to their NVRAM templates.
> > +     * Ofcourse this only applies to local firmware paths, as it is
> diffcult
> > +     * for libvirt to parse all network paths.
> > +     */
> > +    if (!loader->templt && loader->loader_src->type ==
> VIR_STORAGE_TYPE_FILE) {
> >          size_t i;
> >          for (i = 0; i < cfg->nfirmwares; i++) {
> > -            if (STREQ(cfg->firmwares[i]->name, loader->path)) {
> > +            if (STREQ(cfg->firmwares[i]->name,
> loader->loader_src->path)) {
> >                  master_nvram_path = cfg->firmwares[i]->nvram;
> >                  break;
> >              }
> >          }
> >      }
> >
> > -    if (!master_nvram_path) {
> > -        virReportError(VIR_ERR_OPERATION_FAILED,
> > -                       _("unable to find any master var store for "
> > -                         "loader: %s"), loader->path);
> > -        goto cleanup;
> > +    if (!master_nvram_path && loader->nvram) {
> > +        /* There is no template description, but an NVRAM spec
> > +         * has already been provided.
> > +         * Trust the client to have generated the right spec here
> > +         */
> > +        return 0;
> >      }
> >
> >      if ((srcFD = virFileOpenAs(master_nvram_path, O_RDONLY,
> > @@ -4022,13 +4029,13 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
> >                               master_nvram_path);
> >          goto cleanup;
> >      }
> > -    if ((dstFD = virFileOpenAs(loader->nvram,
> > +    if ((dstFD = virFileOpenAs(loader->nvram->path,
> >                                 O_WRONLY | O_CREAT | O_EXCL,
> >                                 S_IRUSR | S_IWUSR,
> >                                 cfg->user, cfg->group, 0)) < 0) {
> >          virReportSystemError(-dstFD,
> >                               _("Failed to create file '%s'"),
> > -                             loader->nvram);
> > +                             loader->nvram->path);
> >          goto cleanup;
> >      }
> >      created = true;
> > @@ -4046,7 +4053,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
> >          if (safewrite(dstFD, buf, r) < 0) {
> >              virReportSystemError(errno,
> >                                   _("Unable to write to file '%s'"),
> > -                                 loader->nvram);
> > +                                 loader->nvram->path);
> >              goto cleanup;
> >          }
> >      } while (r);
> > @@ -4060,7 +4067,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
> >      if (VIR_CLOSE(dstFD) < 0) {
> >          virReportSystemError(errno,
> >                               _("Unable to close file '%s'"),
> > -                             loader->nvram);
> > +                             loader->nvram->path);
> >          goto cleanup;
> >      }
> >
> > @@ -4070,7 +4077,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
> >       * copy the file content. Roll back. */
> >      if (ret < 0) {
> >          if (created)
> > -            unlink(loader->nvram);
> > +            unlink(loader->nvram->path);
> >      }
> >
> >      VIR_FORCE_CLOSE(srcFD);
> > diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> > index 663c8c9..fce4204 100644
> > --- a/src/security/security_dac.c
> > +++ b/src/security/security_dac.c
> > @@ -1604,7 +1604,8 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr
> mgr,
> >      }
> >
> >      if (def->os.loader && def->os.loader->nvram &&
> > -        virSecurityDACRestoreFileLabel(priv, def->os.loader->nvram) <
> 0)
> > +        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> > +        virSecurityDACRestoreFileLabel(priv,
> def->os.loader->nvram->path) < 0)
> >          rc = -1;
> >
> >      return rc;
> > @@ -1732,8 +1733,9 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr
> mgr,
> >          return -1;
> >
> >      if (def->os.loader && def->os.loader->nvram &&
> > +        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> >          virSecurityDACSetOwnership(priv, NULL,
> > -                                   def->os.loader->nvram, user, group)
> < 0)
> > +                                   def->os.loader->nvram->path, user,
> group) < 0)
> >          return -1;
> >
> >      if (def->os.kernel &&
> > diff --git a/src/security/security_selinux.c b/src/security/security_
> selinux.c
> > index c26cdac..396e7fc 100644
> > --- a/src/security/security_selinux.c
> > +++ b/src/security/security_selinux.c
> > @@ -2459,7 +2459,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr
> mgr,
> >          rc = -1;
> >
> >      if (def->os.loader && def->os.loader->nvram &&
> > -        virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram)
> < 0)
> > +        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> > +        virSecuritySELinuxRestoreFileLabel(mgr,
> def->os.loader->nvram->path) < 0)
> >          rc = -1;
> >
> >      return rc;
> > @@ -2851,8 +2852,9 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr
> mgr,
> >      /* This is different than kernel or initrd. The nvram store
> >       * is really a disk, qemu can read and write to it. */
> >      if (def->os.loader && def->os.loader->nvram &&
> > +        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
> >          secdef && secdef->imagelabel &&
> > -        virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram,
> > +        virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram->path,
> >                                       secdef->imagelabel) < 0)
> >          return -1;
> >
> >
>
> There's still far too much missing... even in the qemu side - there
> needs to be xml2argv test changes... *.args output...
>
>
Yes, test and documentation fixes are missing as I'd mentioned.


> Perhaps best to generate a v2 with things separated better and more
> complete changes and take it from there.
>

Agree with that. If only you could pls clarify whether we should
format-as-is-read or change-old-format-to-new.
Thanks once again for the review.

Regards,
Prerna
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Qemu driver: Support network-backed pflash disks.
Posted by John Ferlan 5 years, 11 months ago 

On 05/02/2018 01:24 PM, Prerna wrote:
> 
> 
> On Sat, Apr 28, 2018 at 1:11 AM, John Ferlan <jferlan@redhat.com
> <mailto:jferlan@redhat.com>> wrote:
> 
> 
> 

[...]

>  
> 
>     This will need a v2 anyway because the patch has too much going on and
>     needs to be split up more.
> 
> 
> Will do. I should have properly mentioned this was an RFC rather than a
> ready-to-merge patch, and that this currently excluded test and
> documentation fixes.
>  

Even with an RFC - it's really more pleasant to break things up a bit.
Makes it easier to follow flow rather than one large patch where
multiple things are happening and you're left trying to figure that all
out.  But I also understand that sometimes when posting as RFC that
things languish longer than posting as v1, so it's a gamble as to which
to use...

> 
> 
>     You need to break out the domain_conf, docs, etc. changes into one
>     patch. Much of what you put into the cover that describes the XML
>     should 
> 
> 
> Got it.
>  
> 
>     go into this patch's commit message. There should be xml2xml test
>     changes as well as adjustments to formatdomain.html.in
>     <http://formatdomain.html.in> to describe the
>     new options. The part of the cover that says automatically reformatting
>     to use the storage source cannot happen. There's precedence for that 
> 
>     when the <encryption> and <auth> moved *into* the storage source we
>     still have to accommodate for them outside. Internally, it can be placed
>     as expected, but when formatting, we have to format as we read. After
> 
> 
> Ok. I had explicitly asked around on IRC if it was okay "breaking" the
> existing XML  semantics. Peter had mentioned it was okay to have the XML
> read as its old semantic. The formatter could later "translate" the old
> -style absolute filepaths into the "new-style" source-description that
> is introduced.
> I had kept that in mind while implementing this patch. If that is not to
> be done, I will need to redo parts of this patch.
>  

I see you pinged on IRC today - I had this marked as get back to because
it appears there's questions. Juggling lots of different series and your
response only came on Wed afternoon for me. It's now Friday morning.

In any case, I think that contradicts what was required of me to be done
when moving <auth> and <encryption> into <source> from <disk>. See
commits 37537a7c and 8002d3c which handle formatting as the XML was read.

Now if someone *changes* the read XML to use <source path.../>, then
that's a different story. But if you find:

<loader readonly='yes' secure='no'
type='rom'>/usr/lib/xen/boot/hvmloader</loader>

then you should format that.

If you find

<loader readonly='yes' secure='no' type='rom'>
  <source file='/usr/lib/xen/boot/hvmloader'/>
</loader>

then you format that.

Hopefully the two examples provided give you an idea of the "accepted
mechanism" used in a previous change of XML format.

> 
>     that, perhaps add the security changes in another, the cgroup in
>     another, and finally the qemu adjustments in the last.  Not even sure if
>     you need a capability as well.
> 
> 
> Why do you think we'd need a capability for this?  I'd be keen to
> understand why changes to XML spec  is not enough.
>  

Depends on the command line generated I guess - cannot recall right now
if that was clear while I was looking at the existing changes. In the
long run, you have to decide whether the arguments provided to QEMU have
existed since QEMU 1.5. If they have, then no capability, but if there's
some argument provided on the QEMU command line that was introduced in
(for example) 2.9 in order to allow usage of a networked path, then
you'd need a capability.

> 
>     Finally this doesn't even compile for me.  You removed @path from
>     _virDomainLoaderDef but neglected to adjust all consumers. I suggest
>     using cscope and egrep'ing on "os.*loader->path" as well as ->nvram
>     since you changed it's type.
> 
> 
> I know why. I had run and tested this to work, but my build
> configuration included the qemu driver and excluded every other driver.
> Given that this element extends to other drivers, I can understand the
> build issues. Again, should have mentioned this was an RFC.
>  
> 
> 
>     I assume you've considered that network storage types need to deal with
>     authentication and encryption passphrases, right?  What about using a
>     srcpool storage source?
> 
> 
> Erm, no. This patch does not include support for
> encryption/authenticaton. I will need to add those.
>  

At least a storage source provides the capability to storage, parse,
format that data...

> 
>     I'll briefly scan the rest.
> 

[...]

>     > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>     > index 35666c1..c80f9d9 100644
>     > --- a/src/conf/domain_conf.c
>     > +++ b/src/conf/domain_conf.c
>     > @@ -2883,8 +2883,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr
>     loader)
>     >      if (!loader)
>     >          return;
>     > 
>     > -    VIR_FREE(loader->path);
>     > -    VIR_FREE(loader->nvram);
>     > +    virStorageSourceFree(loader->loader_src);
> 
>     loader_src is redundant to loader isn't it?
> 
> 
> Should this just be called "src" ? I was not sure if this sounded ambiguous.
>  

Well, it's a "disk->src" - so there's precedence.  I was merely pointing
out loader->loader_src is redundant on loader.

> 
> 
>     > +    virStorageSourceFree(loader->nvram);
>     >      VIR_FREE(loader->templt);
>     >      VIR_FREE(loader);
>     >  }

[...]

> 
>     > +    loader->loader_src->type = VIR_STORAGE_TYPE_LAST;
> 
>     ug, ah, no.
> 
>     Consider my comment from above - you have either "path" or "source" and
>     deal with it from there. When you format you need to format as you read.
> 
> 
> As I mentioned above, I had understood that we deprecate the old format
> in favour of the new. I also believe that the new is a superset of the old:
> Eg, the old spec said :
> <loader>/usr/share/OVMF/OVMF_CODE.fd</loader>
> 
> The new one should expect the same thing as:
> <loader backing='file'><source
> file='/usr/share/OVMF/OVMF_CODE.fd'/></loader>
> 
> So, if you notice, the two are not really distinct but rather the new
> spec is a better description of the old.
> So personally, I would prefer deprecating the old style format in favour
> of new. 
> However, I'd go with community recommendation.
> 

I don't disagree that "future" XML should use the newer format and can
be described that way in the docs (see how it was handled for the
commits I had).

Yes, it's not pleasant... But we can handle things internally different
than how they are presented/formatted.  You can have a storage source as
the mechanism to manage internally and using a field in the internal
data structure to determine how the output will be formatted.

I do ask that you take the steps slowly when presenting patches.
Changing to store in storage source first and then adding in the other
storage source features later.

>     > 
>     >      readonly_str = virXMLPropString(node, "readonly");
>     >      secure_str = virXMLPropString(node, "secure");
>     >      type_str = virXMLPropString(node, "type");
>     > -    loader->path = (char *) xmlNodeGetContent(node);
>     > +

[...]

>     > diff --git a/src/qemu/qemu_parse_command.c
>     b/src/qemu/qemu_parse_command.c
>     > index 0ce9632..2a0b200 100644
>     > --- a/src/qemu/qemu_parse_command.c
>     > +++ b/src/qemu/qemu_parse_command.c
>     > @@ -650,6 +650,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr
>     xmlopt,
>     >      int idx = -1;
>     >      int busid = -1;
>     >      int unitid = -1;
>     > +    bool is_firmware = false;
> 
>     wow - changing this code too...  not everyone does this!  I really doubt
>     the code even really works very well any more.
> 
> 
> Added for completeness. Pls let me know if you would want me to drop this.
>  
> 

Understood - it was a drive by comment ;-).  Like I said, I'm not sure
how much of the command line parsing code really works right now.
There's so much new stuff that isn't included there. It'd be a "small
project" in order to get it back in line with what could be found on the
command line today.

> 
>     > 
>     >      if (qemuParseKeywords(val,
>     >                            &keywords,
>     > @@ -772,6 +773,9 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr
>     xmlopt,
>     >                  def->bus = VIR_DOMAIN_DISK_BUS_VIRTIO;
>     >              } else if (STREQ(values[i], "xen")) {
>     >                  def->bus = VIR_DOMAIN_DISK_BUS_XEN;
>     > +            } else if (STREQ(values[i], "pflash")) {
>     > +                def->bus = VIR_DOMAIN_DISK_BUS_LAST;
>     > +                is_firmware = true;
>     >              } else if (STREQ(values[i], "sd")) {
>     >                  def->bus = VIR_DOMAIN_DISK_BUS_SD;
>     >              }
>     > @@ -943,8 +947,25 @@

[...]

>  
> 
>     Perhaps best to generate a v2 with things separated better and more
>     complete changes and take it from there.
> 
> 
> Agree with that. If only you could pls clarify whether we should
> format-as-is-read or change-old-format-to-new.
> Thanks once again for the review.
> 
> Regards,
> Prerna
> 

Well you have my opinion on the matter and an example.  Whether anyone
else pipes in to state their opinion - who knows. Cannot force it. You
could wait a few days and then go from there or just start coding as I
suggested.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.