harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Zhang" <zhanghuang...@gmail.com>
Subject Re: HARMONY-1752: patch review
Date Thu, 19 Oct 2006 10:04:57 GMT
On 10/19/06, Ilya Okomin <ilya.okomin@gmail.com> wrote:
>
> On 10/19/06, Andrew Zhang <zhanghuangzhu@gmail.com> wrote:
>
> > On 10/19/06, Fedotov, Alexei A <alexei.a.fedotov@intel.com> wrote:
> > >
> > > Hello Ilya,
> > >
> > >
> > >
> > > I really like your patch from
> > > http://issues.apache.org/jira/browse/HARMONY-1752. Let me participate
> in
> > > a way I'm able to.
> > >
> > >
> > >
> > > I cannot say why calling free(send_buf) when send_buf == NULL makes me
> > > feel a bit uncomfortable.
> >
> >
> > It's safe to free NULL pointer.
> >
> > I also prefer a descriptive name for a label
> > > cleanup1 (eg cleanup_buf).
>
>
> Alexei, you are right. I was thinking about 'cleanup_all' but for some
> inexplicable reasons forgot to rename it :)
>
>
>
> > >
> > >
> > >
> > > I tried to track that guy who used cleanup labels in his C code.
> >
> >
> > Let the author speak for himself, but I think it's ok. It's a
> frequentely
> > used error handling sytle in C programming. There's a logical problem in
> > OSNetworkSystemWin32.c, the cleanup may free an invalid pointer
> > fdset_read.
> > But I perfer to add NULL initiliazation when declaring fdset_read, say,
> > fd_set * fdset_read = NULL;
> >
> > comments?
>
>
> Thanks, Andrew, IMO it is more convenient way that I've suggested. The
> problem was really that existed code was trying to free fdset_read, that
> wasn't NULL (we went to the 'cleanup' label before fdset_read
> initialization).
> I'll update patch for H-1752.


The updated patch looks fine. :)

Regards, Ilya.
>
>
>
> > modules/luni/src/main/native/luni/linux/OSNetworkSystemLinux.c:
> > >
> > > The Linux version seems to contain exactly the same problem as you fix
> > > in modules/luni/src/main/native/luni/windows/OSNetworkSystemWin32.c
> > >
> > >
> > >
> > > modules/luni/src/main/native/luni/shared/luniglob.c
> > >
> > > cleanup:
> > >
> > >    if (props) {
> > >
> > >        properties_free(PORTLIB, props);
> > >
> > >    }
> > >
> > >    if (bootDirectory) {
> > >
> > >        hymem_free_memory(bootDirectory);
> > >
> > >    }
> > >
> > > The first "if" always fails if we come here using goto - we can put
> the
> > > label after the first if. I suggest replacing the second if with
> > > assert(!bootDirectory)
> > >
> > >
> > >
> > > modules/archive/src/main/native/archive/shared/jarfile.c:
> > >
> > > zip_freeZipEntry is not called on some paths - is it a memory leak?
> > >
> > >
> > >
> > > modules/luni/src/main/native/launcher/shared/main.c
> > >
> > > if (vm_args.options)
> > >
> > >    {
> > >
> > >      hymem_free_memory (vm_args.options);
> > >
> > >    }
> > >
> > > Should we put here assert(!vm_args.options)?
> > >
> > >
> > >
> > >
> > >
> > > With best regards,
> > > Alexei Fedotov,
> > > Intel Java & XML Engineering
> > >
> > >
> > >
> > >
> > >
> >
> >
> > --
> > Best regards,
> > Andrew Zhang
> >
> >
>
>
> --
> --
> Ilya Okomin
> Intel Middleware Products Division
>
>


-- 
Best regards,
Andrew Zhang

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message