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:[EMAIL PROTECTED] >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 <[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
--------------------------------------------------------------------- Terms of use : http://incubator.apache.org/harmony/mailing.html To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
