Bug 312715

Comment 1 Suheb 2010-05-12 17:02:56 EDT

See https://jazz.net/jazz/resource/itemName/com.ibm.team.workitem.WorkItem/114325

Comment 2 Dmitry Karasik 2010-05-12 17:20:11 EDT

Support here means that LocalFile.fetchInfo() should be able to detect symbolic links and return the target just like it does on UNIX.

Comment 3 Dmitry Karasik 2010-05-16 11:14:09 EDT

Created attachment 168652 [details]
proposed patch

For what it's worth here's a patch on top of 3.5.2 that works for me.

Comment 4 Suheb 2010-05-17 03:34:45 EDT

Dmitry - Will this patch work with RTC 2.0.0.2 , since it has Eclipse 3.4.2 ?

Comment 5 Dmitry Karasik 2010-05-17 08:53:57 EDT

I think so, the only difference between 3.4.2 and 3.5.2 is the copyright notice.

Comment 6 Pawel Pogorzelski 2010-05-19 08:06:19 EDT

Created attachment 169094 [details]
Patch_v01

Thank you Dmitry. This is your patch updated to HEAD.

Comment 7 Pawel Pogorzelski 2010-05-19 08:09:21 EDT

Created attachment 169095 [details]
Recompiled libs

I've tested x86 and x86_64 libs. Wasn't able to test IA64 since I don't have access to such machine.

Comment 8 Pawel Pogorzelski 2010-05-19 09:13:59 EDT

Created attachment 169107 [details]
Patch_v02

Patch containing service segment incrementation for windows fragments.

Comment 9 Dmitry Karasik 2010-05-19 09:41:01 EDT

I now see the error of my ways, the lines:

free(rdb);
nameString = (*env)->NewString(env, (jchar *)targetName, len);

need to be switched, otherwise NewString is accessing memory that has just been freed. Sorry about that.

Comment 10 Pawel Pogorzelski 2010-05-19 10:03:36 EDT

Created attachment 169113 [details]
Patch_v03

Thanks Dmitry.

Comment 11 Pawel Pogorzelski 2010-05-19 10:40:39 EDT

Created attachment 169121 [details]
Recompiled libs

Comment 12 Martin Oberhuber 2010-05-19 11:15:00 EDT

FYI, the test cases which I contributed for symlinks in bug 172062 already have some code to deal with Vista. Id should simply take a bit of uncommenting some lines to enable those unittests (e.g. isWindowsVista()) once the support is in.

Note that in order to successfully run those tests, the unittests must run with Admin privileges (because only Administrators can create symlinks on Vista).

Comment 13 Dmitry Karasik 2010-05-19 11:24:40 EDT

Also I think that what's missing from my patch is the update to the attributes() call to return ATTRIBUTE_SYMLINK | ATTRIBUTE_LINK_TARGET (probably would need to detect if running on Vista+)

Comment 14 Martin Oberhuber 2010-05-19 12:00:58 EDT

Would it make sense to also report Windows Junctions as ATTRIBUTE_SYMLINK ?
Or just Vista+ Symlinks?

As long as the link is read only, they are almost conceptually equivalent. The biggest (and potentially dangerous) difference is that in Windows Explorer, deleting a Junction deletes the content behind the junction whereas deleting a symlink deletes the link (which is expected). Would likely need some extra testing how IFileStore#delete() handles this -- as per its Javadoc this is *required* to not delete content behind an item exposed with ATTRIBUTE_SYMLINK.

I've been using Junctions a lot on Win XP with the excellent tool from
http://schinagl.priv.at/nt/hardlinkshellext/hardlinkshellext.html
or the commandline tools from
http://technet.microsoft.com/en-us/sysinternals/bb896768.aspx
.

Comment 15 Dmitry Karasik 2010-05-19 14:14:46 EDT

I would vote against junctions being symlinks. Junctions are more equivalent to mount points on unix, which should be exposed seperately if needed.

Comment 16 Geoffrey Clemm 2010-05-19 15:25:57 EDT

I agree with Dmitry.  Exposing junctions as symbolic links is likely to caue more confusion than benefit.

Comment 17 Pawel Pogorzelski 2010-05-20 08:30:18 EDT

Created attachment 169326 [details]
Patch_v04

(In reply to comment #13)
> Also I think that what's missing from my patch is the update to the
> attributes() call to return ATTRIBUTE_SYMLINK | ATTRIBUTE_LINK_TARGET (probably
> would need to detect if running on Vista+)

Done. The check is based on GetVersionEx call what means the version returned could not match actual if running in compatibility mode. Dmitry, please review.

Comment 18 Pawel Pogorzelski 2010-05-20 08:31:23 EDT

Created attachment 169328 [details]
Recompiled libs

Comment 19 Dmitry Karasik 2010-05-20 09:39:56 EDT

The code looks ok to me, however according to MSDN (http://msdn.microsoft.com/en-us/library/ms724832%28v=VS.85%29.aspx) testing for the presence of functions associated with the feature we care about is preferable, so probably something like this:

HMODULE kernelModule = LoadLibraryW(L"kernel32.dll");
if (kernelModule == NULL) {
   return attributes;
}

if (GetProcAddress(kernelModule, "CreateSymbolicLinkW") != NULL) {
  attributes |= ATTRIBUTE_SYMLINK | ATTRIBUTE_LINK_TARGET;
}

FreeLibrary(kernelModule)

return attributes;


However it's a personal preference and I'm ok with leaving the code as it is now.

Comment 20 Pawel Pogorzelski 2010-05-20 11:54:20 EDT

Created attachment 169368 [details]
Patch_v05

(In reply to comment #19)

Comment 21 Pawel Pogorzelski 2010-05-20 12:05:35 EDT

Created attachment 169369 [details]
Recompiled libs

Comment 22 Martin Oberhuber 2010-05-20 12:31:51 EDT

I like the idea of probing for required functions rather than hard-coded version numbers. It seems odd, though, to probe for "CreateSymbolicLinkW" when we really only _read_ symbolic links.

Comment 23 Dmitry Karasik 2010-05-20 13:17:53 EDT

Yes, however it was the closest function that I could find for that feature.

Comment 24 Pawel Pogorzelski 2010-06-22 15:24:33 EDT

Libs in HEAD. Marking as FIXED.