[libvirt] [PATCH] lxc: do cleanup when failed to create new string
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- src/lxc/lxc_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index c51c4d5..fc399fb 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1045,7 +1045,7 @@ int virLXCProcessStart(virConnectPtr conn, if (virAsprintf(&logfile, "%s/%s.log", cfg->logDir, vm->def->name) < 0) - return -1; + goto cleanup; if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; -- 1.8.2.1
On Tue, Jan 14, 2014 at 05:31:03PM +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- src/lxc/lxc_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index c51c4d5..fc399fb 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1045,7 +1045,7 @@ int virLXCProcessStart(virConnectPtr conn,
if (virAsprintf(&logfile, "%s/%s.log", cfg->logDir, vm->def->name) < 0) - return -1; + goto cleanup;
I see nothing in that cleanup that needs to be done in this codepath. The only thing which might be needed is cleaning up the vm->def->resource, but that doesn't make much sense to me. Can you explain the change? Plus the indentation's off. Thanks, Martin
-----Original Message----- From: Martin Kletzander [mailto:mkletzan@redhat.com] Sent: Tuesday, January 14, 2014 9:24 PM To: Chen Hanxiao Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH] lxc: do cleanup when failed to create new string
On Tue, Jan 14, 2014 at 05:31:03PM +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- src/lxc/lxc_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index c51c4d5..fc399fb 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1045,7 +1045,7 @@ int virLXCProcessStart(virConnectPtr conn,
if (virAsprintf(&logfile, "%s/%s.log", cfg->logDir, vm->def->name) < 0) - return -1; + goto cleanup;
I see nothing in that cleanup that needs to be done in this codepath. The only thing which might be needed is cleaning up the vm->def->resource, but that doesn't make much sense to me. Can you explain the change?
But the code structure looks weird. We have already used 'cleanup' before this section.
Plus the indentation's off.
Thanks, Martin
On Wed, Jan 15, 2014 at 09:47:53AM +0800, Chen Hanxiao wrote:
-----Original Message----- From: Martin Kletzander [mailto:mkletzan@redhat.com] Sent: Tuesday, January 14, 2014 9:24 PM To: Chen Hanxiao Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH] lxc: do cleanup when failed to create new string
On Tue, Jan 14, 2014 at 05:31:03PM +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- src/lxc/lxc_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index c51c4d5..fc399fb 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1045,7 +1045,7 @@ int virLXCProcessStart(virConnectPtr conn,
if (virAsprintf(&logfile, "%s/%s.log", cfg->logDir, vm->def->name) < 0) - return -1; + goto cleanup;
I see nothing in that cleanup that needs to be done in this codepath. The only thing which might be needed is cleaning up the vm->def->resource, but that doesn't make much sense to me. Can you explain the change?
But the code structure looks weird. We have already used 'cleanup' before this section.
I'd rather see either a) the two previous gotos changed into returns (which means using a cleanup only where it needs to be done) or b) using cleanups everywhere, but this doesn't go with what we use elsewhere and the cleanup in this section is *really* huge, so it's unnecessary to go there, I guess. Anyway, me not ACKing it doesn't mean someone else can't agree with you, I just wanted to know the reason (whether there have been a leak or something). Martin
-----Original Message----- From: Martin Kletzander [mailto:mkletzan@redhat.com] Sent: Wednesday, January 15, 2014 2:36 PM To: Chen Hanxiao Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH] lxc: do cleanup when failed to create new string
if (virAsprintf(&logfile, "%s/%s.log", cfg->logDir, vm->def->name) < 0) - return -1; + goto cleanup;
I see nothing in that cleanup that needs to be done in this codepath. The only thing which might be needed is cleaning up the vm->def->resource, but that doesn't make much sense to me. Can you explain the change?
But the code structure looks weird. We have already used 'cleanup' before this section.
I'd rather see either a) the two previous gotos changed into returns (which means using a cleanup only where it needs to be done) or b) using cleanups everywhere, but this doesn't go with what we use elsewhere and the cleanup in this section is *really* huge, so it's unnecessary to go there, I guess.
Anyway, me not ACKing it doesn't mean someone else can't agree with you, I just wanted to know the reason (whether there have been a leak or something).
I didn't spot any issues through tests. But when I read these codes, I felt weird about this section. As we leaked so little memory here and it seldom happened, let's keep what they were. Thanks
Martin
participants (2)
-
Chen Hanxiao -
Martin Kletzander