[PATCH v2] qemuDomainSaveInternal: fix memoryleak of virDomainDef

2020-07-28 Thread Chuan Zheng 
From: Zheng Chuan 

virDomainDefPtr 'def' is forgot to free after qemuDomainDefFormatLive(), fix it.

Signed-off-by: Zheng Chuan 
---
 src/qemu/qemu_driver.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 53980d4..2dafe7c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3356,18 +3356,15 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
  * is NULL or whether it was the live xml of the domain moments
  * before.  */
 if (xmlin) {
-virDomainDefPtr def = NULL;
+g_autoptr(virDomainDef) def = NULL;
 
 if (!(def = virDomainDefParseString(xmlin, driver->xmlopt,
 priv->qemuCaps,
 VIR_DOMAIN_DEF_PARSE_INACTIVE |
-
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) {
+
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
 goto endjob;
-}
-if (!qemuDomainCheckABIStability(driver, vm, def)) {
-virDomainDefFree(def);
+if (!qemuDomainCheckABIStability(driver, vm, def))
 goto endjob;
-}
 xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, def, NULL, true, 
true);
 } else {
 xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, vm->def,
-- 
1.8.3.1

Re: [PATCH v2] qemuDomainSaveInternal: fix memoryleak of virDomainDef

2020-07-29 Thread Laine Stump 

On 7/28/20 11:42 PM, Chuan Zheng wrote:

From: Zheng Chuan 

virDomainDefPtr 'def' is forgot to free after qemuDomainDefFormatLive(), fix it.

Signed-off-by: Zheng Chuan 
---
  src/qemu/qemu_driver.c | 9 +++--
  1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 53980d4..2dafe7c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3356,18 +3356,15 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
   * is NULL or whether it was the live xml of the domain moments
   * before.  */
  if (xmlin) {
-virDomainDefPtr def = NULL;
+g_autoptr(virDomainDef) def = NULL;
  
  if (!(def = virDomainDefParseString(xmlin, driver->xmlopt,

  priv->qemuCaps,
  VIR_DOMAIN_DEF_PARSE_INACTIVE |
-
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) {
+
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
  goto endjob;
-}



I had actually meant the {} below this comment, not the ones above ^^. 
Braces are required by the libvirt coding style guide 
(https://libvirt.org/coding-style.html#curly-braces) if either the 
condition or the body is multi-lined.



-if (!qemuDomainCheckABIStability(driver, vm, def)) {
-virDomainDefFree(def);
+if (!qemuDomainCheckABIStability(driver, vm, def))
  goto endjob;
-}
  xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, def, NULL, 
true, true);
  } else {
  xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, vm->def,



This looks good now.


Reviewed-by: Laine Stump 


(and pushed)


Before pushing, I added back the extra braces you removed, and reworded 
the commit log message to be more in line with our standard template 
(and also noted the commit when the leak was added - 0ea479f8f6 all the 
way back in July 2011!)



I thought about not pushing this until after the freeze is over and 
6.7.0 is released (since it is a bug, but a bug that's been in the code 
for 9 years), but in the end decided to push it, because 1) it's a 
bonafide leak in the  *non*-error path of a libvirt public API, it's 
very simple, and 3) I would probably forget to push it if I waited until 
after the freeze is over.



Thanks!

RE: [PATCH v2] qemuDomainSaveInternal: fix memoryleak of virDomainDef

2020-07-29 Thread zhengchuan 
Thanks for your review.
I'll remind myself and follow the libvirt coding style in future work:)

-Original Message-
From: Laine Stump [mailto:la...@redhat.com] 
Sent: 2020年7月30日 3:52
To: libvir-list@redhat.com
Cc: zhengchuan ; fangying ; 
Chenzhendong (alex) ; wanghao (O) 
; Zhanghailiang ; 
yubihong 
Subject: Re: [PATCH v2] qemuDomainSaveInternal: fix memoryleak of virDomainDef

On 7/28/20 11:42 PM, Chuan Zheng wrote:
> From: Zheng Chuan 
>
> virDomainDefPtr 'def' is forgot to free after qemuDomainDefFormatLive(), fix 
> it.
>
> Signed-off-by: Zheng Chuan 
> ---
>   src/qemu/qemu_driver.c | 9 +++--
>   1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 
> 53980d4..2dafe7c 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -3356,18 +3356,15 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
>* is NULL or whether it was the live xml of the domain moments
>* before.  */
>   if (xmlin) {
> -virDomainDefPtr def = NULL;
> +g_autoptr(virDomainDef) def = NULL;
>   
>   if (!(def = virDomainDefParseString(xmlin, driver->xmlopt,
>   priv->qemuCaps,
>   VIR_DOMAIN_DEF_PARSE_INACTIVE |
> -
> VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) {
> +
> + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
>   goto endjob;
> -}


I had actually meant the {} below this comment, not the ones above ^^. 
Braces are required by the libvirt coding style guide
(https://libvirt.org/coding-style.html#curly-braces) if either the condition or 
the body is multi-lined.

> -if (!qemuDomainCheckABIStability(driver, vm, def)) {
> -virDomainDefFree(def);
> +if (!qemuDomainCheckABIStability(driver, vm, def))
>   goto endjob;
> -}
>   xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, def, NULL, 
> true, true);
>   } else {
>   xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, 
> vm->def,


This looks good now.


Reviewed-by: Laine Stump 


(and pushed)


Before pushing, I added back the extra braces you removed, and reworded the 
commit log message to be more in line with our standard template (and also 
noted the commit when the leak was added - 0ea479f8f6 all the way back in July 
2011!)


I thought about not pushing this until after the freeze is over and
6.7.0 is released (since it is a bug, but a bug that's been in the code for 9 
years), but in the end decided to push it, because 1) it's a bonafide leak in 
the  *non*-error path of a libvirt public API, it's very simple, and 3) I would 
probably forget to push it if I waited until after the freeze is over.


Thanks!

Re: [PATCH v2] qemuDomainSaveInternal: fix memoryleak of virDomainDef

2020-07-30 Thread Michal Privoznik 

On 7/29/20 9:51 PM, Laine Stump wrote:



I thought about not pushing this until after the freeze is over and 
6.7.0 is released (since it is a bug, but a bug that's been in the code 
for 9 years), but in the end decided to push it, because 1) it's a 
bonafide leak in the  *non*-error path of a libvirt public API, it's 
very simple, and 3) I would probably forget to push it if I waited until 
after the freeze is over.


(my apologies for hijacking this thread)

Actually, I just thought about this yesterday. We have these freezes 
which in most cases mean "I will postpone pushing a patch for a week". I 
mean, does anybody do any testing of release candidates and how 
extensive is that? Because what I do is merely some very basic testing 
"virsh start/hotplug/destroy/shutdown". Hopefully, we will have some 
integration testing set up soon at which point, the master will always 
be in release ready state and we won't need much (if any) additional 
testing before making a release.


Michal