[libvirt] [PATCH RESEND] qemu_driver: unlink new domain cfg file when rollback

[Martin Kletzander]

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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161031/529af9c3/attachment-0001.sig>