[libvirt] [PATCH 0/5] UEFI loader NVRAM image in Qcow2 format
Libvirt allows to specify a path to an image file that will be used as a var storage for UEFI firmware. <nvram>/var/lib/libvirt/nvram/guest_VARS.fd</nvram> By default this image is created as a copy of a master image. The master image and it's copy are stored in 'raw' format. Qemu isn't able to create snapshot for an image in 'raw' format. That makes snapshotting impossible for any UEFI configuration. If the image with UEFI nvram is converted to Qcow2 format (and qemu command is modified appropriately), snapshotting works fine. In the patch-set I introduce 'format' attribute for nvram tag that specifies nvram image file format. The patch-set doesn't contains docs and test in v1. Dmitry Andreev (5): storage: split virStorageBackendCreateExecCommand in two functions storage: refactor: split out image creating tool search function storage: add new function virStorageBackendConvertImage conf: add 'format' attribute to domain/os/nvram element qemu: add support for os.nvram 'format' attribute docs/schemas/domaincommon.rng | 8 ++ src/conf/domain_conf.c | 18 +++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 8 +- src/qemu/qemu_process.c | 17 ++++ src/storage/storage_backend.c | 181 ++++++++++++++++++++++++++---------------- src/storage/storage_backend.h | 5 ++ 7 files changed, 167 insertions(+), 71 deletions(-) -- 1.8.3.1
This patch introduces virStorageBackendCreateExec function that has simple type arguments so it can be used without 'virStorageXXX' structures. --- src/storage/storage_backend.c | 130 +++++++++++++++++++++++------------------- 1 file changed, 70 insertions(+), 60 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 194736b..2e14af9 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -675,92 +675,89 @@ virStorageGenerateQcowEncryption(virConnectPtr conn, return ret; } -static int -virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, - virStorageVolDefPtr vol, - virCommandPtr cmd) +static +int virStorageBackendCreateExec(virCommandPtr cmd, const char* path, + uid_t uid, gid_t gid, mode_t mode) { struct stat st; - gid_t gid; - uid_t uid; - mode_t mode = (vol->target.perms->mode == (mode_t) -1 ? - VIR_STORAGE_DEFAULT_VOL_PERM_MODE : - vol->target.perms->mode); bool filecreated = false; int ret = -1; - if ((pool->def->type == VIR_STORAGE_POOL_NETFS) - && (((geteuid() == 0) - && (vol->target.perms->uid != (uid_t) -1) - && (vol->target.perms->uid != 0)) - || ((vol->target.perms->gid != (gid_t) -1) - && (vol->target.perms->gid != getegid())))) { - - virCommandSetUID(cmd, vol->target.perms->uid); - virCommandSetGID(cmd, vol->target.perms->gid); - virCommandSetUmask(cmd, S_IRWXUGO ^ mode); - - if (virCommandRun(cmd, NULL) == 0) { - /* command was successfully run, check if the file was created */ - if (stat(vol->target.path, &st) >= 0) { - filecreated = true; - - /* seems qemu-img disregards umask and open/creates using 0644. - * If that doesn't match what we expect, then let's try to - * re-open the file and attempt to force the mode change. - */ - if (mode != (st.st_mode & S_IRWXUGO)) { - int fd = -1; - int flags = VIR_FILE_OPEN_FORK | VIR_FILE_OPEN_FORCE_MODE; - - if ((fd = virFileOpenAs(vol->target.path, O_RDWR, mode, - vol->target.perms->uid, - vol->target.perms->gid, - flags)) >= 0) { - /* Success - means we're good */ - VIR_FORCE_CLOSE(fd); - ret = 0; - goto cleanup; - } - } - } + /* check is there a need to set uid/gid */ + if ((uid == (uid_t) -1 || geteuid() == uid) + && (gid == (gid_t) -1 || geteuid() == gid)) + goto retry; + + /* first try - create with uid/gid/umask + * Originaly this try was added for NETFS + * that doesn't support chmod. + */ + virCommandSetUID(cmd, uid); + virCommandSetGID(cmd, gid); + virCommandSetUmask(cmd, S_IRWXUGO ^ mode); + + if (virCommandRun(cmd, NULL) < 0) + goto retry; + + if (stat(path, &st) < 0) + goto retry; + + filecreated = true; + + /* seems qemu-img disregards umask and open/creates using 0644. + * If that doesn't match what we expect, then let's try to + * re-open the file and attempt to force the mode change. + */ + if (mode != (st.st_mode & S_IRWXUGO)) { + int fd = -1; + int flags = VIR_FILE_OPEN_FORK | VIR_FILE_OPEN_FORCE_MODE; + + if ((fd = virFileOpenAs(path, O_RDWR, mode, + uid, gid, flags)) >= 0) { + /* Success - means we're good */ + VIR_FORCE_CLOSE(fd); + ret = 0; + virReportSystemError(errno, _("no need to retry %s"), path); + goto cleanup; } } + retry: + /* second try - set uid/gid/umask after creation */ if (!filecreated) { - /* don't change uid/gid/mode if we retry */ virCommandSetUID(cmd, -1); virCommandSetGID(cmd, -1); virCommandSetUmask(cmd, 0); if (virCommandRun(cmd, NULL) < 0) goto cleanup; - if (stat(vol->target.path, &st) < 0) { + + if (stat(path, &st) < 0) { virReportSystemError(errno, - _("failed to create %s"), vol->target.path); + _("failed to create %s"), path); goto cleanup; } + filecreated = true; } - uid = (vol->target.perms->uid != st.st_uid) ? vol->target.perms->uid - : (uid_t) -1; - gid = (vol->target.perms->gid != st.st_gid) ? vol->target.perms->gid - : (gid_t) -1; + uid = (uid != st.st_uid) ? uid : (uid_t) -1; + gid = (gid != st.st_gid) ? gid : (gid_t) -1; + if (((uid != (uid_t) -1) || (gid != (gid_t) -1)) - && (chown(vol->target.path, uid, gid) < 0)) { + && (chown(path, uid, gid) < 0)) { virReportSystemError(errno, _("cannot chown %s to (%u, %u)"), - vol->target.path, (unsigned int) uid, + path, (unsigned int) uid, (unsigned int) gid); goto cleanup; } if (mode != (st.st_mode & S_IRWXUGO) && - chmod(vol->target.path, mode) < 0) { + chmod(path, mode) < 0) { virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), - vol->target.path, mode); + path, mode); goto cleanup; } @@ -768,11 +765,24 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, cleanup: if (ret < 0 && filecreated) - virFileRemove(vol->target.path, vol->target.perms->uid, - vol->target.perms->gid); + virFileRemove(path, uid, gid); return ret; } +static int +virStorageBackendCreateExecCommand(virStorageVolDefPtr vol, + virCommandPtr cmd) +{ + mode_t mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : + vol->target.perms->mode); + + return virStorageBackendCreateExec(cmd, vol->target.path, + vol->target.perms->uid, + vol->target.perms->gid, + mode); +} + enum { QEMU_IMG_BACKING_FORMAT_NONE = 0, QEMU_IMG_BACKING_FORMAT_FLAG, @@ -1153,7 +1163,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, if (!cmd) goto cleanup; - ret = virStorageBackendCreateExecCommand(pool, vol, cmd); + ret = virStorageBackendCreateExecCommand(vol, cmd); virCommandFree(cmd); cleanup: @@ -1167,7 +1177,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, */ static int virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, - virStoragePoolObjPtr pool, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags) @@ -1217,7 +1227,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, cmd = virCommandNewArgList("qcow-create", size, vol->target.path, NULL); - ret = virStorageBackendCreateExecCommand(pool, vol, cmd); + ret = virStorageBackendCreateExecCommand(vol, cmd); virCommandFree(cmd); VIR_FREE(size); -- 1.8.3.1
On 12/08/2015 09:11 AM, Dmitry Andreev wrote:
This patch introduces virStorageBackendCreateExec function that has simple type arguments so it can be used without 'virStorageXXX' structures.
--- src/storage/storage_backend.c | 130 +++++++++++++++++++++++------------------- 1 file changed, 70 insertions(+), 60 deletions(-)
caveat... only looked at this patch of the series... Mostly because I've recently had the (dis)pleasure of working in this code. There's also a backlog of other changes that I want to look go through first prior to the rest of this series. Still seeing this particular code change just caused my "warning, danger, radar" to go off...
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 194736b..2e14af9 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -675,92 +675,89 @@ virStorageGenerateQcowEncryption(virConnectPtr conn, return ret; }
-static int -virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, - virStorageVolDefPtr vol, - virCommandPtr cmd) +static +int virStorageBackendCreateExec(virCommandPtr cmd, const char* path, + uid_t uid, gid_t gid, mode_t mode)
The coding practices are one argument per line... Generating a comment regarding input arguments might have been nice too. Also, if your goal was to remove the "pool pointer", then why not pass the pool->def->type as well and thus avoid the need to remove/rework the if loop below?
{ struct stat st; - gid_t gid; - uid_t uid; - mode_t mode = (vol->target.perms->mode == (mode_t) -1 ? - VIR_STORAGE_DEFAULT_VOL_PERM_MODE : - vol->target.perms->mode); bool filecreated = false; int ret = -1;
- if ((pool->def->type == VIR_STORAGE_POOL_NETFS) - && (((geteuid() == 0) - && (vol->target.perms->uid != (uid_t) -1) - && (vol->target.perms->uid != 0)) - || ((vol->target.perms->gid != (gid_t) -1) - && (vol->target.perms->gid != getegid())))) { - - virCommandSetUID(cmd, vol->target.perms->uid); - virCommandSetGID(cmd, vol->target.perms->gid); - virCommandSetUmask(cmd, S_IRWXUGO ^ mode); - - if (virCommandRun(cmd, NULL) == 0) { - /* command was successfully run, check if the file was created */ - if (stat(vol->target.path, &st) >= 0) { - filecreated = true; - - /* seems qemu-img disregards umask and open/creates using 0644. - * If that doesn't match what we expect, then let's try to - * re-open the file and attempt to force the mode change. - */ - if (mode != (st.st_mode & S_IRWXUGO)) { - int fd = -1; - int flags = VIR_FILE_OPEN_FORK | VIR_FILE_OPEN_FORCE_MODE; - - if ((fd = virFileOpenAs(vol->target.path, O_RDWR, mode, - vol->target.perms->uid, - vol->target.perms->gid, - flags)) >= 0) { - /* Success - means we're good */ - VIR_FORCE_CLOSE(fd); - ret = 0; - goto cleanup; - } - } - } + /* check is there a need to set uid/gid */ + if ((uid == (uid_t) -1 || geteuid() == uid) + && (gid == (gid_t) -1 || geteuid() == gid)) + goto retry;
Not the label retry really conveys the right meaning. To me it's more of a "skip_netfs_when_running_as_root". It seems there is a slight difference to the original code which checked geteuid() == 0 first, then "uid != 0" (where uid would have been the passed vol->target.perms->uid). So the next question is - have you considered whether this is a non privileged connection? e.g. qemu:///system vs. qemu:///session. The geteuid() may not be 0, but the NETFS check was ensuring that if going through that path, not only was it a NETFS storage pool target, but that we were running as root and the passed uid was set and it wasn't 0. This all because of the NFS root_squash environment. As an aside, generally the order for geteuid/getegid checks is swapped - eg uid == geteuid() and gid == getegid()
+ + /* first try - create with uid/gid/umask + * Originaly this try was added for NETFS + * that doesn't support chmod. + */
Not sure I agree with the comment. NETFS is a "special case" especially when the target NFS Server has root_squash enabled. I would suggest you check the very recent history on this code... I'll claim short term memory lapse (or purge out of unnecessary pages in memory) if you ask too many questions though! It's not that NETFS doesn't support chmod, it's written this way because it's possible that a storage volume being created using kemu-img, qmeu-img, or qcow-create is found on a NFS Server with root_squash enabled. That means when the command runs, it has to run under the specified uid/gid; otherwise, the file gets created with (I think the default of) nfsnobody because NFS takes an incoming root connection and "squashes" it. As for the 'mode' - as I found it in my recent foray into the code... If that's provided, then a 'umask' is done during the command run which "theoretically" would set the right protection bits. However, as I learned qemu-img does it's own thing thus the odd comment. With your/this change every pool type target could try the setting of uid, gid, and mode in this manner as long as they've been provided in the volume.xml. While perhaps technically not different than providing uid, gid, and mode, then creating the file, then changing uid, gid, and mode afterwards. So that's input - I'm not sure doing the reworked/retry logic is necessary and I'd certainly caution against it. You can also look at virFileOpenAs, virFileRemove, and virDirCreate to see other code has the (dis)pleasure of dealing with this NFS root_squash environment. John
+ virCommandSetUID(cmd, uid); + virCommandSetGID(cmd, gid); + virCommandSetUmask(cmd, S_IRWXUGO ^ mode); + + if (virCommandRun(cmd, NULL) < 0) + goto retry; + + if (stat(path, &st) < 0) + goto retry; + + filecreated = true; + + /* seems qemu-img disregards umask and open/creates using 0644. + * If that doesn't match what we expect, then let's try to + * re-open the file and attempt to force the mode change. + */ + if (mode != (st.st_mode & S_IRWXUGO)) { + int fd = -1; + int flags = VIR_FILE_OPEN_FORK | VIR_FILE_OPEN_FORCE_MODE; + + if ((fd = virFileOpenAs(path, O_RDWR, mode, + uid, gid, flags)) >= 0) { + /* Success - means we're good */ + VIR_FORCE_CLOSE(fd); + ret = 0; + virReportSystemError(errno, _("no need to retry %s"), path); + goto cleanup; } }
+ retry: + /* second try - set uid/gid/umask after creation */ if (!filecreated) { - /* don't change uid/gid/mode if we retry */ virCommandSetUID(cmd, -1); virCommandSetGID(cmd, -1); virCommandSetUmask(cmd, 0);
if (virCommandRun(cmd, NULL) < 0) goto cleanup; - if (stat(vol->target.path, &st) < 0) { + + if (stat(path, &st) < 0) { virReportSystemError(errno, - _("failed to create %s"), vol->target.path); + _("failed to create %s"), path); goto cleanup; } + filecreated = true; }
- uid = (vol->target.perms->uid != st.st_uid) ? vol->target.perms->uid - : (uid_t) -1; - gid = (vol->target.perms->gid != st.st_gid) ? vol->target.perms->gid - : (gid_t) -1; + uid = (uid != st.st_uid) ? uid : (uid_t) -1; + gid = (gid != st.st_gid) ? gid : (gid_t) -1; + if (((uid != (uid_t) -1) || (gid != (gid_t) -1)) - && (chown(vol->target.path, uid, gid) < 0)) { + && (chown(path, uid, gid) < 0)) { virReportSystemError(errno, _("cannot chown %s to (%u, %u)"), - vol->target.path, (unsigned int) uid, + path, (unsigned int) uid, (unsigned int) gid); goto cleanup; }
if (mode != (st.st_mode & S_IRWXUGO) && - chmod(vol->target.path, mode) < 0) { + chmod(path, mode) < 0) { virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), - vol->target.path, mode); + path, mode); goto cleanup; }
@@ -768,11 +765,24 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
cleanup: if (ret < 0 && filecreated) - virFileRemove(vol->target.path, vol->target.perms->uid, - vol->target.perms->gid); + virFileRemove(path, uid, gid); return ret; }
+static int +virStorageBackendCreateExecCommand(virStorageVolDefPtr vol, + virCommandPtr cmd) +{ + mode_t mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : + vol->target.perms->mode); + + return virStorageBackendCreateExec(cmd, vol->target.path, + vol->target.perms->uid, + vol->target.perms->gid, + mode); +} + enum { QEMU_IMG_BACKING_FORMAT_NONE = 0, QEMU_IMG_BACKING_FORMAT_FLAG, @@ -1153,7 +1163,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, if (!cmd) goto cleanup;
- ret = virStorageBackendCreateExecCommand(pool, vol, cmd); + ret = virStorageBackendCreateExecCommand(vol, cmd);
virCommandFree(cmd); cleanup: @@ -1167,7 +1177,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, */ static int virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, - virStoragePoolObjPtr pool, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags) @@ -1217,7 +1227,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED,
cmd = virCommandNewArgList("qcow-create", size, vol->target.path, NULL);
- ret = virStorageBackendCreateExecCommand(pool, vol, cmd); + ret = virStorageBackendCreateExecCommand(vol, cmd); virCommandFree(cmd); VIR_FREE(size);
This patch splits out the function from virStorageBackendCreateQemuImg. New function virStorageBackendFindQemuImgTool has a char** argument so it can be used without virConnect and virStoragePool. --- src/storage/storage_backend.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2e14af9..efb5ce5 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1129,6 +1129,25 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, return cmd; } +static +int virStorageBackendFindQemuImgTool(char** create_tool) +{ + if (!create_tool) + return -1; + + /* KVM is usually ahead of qemu on features, so try that first */ + *create_tool = virFindFileInPath("kvm-img"); + if (!*create_tool) + *create_tool = virFindFileInPath("qemu-img"); + + if (!*create_tool) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("unable to find kvm-img or qemu-img")); + return -1; + } + return 0; +} + static int virStorageBackendCreateQemuImg(virConnectPtr conn, virStoragePoolObjPtr pool, @@ -1143,16 +1162,8 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, -1); - /* KVM is usually ahead of qemu on features, so try that first */ - create_tool = virFindFileInPath("kvm-img"); - if (!create_tool) - create_tool = virFindFileInPath("qemu-img"); - - if (!create_tool) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("unable to find kvm-img or qemu-img")); + if (virStorageBackendFindQemuImgTool(&create_tool) < 0) return -1; - } imgformat = virStorageBackendQEMUImgBackingFormat(create_tool); if (imgformat < 0) -- 1.8.3.1
virStorageBackendConvertImage can be used to convert image from one format to another. --- src/storage/storage_backend.c | 22 ++++++++++++++++++++++ src/storage/storage_backend.h | 5 +++++ 2 files changed, 27 insertions(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index efb5ce5..8fe5b8b 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2194,3 +2194,25 @@ virStorageBackendFindGlusterPoolSources(const char *host ATTRIBUTE_UNUSED, return 0; } #endif /* #ifdef GLUSTER_CLI */ + +int virStorageBackendConvertImage(const char* inputPath, virStorageFileFormat inputFormat, + const char* path, virStorageFileFormat format, + uid_t uid, gid_t gid, mode_t mode) +{ + const char* inFmt = NULL; + const char* outFmt = NULL; + char* tool = NULL; + virCommandPtr cmd = NULL; + int ret = -1; + + if (virStorageBackendFindQemuImgTool(&tool) < 0 || + (inFmt = virStorageFileFormatTypeToString(inputFormat)) == NULL || + (outFmt = virStorageFileFormatTypeToString(format)) == NULL) + return -1; + + cmd = virCommandNewArgList(tool, "convert", "-f", inFmt, "-O", outFmt, + inputPath, path, NULL); + ret = virStorageBackendCreateExec(cmd, path, uid, gid, mode); + virCommandFree(cmd); + return ret; +} diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 96b5f39..8e6b2d7 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -214,6 +214,11 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, const char *create_tool, int imgformat); +int virStorageBackendConvertImage(const char* inputPath, + virStorageFileFormat inputFormat, + const char* path, + virStorageFileFormat format, + uid_t uid, gid_t gid, mode_t mode); /* ------- virStorageFile backends ------------ */ typedef struct _virStorageFileBackend virStorageFileBackend; typedef virStorageFileBackend *virStorageFileBackendPtr; -- 1.8.3.1
By default libvirt's qemu driver creates nvram file as a copy of the file specified in 'template' attribute and use it as a 'raw' image file. 'raw' image format doesn't support snapshotting. 'format' attribut can be used to specify different format for nvram image file. This patch introduces 'format' attribute with two possible values: 'raw' and 'qcow2'. --- docs/schemas/domaincommon.rng | 8 ++++++++ src/conf/domain_conf.c | 18 ++++++++++++++++++ src/conf/domain_conf.h | 1 + 3 files changed, 27 insertions(+) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4804c69..7c609b5 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -275,6 +275,14 @@ </attribute> </optional> <optional> + <attribute name='format'> + <choice> + <value>raw</value> + <value>qcow2</value> + </choice> + </attribute> + </optional> + <optional> <ref name="absFilePath"/> </optional> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2f5c0ed..ce47522 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15526,6 +15526,19 @@ virDomainDefParseXML(xmlDocPtr xml, def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt); + + if ((tmp = virXPathString("string(./os/nvram[1]/@format)", ctxt))) { + int format = virStorageFileFormatTypeFromString(tmp); + if (format < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("invalid value of format attribute " + "for NVRAM '%s'"), tmp); + goto error; + } + + VIR_FREE(tmp); + def->os.loader->format = format; + } } } @@ -21270,6 +21283,7 @@ virDomainLoaderDefFormat(virBufferPtr buf, { const char *readonly = virTristateBoolTypeToString(loader->readonly); const char *type = virDomainLoaderTypeToString(loader->type); + const char *format = virStorageFileFormatTypeToString(loader->format); virBufferAddLit(buf, "<loader"); @@ -21282,6 +21296,10 @@ virDomainLoaderDefFormat(virBufferPtr buf, if (loader->nvram || loader->templt) { virBufferAddLit(buf, "<nvram"); virBufferEscapeString(buf, " template='%s'", loader->templt); + + if (format && loader->format > 0) + virBufferEscapeString(buf, " format='%s'", format); + if (loader->nvram) virBufferEscapeString(buf, ">%s</nvram>\n", loader->nvram); else diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 90d8e13..4607fac 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1816,6 +1816,7 @@ struct _virDomainLoaderDef { virDomainLoader type; char *nvram; /* path to non-volatile RAM */ char *templt; /* user override of path to master nvram */ + int format; /* virStorageFileFormat */ }; void virDomainLoaderDefFree(virDomainLoaderDefPtr loader); -- 1.8.3.1
UEFI firmwares may want to use a non-volatile memory to store some variables. os.nvram tag is used to specify image file path for this store. 'format' attribute is used to specify this image format. --- src/qemu/qemu_command.c | 8 ++++++-- src/qemu/qemu_process.c | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4ff31dc..26f294c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9132,10 +9132,14 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd, virCommandAddArgBuffer(cmd, &buf); if (loader->nvram) { + const char* format; + + format = loader->format <= 0 ? "raw" : + virStorageFileFormatTypeToString(loader->format); virBufferFreeAndReset(&buf); virBufferAsprintf(&buf, - "file=%s,if=pflash,format=raw,unit=%d", - loader->nvram, unit); + "file=%s,if=pflash,format=%s,unit=%d", + loader->nvram, format, unit); virCommandAddArg(cmd, "-drive"); virCommandAddArgBuffer(cmd, &buf); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4201962..733282c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -69,6 +69,7 @@ #include "virstring.h" #include "virhostdev.h" #include "storage/storage_driver.h" +#include "storage/storage_backend.h" #include "configmake.h" #include "nwfilter_conf.h" #include "netdev_bandwidth_conf.h" @@ -4014,6 +4015,22 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg, goto cleanup; } + /* if the nvram format is configured and it is not equal to 'raw' + * we should convert the master var store instead of copy it */ + if (loader->format > VIR_STORAGE_FILE_RAW) { + if (virStorageBackendConvertImage(master_nvram_path, + VIR_STORAGE_FILE_RAW, + loader->nvram, + loader->format, + cfg->user, cfg->group, + S_IRUSR | S_IWUSR) == 0) + return 0; + virReportError(VIR_ERR_OPERATION_FAILED, + _("Failed to conver master var storage %s to %s " + "for loader"), master_nvram_path, loader->nvram); + goto cleanup; + } + if ((srcFD = virFileOpenAs(master_nvram_path, O_RDONLY, 0, -1, -1, 0)) < 0) { virReportSystemError(-srcFD, -- 1.8.3.1
Found this message right after I'v sent the patch. https://www.redhat.com/archives/libvir-list/2015-January/msg00446.html On 08.12.2015 17:11, Dmitry Andreev wrote:
Libvirt allows to specify a path to an image file that will be used as a var storage for UEFI firmware.
<nvram>/var/lib/libvirt/nvram/guest_VARS.fd</nvram>
By default this image is created as a copy of a master image. The master image and it's copy are stored in 'raw' format.
Qemu isn't able to create snapshot for an image in 'raw' format. That makes snapshotting impossible for any UEFI configuration.
If the image with UEFI nvram is converted to Qcow2 format (and qemu command is modified appropriately), snapshotting works fine.
In the patch-set I introduce 'format' attribute for nvram tag that specifies nvram image file format. The patch-set doesn't contains docs and test in v1.
Dmitry Andreev (5): storage: split virStorageBackendCreateExecCommand in two functions storage: refactor: split out image creating tool search function storage: add new function virStorageBackendConvertImage conf: add 'format' attribute to domain/os/nvram element qemu: add support for os.nvram 'format' attribute
docs/schemas/domaincommon.rng | 8 ++ src/conf/domain_conf.c | 18 +++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 8 +- src/qemu/qemu_process.c | 17 ++++ src/storage/storage_backend.c | 181 ++++++++++++++++++++++++++---------------- src/storage/storage_backend.h | 5 ++ 7 files changed, 167 insertions(+), 71 deletions(-)
On 08.12.2015 15:17, Dmitry Andreev wrote:
Found this message right after I'v sent the patch. https://www.redhat.com/archives/libvir-list/2015-January/msg00446.html
Right. When I came across this patch set I recalled having some discussion about it a long time ago (in fact as it turns out merely a year ago). So I think these patches are not needed then. Michal
On 16.12.2015 11:13, Michal Privoznik wrote:
On 08.12.2015 15:17, Dmitry Andreev wrote:
Found this message right after I'v sent the patch. https://www.redhat.com/archives/libvir-list/2015-January/msg00446.html
Right. When I came across this patch set I recalled having some discussion about it a long time ago (in fact as it turns out merely a year ago). So I think these patches are not needed then.
Yes. In this comment https://bugzilla.redhat.com/show_bug.cgi?id=1180955#c7 Laszlo mentioned the qemu specific problem. I'll discuss it with our QEMU developers and maybe rework this patch set.
Michal
participants (3)
-
Dmitry Andreev -
John Ferlan -
Michal Privoznik