3:31 a.m.
From: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
7:24 a.m.
On Tue, Jan 14, 2014 at 05:31:03PM +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
7:47 p.m.
-----Original Message-----
From: Martin Kletzander [mailto:mkletzan@redhat.com]
Sent: Tuesday, January 14, 2014 9:24 PM
To: Chen Hanxiao
Cc: libvir-list(a)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(a)cn.fujitsu.com>
>
> Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
12:36 a.m.
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(a)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(a)cn.fujitsu.com>
> >
> > Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
1:18 a.m.
-----Original Message-----
From: Martin Kletzander [mailto:mkletzan@redhat.com]
Sent: Wednesday, January 15, 2014 2:36 PM
To: Chen Hanxiao
Cc: libvir-list(a)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
3964
days inactive
3965
days old
4 comments
2 participants
participants (2)
-
Chen Hanxiao -
Martin Kletzander