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(a)gmail.com&gt; --- 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

On Sat, Apr 28, 2018 at 1:11 AM, John Ferlan <jferlan(a)redhat.com&gt; wrote: Thank you for taking a look. 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. 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. Got it. when the <encryption> and <auth> moved *into* the storage source we 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. 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. 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. Erm, no. This patch does not include support for encryption/authenticaton. I will need to add those. Should this just be called "src" ? I was not sure if this sounded ambiguous. Will remove. 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. Pls see my comment above on why the two should not be treated as distinct options. Will fix. Will remove. Will remove. Sorry. Will add a separate ptr. Will do. Hmm, as mentioned, this was not the assumption on which this patch was based. Will check. Not supported in this patch, as I had not scoped for it. Will add. Added for completeness. Pls let me know if you would want me to drop this. Yes, test and documentation fixes are missing as I'd mentioned. 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