[libvirt] [PATCH RESEND] qemu_driver: unlink new domain cfg file when rollback
From: Chen Hanxiao <chenhanxiao@gmail.com> If we failed to unlink old dom cfg file, we goto rollback. But inside rollback, we fogot to unlink the new dom cfg file. This patch fixes this issue. Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- src/qemu/qemu_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e6f845d..3f4a2fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19869,6 +19869,7 @@ qemuDomainRenameCallback(virDomainObjPtr vm, char *new_dom_name = NULL; char *old_dom_name = NULL; char *old_dom_cfg_file = NULL; + char *new_dom_cfg_file = NULL; virCheckFlags(0, ret); @@ -19882,6 +19883,11 @@ qemuDomainRenameCallback(virDomainObjPtr vm, goto cleanup; } + if (!(new_dom_cfg_file = virDomainConfigFile(cfg->configDir, + new_dom_name))) { + goto cleanup; + } + event_old = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_UNDEFINED, VIR_DOMAIN_EVENT_UNDEFINED_RENAMED); @@ -19909,6 +19915,7 @@ qemuDomainRenameCallback(virDomainObjPtr vm, cleanup: VIR_FREE(old_dom_cfg_file); + VIR_FREE(new_dom_cfg_file); VIR_FREE(old_dom_name); VIR_FREE(new_dom_name); qemuDomainEventQueue(driver, event_old); @@ -19922,6 +19929,10 @@ qemuDomainRenameCallback(virDomainObjPtr vm, vm->def->name = old_dom_name; old_dom_name = NULL; } + + if (virFileExists(new_dom_cfg_file)) + unlink(new_dom_cfg_file); + goto cleanup; } -- 1.8.3.1
On 27.10.2016 17:45, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@gmail.com>
If we failed to unlink old dom cfg file, we goto rollback. But inside rollback, we fogot to unlink the new dom cfg file. This patch fixes this issue.
Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- src/qemu/qemu_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e6f845d..3f4a2fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19869,6 +19869,7 @@ qemuDomainRenameCallback(virDomainObjPtr vm, char *new_dom_name = NULL; char *old_dom_name = NULL; char *old_dom_cfg_file = NULL; + char *new_dom_cfg_file = NULL;
This could be moved one line up, so that it matches the pattern.
virCheckFlags(0, ret);
@@ -19882,6 +19883,11 @@ qemuDomainRenameCallback(virDomainObjPtr vm, goto cleanup; }
+ if (!(new_dom_cfg_file = virDomainConfigFile(cfg->configDir, + new_dom_name))) { + goto cleanup; + } +
Whoa, I'm really surprised that our syntax-check does not catch this. It has a one line body therefore there shouldn't be any curly braces around it. Also, this could be joined with previous condition.
event_old = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_UNDEFINED, VIR_DOMAIN_EVENT_UNDEFINED_RENAMED); @@ -19909,6 +19915,7 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
cleanup: VIR_FREE(old_dom_cfg_file); + VIR_FREE(new_dom_cfg_file); VIR_FREE(old_dom_name); VIR_FREE(new_dom_name); qemuDomainEventQueue(driver, event_old); @@ -19922,6 +19929,10 @@ qemuDomainRenameCallback(virDomainObjPtr vm, vm->def->name = old_dom_name; old_dom_name = NULL; } + + if (virFileExists(new_dom_cfg_file)) + unlink(new_dom_cfg_file); + goto cleanup; }
ACKed and pushed. Michal
On Fri, Oct 28, 2016 at 04:29:30AM -0700, Michal Privoznik wrote:
On 27.10.2016 17:45, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@gmail.com>
If we failed to unlink old dom cfg file, we goto rollback. But inside rollback, we fogot to unlink the new dom cfg file. This patch fixes this issue.
Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- src/qemu/qemu_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e6f845d..3f4a2fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19882,6 +19883,11 @@ qemuDomainRenameCallback(virDomainObjPtr vm, goto cleanup; }
+ if (!(new_dom_cfg_file = virDomainConfigFile(cfg->configDir, + new_dom_name))) { + goto cleanup; + } +
Whoa, I'm really surprised that our syntax-check does not catch this. It has a one line body therefore there shouldn't be any curly braces around it. Also, this could be joined with previous condition.
Actually! =D We have that so "precisely defined" that in this precise case, it "is optional (not enforced either way)" -- Just to note how specific we can be =)
On 10/31/2016 06:43 AM, Martin Kletzander wrote:
On Fri, Oct 28, 2016 at 04:29:30AM -0700, Michal Privoznik wrote:
On 27.10.2016 17:45, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@gmail.com>
If we failed to unlink old dom cfg file, we goto rollback. But inside rollback, we fogot to unlink the new dom cfg file. This patch fixes this issue.
Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- src/qemu/qemu_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e6f845d..3f4a2fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19882,6 +19883,11 @@ qemuDomainRenameCallback(virDomainObjPtr vm, goto cleanup; }
+ if (!(new_dom_cfg_file = virDomainConfigFile(cfg->configDir, + new_dom_name))) { + goto cleanup; + } +
Whoa, I'm really surprised that our syntax-check does not catch this. It has a one line body therefore there shouldn't be any curly braces around it. Also, this could be joined with previous condition.
Actually! =D We have that so "precisely defined" that in this precise case, it "is optional (not enforced either way)" -- Just to note how specific we can be =)
It used to be undefined, but according to http://libvirt.org/hacking.html#curly_braces (a change made in in 2014 in commit a9d07d33 by Martin), when the condition spans multiple lines, curly braces are now required: "Omit the curly braces around an if, while, for etc. body only when both that body *and the condition itself* occupy a single line." One of the examples following that text is of exactly this situation. (I've always preferred doing it this way, but have been lazy about it a few times and had it pointed out during review by "The Crusader of the Braces", abologna)
On Mon, Oct 31, 2016 at 09:40:21AM -0400, Laine Stump wrote:
On 10/31/2016 06:43 AM, Martin Kletzander wrote:
On Fri, Oct 28, 2016 at 04:29:30AM -0700, Michal Privoznik wrote:
On 27.10.2016 17:45, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@gmail.com>
If we failed to unlink old dom cfg file, we goto rollback. But inside rollback, we fogot to unlink the new dom cfg file. This patch fixes this issue.
Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- src/qemu/qemu_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e6f845d..3f4a2fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19882,6 +19883,11 @@ qemuDomainRenameCallback(virDomainObjPtr vm, goto cleanup; }
+ if (!(new_dom_cfg_file = virDomainConfigFile(cfg->configDir, + new_dom_name))) { + goto cleanup; + } +
Whoa, I'm really surprised that our syntax-check does not catch this. It has a one line body therefore there shouldn't be any curly braces around it. Also, this could be joined with previous condition.
Actually! =D We have that so "precisely defined" that in this precise case, it "is optional (not enforced either way)" -- Just to note how specific we can be =)
It used to be undefined, but according to http://libvirt.org/hacking.html#curly_braces (a change made in in 2014 in commit a9d07d33 by Martin), when the condition spans multiple lines, curly braces are now required:
"Omit the curly braces around an if, while, for etc. body only when both that body *and the condition itself* occupy a single line."
One of the examples following that text is of exactly this situation.
(I've always preferred doing it this way, but have been lazy about it a few times and had it pointed out during review by "The Crusader of the Braces", abologna)
I haven't read that, just looked at the example (and others probably did it the other way around) because the text doesn't match the second example (which is the same as this patch). The commit was pushed by me, but I was kinda directed to do that, it followed a discussion and I should've probably left that to others to send the patch =) Oh silly "young" me. Anyway, just to make sure we're on the same boat, I mentioned this as a funny side note, not that it would be that big of a deal or anything ;) Have a nice day, Martin
participants (4)
-
Chen Hanxiao -
Laine Stump -
Martin Kletzander -
Michal Privoznik