On 27.10.2016 17:45, Chen Hanxiao wrote: > From: Chen Hanxiao <chenhanxiao(a)gmail.com&gt; > > 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(a)gmail.com&gt; > --- > 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.

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(a)gmail.com&gt; >>> >>> 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(a)gmail.com&gt; >>> --- >>> 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)