comments below
On 08/15/14 15:43, Michal Privoznik wrote:
> When using split UEFI image, it may come handy if libvirt manages per
> domain _VARS file automatically. While the _CODE file is RO and can be
> shared among multiple domains, you certainly don't want to do that on
> the _VARS file. This latter one needs to be per domain. So at the
> domain startup process, if it's determined that domain needs _VARS
> file it's copied from this master _VARS file. The location of the
> master file is configurable in qemu.conf.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> configure.ac | 1 -
> libvirt.spec.in | 2 +
> src/Makefile.am | 1 +
> src/qemu/libvirtd_qemu.aug | 3 +
> src/qemu/qemu.conf | 14 ++++
> src/qemu/qemu_conf.c | 92 ++++++++++++++++++++++++++
> src/qemu/qemu_conf.h | 5 ++
> src/qemu/qemu_process.c | 131 +++++++++++++++++++++++++++++++++++++
> src/qemu/test_libvirtd_qemu.aug.in | 3 +
> 9 files changed, 251 insertions(+), 1 deletion(-)
I can see that you dropped "--with-qemu-nvram-file". This makes sense,
given that we'll have a map of defaults.
> diff --git a/configure.ac b/configure.ac
> index af3fe28..8668e60 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1418,7 +1418,6 @@ platform])
> fi
> AM_CONDITIONAL([VIR_CHRDEV_LOCK_FILE_PATH], [test "$with_chrdev_lock_files" != "no"])
>
> -
> AC_ARG_WITH([secdriver-selinux],
> [AS_HELP_STRING([--with-secdriver-selinux],
> [use SELinux security driver @<:@default=check@:>@])],
(1) I think you could drop this hunk. It doesn't hurt of course and
helps cleaning up the configure.ac file, but not touching it at all in
this series would keep the noise down.
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index f491de7..762b404 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1948,6 +1948,7 @@ exit 0
> %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/
> %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/channel/
> %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/channel/target/
> +%dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/nvram/
> %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/cache/libvirt/qemu/
> %{_datadir}/augeas/lenses/libvirtd_qemu.aug
> %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
> @@ -2050,6 +2051,7 @@ exit 0
> %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/
> %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/channel/
> %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/channel/target/
> +%dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/nvram/
> %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/cache/libvirt/qemu/
> %{_datadir}/augeas/lenses/libvirtd_qemu.aug
> %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
> diff --git a/src/Makefile.am b/src/Makefile.am
> index f69923f..78ef54e 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -2643,6 +2643,7 @@ endif WITH_SANLOCK
> if WITH_QEMU
> $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/qemu"
> $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/qemu/channel/target"
> + $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/qemu/nvram"
> $(MKDIR_P) "$(DESTDIR)$(localstatedir)/run/libvirt/qemu"
> $(MKDIR_P) "$(DESTDIR)$(localstatedir)/cache/libvirt/qemu"
> $(MKDIR_P) "$(DESTDIR)$(localstatedir)/log/libvirt/qemu"
No changes relative to v1 thus far AFAICS, good...
> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index e7db7fe..62951da 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -88,6 +88,8 @@ module Libvirtd_qemu =
>
> let log_entry = bool_entry "log_timestamp"
>
> + let nvram_entry = str_array_entry "nvram"
> +
> (* Each entry in the config is one of the following ... *)
> let entry = vnc_entry
> | spice_entry
> @@ -100,6 +102,7 @@ module Libvirtd_qemu =
> | rpc_entry
> | network_entry
> | log_entry
> + | nvram_entry
>
> let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ]
> let empty = [ label "#empty" . eol ]
makes sense and seems consistent with other str_array_entries
(then, quoting out of order:)
> diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
> index 7796acc..d7802c3 100644
> --- a/src/qemu/test_libvirtd_qemu.aug.in
> +++ b/src/qemu/test_libvirtd_qemu.aug.in
> @@ -74,3 +74,6 @@ module Test_libvirtd_qemu =
> { "migration_port_min" = "49152" }
> { "migration_port_max" = "49215" }
> { "log_timestamp" = "0" }
> + { "nvram"
> + { "1" = "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd" }
> + }
>
(2) Hmmm, looking at other lists in this file, like "cgroup_controllers"
and "cgroup_device_acl", I think you shouldn't indent
{ "nvram"
itself. The four space indentation looks okay for the entries (the one
entry) though.
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index 7bbbe09..79bba36 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -487,3 +487,17 @@
> # Defaults to 1.
> #
> #log_timestamp = 0
> +
> +
> +# Location of master nvram file
> +#
> +# When a domain is configured to use UEFI instead of standard
> +# BIOS it may use a separate storage for UEFI variables. If
> +# that's the case libvirt creates the variable store per domain
> +# using this master file as image. Each UEFI firmware can,
> +# however, have different variables store. Therefore the nvram is
> +# a list of strings when a single item is in form of:
> +# ${PATH_TO_UEFI_FW}:${PATH_TO_UEFI_VARS}.
> +# Later, when libvirt creates per domain variable store, this
> +# list is searched for the master image.
> +#nvram = [ "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd" ]
okay
Now I'll have to rearrange your patch again, to set the scene:
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 90aebef..a14b36f 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -172,6 +172,11 @@ struct _virQEMUDriverConfig {
> int migrationPortMax;
>
> bool logTimestamp;
> +
> + /* Pairs of loader:nvram paths. The list is @nloader items long */
> + char **loader;
> + char **nvram;
> + size_t nloader;
> };
>
> /* Main driver state */
Okay, this representation (for a list of pairs) seems to match existent
practice: _qemuDomainCmdlineDef. Good.
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 238d2b1..0f4dfb1 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -107,6 +107,9 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def)
> VIR_FREE(def);
> }
>
> +#define VIR_QEMU_LOADER_FILE_PATH "/usr/share/OVMF/OVMF_CODE.fd"
> +#define VIR_QEMU_NVRAM_FILE_PATH "/usr/share/OVMF/OVMF_VARS.fd"
> +
I'm okay with these being here.
> virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
> {
> virQEMUDriverConfigPtr cfg;
> @@ -255,6 +258,13 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
>
> cfg->logTimestamp = true;
>
> + if (VIR_ALLOC_N(cfg->loader, 1) < 0 ||
> + VIR_ALLOC_N(cfg->nvram, 1) < 0 ||
> + VIR_STRDUP(cfg->loader[0], VIR_QEMU_LOADER_FILE_PATH) < 0 ||
> + VIR_STRDUP(cfg->nvram[0], VIR_QEMU_NVRAM_FILE_PATH) < 0)
> + goto error;
> + cfg->nloader = 1;
> +
> return cfg;
(3) What happens when the first strdup succeeds, and the second fails?
Under the "error:" label, virObjectUnref() is called, which (apparently)
leads to virQEMUDriverConfigDispose(). However,
virQEMUDriverConfigDispose() depends on cfg->nloader, and if the second
strdup fails, nloader will be zero. Which means that cfg->loader[0] will
be leaked.
I think you should set "cfg->nloader" to 1 between the second alloc, and
the first strdup. Then,
- if the first alloc fails:
- the loop in the destructor is not entered. (Correctly.)
- if the second alloc fails:
- the loop in the destructor is not entered. This is again correct
because cfg->loader[0] will be NULL, and we won't leak anything.
- if the first strdup fails:
- the loop in the destructor is entered. Both [0] array elements will
be NULL, but VIR_FREE can handle that
- if the 2nd strdup fails:
- the loop in the destructor is entered. loader[0] is released, and
VIR_FREE handles the fact that nvram[0] is NULL.
>
> error:
> @@ -305,6 +315,14 @@ static void virQEMUDriverConfigDispose(void *obj)
> virStringFreeList(cfg->securityDriverNames);
>
> VIR_FREE(cfg->lockManagerName);
> +
> + while (cfg->nloader) {
> + VIR_FREE(cfg->loader[cfg->nloader - 1]);
> + VIR_FREE(cfg->nvram[cfg->nloader - 1]);
> + cfg->nloader--;
> + }
> + VIR_FREE(cfg->loader);
> + VIR_FREE(cfg->nvram);
> }
>
>
> @@ -328,6 +346,47 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs,
> }
>
>
> +static int
> +virQEMUDriverConfigNVRAMParse(const char *str,
> + char **loader,
> + char **nvram)
> +{
> + int ret = -1;
> + char **token;
> +
> + if (!(token = virStringSplit(str, ":", 0))) {
Q1: This assumes that "str" is non-NULL... Is that guaranteed? (--> yes,
see later)
virStringSplit() can return:
- NULL, if out of memory,
- an empty vector (ie. token[0] == NULL) if the input string is empty
> + virReportError(VIR_ERR_CONF_SYNTAX,
> + _("Invalid nvram format: '%s'"),
> + str);
(4) Therefore this error message seems incorrect, because (token ==
NULL) means out-of-memory. I think virStringSplit() will have internally
reported that though (via the allocator functions).
> + goto cleanup;
> + }
> +
> + if (token[0]) {
> + virSkipSpaces((const char **) &token[0]);
> + if (token[1])
> + virSkipSpaces((const char **) &token[1]);
> + }
Makes sense. (Note for self: both elements may be empty strings now, see
eg " : ".)
> +
> + /* Up to two tokens are expected */
(5) The comment is inaccurate; exactly two tokens are expected.
> + if (!token[0] || !token[1] || token[2] ||
> + STREQ(token[0], "") || STREQ(token[1], "")) {
> + virReportError(VIR_ERR_CONF_SYNTAX,
> + _("Invalid nvram format: '%s'"),
> + str);
> + goto cleanup;
> + }
> +
> + if (VIR_STRDUP(*loader, token[0]) < 0 ||
> + VIR_STRDUP(*nvram, token[1]) < 0)
> + goto cleanup;
Q2: What happens if the first stdup succeeds but the second fails? (-->
will work correctly, thanks to the caller, see later)
> +
> + ret = 0;
> + cleanup:
> + virStringFreeList(token);
> + return ret;
> +}
> +
> +
> int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
> const char *filename)
> {
> @@ -654,6 +713,39 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
>
> GET_VALUE_BOOL("log_timestamp", cfg->logTimestamp);
>
> + p = virConfGetValue(conf, "nvram");
> + if (p && p->type == VIR_CONF_LIST) {
> + size_t len;
> + virConfValuePtr pp;
> +
> + while (cfg->nloader) {
> + VIR_FREE(cfg->loader[cfg->nloader - 1]);
> + VIR_FREE(cfg->nvram[cfg->nloader - 1]);
> + cfg->nloader--;
> + }
okay (this leaves the lists in consistent state even if we fail lower
down)
> +
> + /* Calc length and check items */
> + for (len = 0, pp = p->list; pp; len++, pp = pp->next) {
> + if (pp->type != VIR_CONF_STRING) {
> + virReportError(VIR_ERR_CONF_SYNTAX, "%s",
> + _("nvram must be a list of strings"));
> + goto cleanup;
> + }
> + }
okay
... what happens if the list is empty? As in, nvram = [].
> +
> + if (VIR_REALLOC_N(cfg->loader, len) < 0 ||
> + VIR_REALLOC_N(cfg->nvram, len) < 0)
> + goto cleanup;
> + cfg->nloader = len;
This is exactly right, see what I proposed for virQEMUDriverConfigNew()!
> +
> + for (i = 0, pp = p->list; pp; i++, pp = pp->next) {
> + if (virQEMUDriverConfigNVRAMParse(pp->str,
I guess I can't tell if "pp->str" is guaranteed to be non-NULL. It
probably is, at worst it should be an empty string. Answers Q1.
> + &cfg->loader[i],
> + &cfg->nvram[i]) < 0)
okay, this answers Q2 in virQEMUDriverConfigNVRAMParse(). This pattern
happens to follow what I proposed for virQEMUDriverConfigNew(). If the
first strdup succeeds in virQEMUDriverConfigNVRAMParse(), and the 2nd
fails, then nvram[i] will remain NULL. But, cfg->nloader will be
correctly set already, so the disposal function will do the right thing.
Great.
(6) Haha, no, not entirely. VIR_REALLOC_N() does *not* work like
VIR_ALLOC_N(): it does *not* zero-fill the fresh area extension. Now
consider the following:
- you specify a list of 10 elements (ie. ten pairs)
- the default size (from the default constructor) is 1 pair
- in the first while loop, we kill said 1st pair (the loader[0] and
nvram[0] will be zeroed by VIR_FREE)
- then the reallocs succeed, appending 9 pairs of garbage
- then virQEMUDriverConfigNVRAMParse() fails for whatever reason
(before, between, or after the two strdups, doesn't matter),
- and the disposal function will try to free a number of garbage
pointers.
You need to call VIR_EXPAND_N() or VIR_RESIZE_N() instead (they zero new
elements).
virResizeN() is based on virExpandN(), and virExpandN() is based on
virReallocN(). Since we don't expand in a loop, one by one, I recommend
VIR_EXPAND_N().
> + goto cleanup;
> + }
> + }
> +
(7) Shouldn't we complain here if nvram is present (p != NULL), but it's
not a list (p->type != VIR_CONF_LIST)?
> ret = 0;
>
> cleanup:
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 52d9052..c64923f 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -66,6 +66,7 @@
> #include "virnuma.h"
> #include "virstring.h"
> #include "virhostdev.h"
> +#include "configmake.h"
>
> #define VIR_FROM_THIS VIR_FROM_QEMU
>
> @@ -3729,6 +3730,129 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver,
> }
>
>
> +static int
> +qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
> + virDomainDefPtr def,
> + bool migrated)
> +{
> + int ret = -1;
> + int srcFD = -1;
> + int dstFD = -1;
> + virDomainLoaderDefPtr loader = def->os.loader;
> + bool generated = false;
> +
> + /* Unless domain has RO loader of pflash type, we have
> + * nothing to do here. If the loader is RW then it's not
> + * using split code and vars feature, so no nvram file needs
> + * to be created. */
> + if (!loader || loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH ||
> + loader->readonly != VIR_TRISTATE_SWITCH_ON)
> + return 0;
> +
> + /* If the nvram path is configured already, there's nothing
> + * we need to do. Unless we are starting the destination side
> + * of migration in which case nvram is configured in the
> + * domain XML but the file doesn't exist yet. Moreover, after
> + * the migration is completed, qemu will invoke a
> + * synchronization write into the nvram file so we don't have
> + * to take care about transmitting the real data on the other
> + * side. */
> + if (loader->nvram && !migrated)
> + return 0;
> +
> + /* Autogenerate nvram path if needed.*/
> + if (!loader->nvram) {
> + if (virAsprintf(&loader->nvram,
> + "%s/lib/libvirt/qemu/nvram/%s_VARS.fd",
> + LOCALSTATEDIR, def->name) < 0)
> + goto cleanup;
> +
> + generated = true;
> + }
> +
> + if (!virFileExists(loader->nvram)) {
> + size_t i;
> + ssize_t r = -1;
> +
> + for (i = 0; i < cfg->nloader; i++) {
> + if (STREQ(cfg->loader[i], loader->path))
> + break;
> + }
> +
> + if (i == cfg->nloader) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("unable to find any master var store for "
> + "loader: %s"), loader->path);
> + goto cleanup;
> + }
> +
> + if ((srcFD = virFileOpenAs(cfg->nvram[i], O_RDONLY,
> + 0, -1, -1, 0)) < 0) {
> + virReportSystemError(-srcFD,
> + _("Failed to open file '%s'"),
> + cfg->nvram[i]);
> + goto cleanup;
> + }
> + if ((dstFD = virFileOpenAs(loader->nvram,
> + 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);
> + goto cleanup;
> + }
> +
> + while (r) {
> + char buf[1024];
> + ssize_t w = 0;
> +
> + if ((r = saferead(srcFD, buf, sizeof(buf))) < 0) {
> + virReportSystemError(errno,
> + _("Unable to read from file '%s'"),
> + cfg->nvram[i]);
> + goto cleanup;
> + }
> +
> + do {
> + ssize_t actW;
> + if ((actW = safewrite(dstFD, buf + w, r - w)) < 0) {
> + virReportSystemError(errno,
> + _("Unable to write to file '%s'"),
> + loader->nvram);
> + goto cleanup;
> + }
> + w += actW;
> + } while (w != r);
> + }
(8) I argue that you do not need to wrap safewrite() in a loop. First of
all, dstFD is a regular file (you just created it with O_EXCL). Then,
safewrite() itself contains a loop. And, in *that* loop, the (r == 0)
condition will never fire. write() can never return 0 on a regular file
*unless* you request it to write 0 bytes, but in that case, the write()
isn't even reached in safewrite(). Hence, write() in safewrite() will
either return: - minus one, - a positive value.
Consequently, safewrite() will return with an error, or write the full
buffer.
> +
> + if (VIR_CLOSE(srcFD) < 0) {
> + virReportSystemError(errno,
> + _("Unable to close file '%s'"),
> + cfg->nvram[i]);
> + goto cleanup;
> + }
> + if (VIR_CLOSE(dstFD) < 0) {
> + virReportSystemError(errno,
> + _("Unable to close file '%s'"),
> + loader->nvram);
> + goto cleanup;
> + }
> + }
> +
> + ret = 0;
> + cleanup:
> + /* We successfully generated the nvram path, but failed to
> + * copy the file content. Roll back. */
> + if (ret < 0 && generated)
> + VIR_FREE(loader->nvram);
(9) In addition, if we're failing and you have created the file
(independently of whether you've created the pathname!), we should
unlink the file. Otherwise, suppose the user just tries to start the VM
*again*, after seeing the error message. This time a truncated
(incomplete) varstore file might exist, from the last (failed) attempt.
Such a truncated varstore should not be passed to qemu, hence it's best
to delete it right when we notice the error creating it.
> +
> + VIR_FORCE_CLOSE(srcFD);
> + VIR_FORCE_CLOSE(dstFD);
> + return ret;
> +}
> +
> +
> int qemuProcessStart(virConnectPtr conn,
> virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> @@ -3797,6 +3921,13 @@ int qemuProcessStart(virConnectPtr conn,
> if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> goto cleanup;
>
> + /* Some things, paths, ... are generated here and we want them to persist.
> + * Fill them in prior to setting the domain def as transient. */
> + VIR_DEBUG("Generating paths");
> +
> + if (qemuPrepareNVRAM(cfg, vm->def, migrateFrom) < 0)
> + goto cleanup;
> +
> /* Do this upfront, so any part of the startup process can add
> * runtime state to vm->def that won't be persisted. This let's us
> * report implicit runtime defaults in the XML, like vnc listen/socket
Thanks,
Laszlo
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list