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 Fri, 20 Oct 2006 01:17:56 GMT
On 10/20/06, Fedotov, Alexei A <alexei.a.fedotov@intel.com> wrote:
>
> Ilya,
> Shouldn't we change OSNetworkSystemLinux.c as well?


ya, it's painful. Would it be better put system different calls into
portlib?

I think this case is because of raw socket which is not supported by portlib
currently.

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
>
>


-- 
Best regards,
Andrew Zhang

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