8:11 a.m.
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
8:11 a.m.
New subject: [libvirt] [PATCH 1/5] storage: split virStorageBackendCreateExecCommand in two functions
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
9:31 a.m.
New subject: [libvirt] [PATCH 1/5] storage: split virStorageBackendCreateExecCommand in two functions
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);
8:11 a.m.
New subject: [libvirt] [PATCH 2/5] storage: refactor: split out image creating tool search function
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
8:11 a.m.
New subject: [libvirt] [PATCH 3/5] storage: add new function virStorageBackendConvertImage
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
8:11 a.m.
New subject: [libvirt] [PATCH 4/5] conf: add 'format' attribute to domain/os/nvram element
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
8:11 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: add support for os.nvram 'format' attribute
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
8:17 a.m.
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(-)
2:13 a.m.
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
3:30 a.m.
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
3264
days inactive
3272
days old
9 comments
3 participants
participants (3)
-
Dmitry Andreev -
John Ferlan -
Michal Privoznik