Mailing-List: contact harmony-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: harmony-dev@incubator.apache.org
Received-SPF: pass (asf.osuosl.org: local policy)
Content-class: urn:content-classes:message
MIME-Version: 1.0
Content-Type: text/plain;
	charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Subject: RE: HARMONY-1752: patch review
Date: Fri, 20 Oct 2006 11:16:00 +0400
Message-ID: <8E389A5F2FEABA4CB1DEC35A25CB39CE5E486E@mssmsx411>
Thread-Topic: HARMONY-1752: patch review
thread-index: Acb0FoNHYKv0fvHJSnyRYnjNpuFx1QAAKdVw
From: "Fedotov, Alexei A" <alexei.a.fedotov@intel.com>
To: <harmony-dev@incubator.apache.org>

Ilya,
Sorry, my fault. I was confused by the line 41.

With best regards,
Alexei Fedotov,
Intel Java & XML Engineering
>-----Original Message-----
>From: Ilya Okomin [mailto:ilya.okomin@gmail.com]
>Sent: Friday, October 20, 2006 11:07 AM
>To: harmony-dev@incubator.apache.org
>Subject: Re: HARMONY-1752: patch review
>
>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 =3D=3D =
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 =3D 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

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