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: Thu, 19 Oct 2006 20:28:23 +0400
Message-ID: <8E389A5F2FEABA4CB1DEC35A25CB39CE5E4767@mssmsx411>
Thread-Topic: HARMONY-1752: patch review
thread-index: AcbzZiGjDXW0lU6wRCGZyHlJvuFjuwANWGtQ
From: "Fedotov, Alexei A" <alexei.a.fedotov@intel.com>
To: <harmony-dev@incubator.apache.org>

Ilya,
Shouldn't we change OSNetworkSystemLinux.c as well?

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