On 10/19/06, Fedotov, Alexei A <[EMAIL PROTECTED]> 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:[EMAIL PROTECTED] >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 <[EMAIL PROTECTED]> wrote: >> >> On 10/19/06, Andrew Zhang <[EMAIL PROTECTED]> wrote: >> >> > On 10/19/06, Fedotov, Alexei A <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
-- -- Ilya Okomin Intel Middleware Products Division
