8 a.m.
On a domain startup, the variable store path is generated if needed.
The path is intended to be generated only once. However, the updated
domain definition is not saved into config dir rather than state XML
only. So later, whenever the domain is destroyed and the daemon is
restarted, the generated path is forgotten and the file may be left
behind on virDomainUndefine() call.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index dddca35..1b8931e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3876,6 +3876,9 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
goto cleanup;
generated = true;
+
+ if (virDomainSaveConfig(cfg->configDir, def) < 0)
+ goto cleanup;
}
if (!virFileExists(loader->nvram)) {
--
1.8.5.5
12:21 p.m.
On Thu, Sep 25, 2014 at 03:00:06PM +0200, Michal Privoznik wrote:
On a domain startup, the variable store path is generated if needed.
The path is intended to be generated only once. However, the updated
domain definition is not saved into config dir rather than state XML
the 'is not' confused me in here. Did you mean s/not // by any chance?
only. So later, whenever the domain is destroyed and the daemon is
restarted, the generated path is forgotten and the file may be left
behind on virDomainUndefine() call.
And if you did really mean "the updated domain definition is saved
into config dir rather than state XML only", why isn't that done when
the domain is defined (in PostParse() for example) like address
assignment, etc.?
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index dddca35..1b8931e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3876,6 +3876,9 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
goto cleanup;
generated = true;
+
+ if (virDomainSaveConfig(cfg->configDir, def) < 0)
+ goto cleanup;
}
if (!virFileExists(loader->nvram)) {
--
1.8.5.5
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:13 a.m.
On 25.09.2014 19:21, Martin Kletzander wrote:
On Thu, Sep 25, 2014 at 03:00:06PM +0200, Michal Privoznik wrote:
> On a domain startup, the variable store path is generated if needed.
> The path is intended to be generated only once. However, the updated
> domain definition is not saved into config dir rather than state XML
the 'is not' confused me in here. Did you mean s/not // by any chance?
Nope.
> only. So later, whenever the domain is destroyed and the daemon is
> restarted, the generated path is forgotten and the file may be left
> behind on virDomainUndefine() call.
>
And if you did really mean "the updated domain definition is saved
into config dir rather than state XML only", why isn't that done when
the domain is defined (in PostParse() for example) like address
assignment, etc.?
Well, the main reason is that path I'd like to keep path generation
close to copying from the template/master var store. And the copying
doesn't need to be done until the domain is started. But once the path
is generated it shoul persist in the config XML. However, early in the
qemuProcessStart() the vm->def and vm->newDef are swapped to create live
XML. Anything that's done to domain def after the swap is reflected in
live XML only. That's why the config didn't need to be saved. But now,
with NVRAM things have changed.
What we can do (post release preferably) is to break qemuPrepareNVRAM()
into smaller pieces: the NVRAM path would be generated in PostParse()
callback and the file creation somewhere in qemuProcessStart.
Michal
3:46 a.m.
On Fri, Sep 26, 2014 at 10:13:35AM +0200, Michal Privoznik wrote:
On 25.09.2014 19:21, Martin Kletzander wrote:
> On Thu, Sep 25, 2014 at 03:00:06PM +0200, Michal Privoznik wrote:
>> On a domain startup, the variable store path is generated if needed.
>> The path is intended to be generated only once. However, the updated
>> domain definition is not saved into config dir rather than state XML
>
> the 'is not' confused me in here. Did you mean s/not // by any chance?
Nope.
>
>> only. So later, whenever the domain is destroyed and the daemon is
>> restarted, the generated path is forgotten and the file may be left
>> behind on virDomainUndefine() call.
>>
>
> And if you did really mean "the updated domain definition is saved
> into config dir rather than state XML only", why isn't that done when
> the domain is defined (in PostParse() for example) like address
> assignment, etc.?
>
Well, the main reason is that path I'd like to keep path generation
close to copying from the template/master var store. And the copying
doesn't need to be done until the domain is started. But once the path
is generated it shoul persist in the config XML. However, early in the
qemuProcessStart() the vm->def and vm->newDef are swapped to create live
XML. Anything that's done to domain def after the swap is reflected in
live XML only. That's why the config didn't need to be saved. But now,
with NVRAM things have changed.
What we can do (post release preferably) is to break qemuPrepareNVRAM()
into smaller pieces: the NVRAM path would be generated in PostParse()
callback and the file creation somewhere in qemuProcessStart.
That's just one condition that's about few lines that needs to be
moved, but for the release it's fine, as you said, we can do that
post-release.
ACK,
Martin
12:26 p.m.
On 09/25/14 15:00, Michal Privoznik wrote:
On a domain startup, the variable store path is generated if needed.
The path is intended to be generated only once. However, the updated
domain definition is not saved into config dir rather than state XML
only. So later, whenever the domain is destroyed and the daemon is
restarted, the generated path is forgotten and the file may be left
behind on virDomainUndefine() call.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index dddca35..1b8931e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3876,6 +3876,9 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
goto cleanup;
generated = true;
+
+ if (virDomainSaveConfig(cfg->configDir, def) < 0)
+ goto cleanup;
}
if (!virFileExists(loader->nvram)) {
Notes:
(1) I skimmed a few qemu-related source files *very superficially*, and
apparently it is allowed / quite liberal to re-save a domain XML
*whenever*. I was worried about that, but seems like it's fine.
Specifically, the following call chain should be allowed:
qemuProcessStart()
qemuPrepareNVRAM()
virDomainSaveConfig()
OK I guess.
(2) if the virDomainSaveConfig() call fails, then (due to "generated"
being true) loader->nvram will be freed, and re-set to NULL. Hence at
the next-start we'll retry the pathname generation and to save the
domain XML. Seems OK. (Assuming that saving the domain XML is "atomic",
which, if my reading of virFileRewrite() is correct, it is -- written,
synced, then renamed. Good.)
(3) If the virDomainSaveConfig() call succeeds, and we fail sometime
later, then the domain XML will have been rewritten, but loader->nvram
will be freed and NULL-ed nonetheless.
What happens if the user tries to start the domain again? I can imagine
two possibilities:
- the in-memory loader->nvram value is still NULL, not reflecting the
domain XML. Shouldn't be the problem, we'll just regenerate the pathname
again, and dump the XML again. A bit strange, but should be fine.
- or, the in-memory loader-nvram value is not NULL -- it reflects the
domain XML closely. (This should be the case eg. when libvirtd has been
restarted). This should be fine too, we'll just proceed to the
virFileExists() check. Should be fine.
(4) I like how this approach keeps both the pathname generation and the
contents copying associated with VM startup. It "just" saves the
generated pathname for everything later, including a later startup as well.
(5) As Michal explained to me, renames are not supported generally
anyway, so that's fine.
I hope we're not missing anything.
I should really test this, but I'm too tired to do that now, sorry. I'll
trust that you tested it. :)
Acked-by: Laszlo Ersek <lersek(a)redhat.com>
Cole, Giuseppe, do you agree this patch should be fine?
Thanks,
Laszlo
3745
days inactive
3746
days old
4 comments
3 participants
participants (3)
-
Laszlo Ersek -
Martin Kletzander -
Michal Privoznik