harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ilya Okomin" <ilya.oko...@gmail.com>
Subject Re: HARMONY-1752: patch review
Date Fri, 20 Oct 2006 07:07:23 GMT
On 10/19/06, Fedotov, Alexei A <alexei.a.fedotov@intel.com> wrote:
>
> Ilya,
> Shouldn't we change OSNetworkSystemLinux.c as well?


Alexei, I'm watching at the latest sources from the svn and I'm not sure we
need make changes to OSNetworkSystemLinux.c. The problem was in freeing
uninitialized fdset_read pointer. In the OSNetworkSystemWin32.c initial
value was undefined and in the OSNetworkSystemLinux.c it is set to NULL.
Thus linux native looks fine for me.

Thanks, Ilya.

With best regards,
> Alexei Fedotov,
> Intel Java & XML Engineering
> >-----Original Message-----
> >From: Andrew Zhang [mailto:zhanghuangzhu@gmail.com]
> >Sent: Thursday, October 19, 2006 2:05 PM
> >To: harmony-dev@incubator.apache.org
> >Subject: Re: HARMONY-1752: patch review
> >
> >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
>
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>
>


-- 
--
Ilya Okomin
Intel Middleware Products Division

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