Bugzilla – Full Text Bug Listing
| Summary: | Failed to copy the content of a tar file | ||
|---|---|---|---|
| Product: | [Tools] Target Management | Reporter: | Samuel Wu <samuelwu> |
| Component: | RSE | Assignee: | David McKnight <dmcknigh> |
| Status: | RESOLVED FIXED | QA Contact: | Martin Oberhuber <mober.at+eclipse> |
| Severity: | major | ||
| Priority: | P3 | CC: | dmcknigh, majmal, wagnross |
| Version: | unspecified | ||
| Target Milestone: | 3.3 M6 | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 338120 | ||
| Attachments: | |||
|
Description
Samuel Wu
Archive download used to 'accidentally' work because we were unnecessarily escaping the # character of the destination folder (i.e. "#virtual#" -> #035virtual#035) which caused ArchiveHandlerManager.isVirtual() to return false when it shouldn't have. We no longer escape # because of bug 332275. When isVirtual() returns true, it indicates a transfer into an existing archive in the temp files project. No such archive exists in this case, so the operation fails. When it returns false, as it used to, it indicates a transfer into a regular folder. The operation should have failed in earlier releases however we managed to get by because of the failed check. When downloading a virtual to the temp files project, we shouldn't be transferring into an archive - certainly not if the temp file archive does not exist. The simplest way to fix is a hack which just ensures we use #035virtual#035 in the destination folder rather than #virtual# - that restores the original behaviour. A proper solution may involve making the ArchiveHandlerManager aware of the temp files, treating a path to a non-existent archive local as non-virtual or something else. Created attachment 189529 [details]
patch to preserve original virtual download behaviour
Hi Dave, The patch doesn't see to work for me. When I tried to copy a directory in a tar file, the directory was created on the remote host but the files in the directory were not copied over. When I tried to copy a file in the tar file, I got the following error message. Operation failed. File system input or output error Message reported from file system: D:\temp\workspace_debug\workspace\RemoteSystemsTempFiles\LOCALHOST\d\TPFToolkit_360\Config\host\dstore.linux.tar#virtual#\dstore\bin\rsed.sh (The system cannot find the path specified.) Created attachment 189604 [details]
patch updated to avoid hardcoded strings where possible
(In reply to comment #3) > Hi Dave, > The patch doesn't see to work for me. When I tried to copy a directory in a tar > file, the directory was created on the remote host but the files in the > directory were not copied over. > When I tried to copy a file in the tar file, I got the following error message. > > Operation failed. File system input or output error > Message reported from file system: > D:\temp\workspace_debug\workspace\RemoteSystemsTempFiles\LOCALHOST\d\TPFToolkit_360\Config\host\dstore.linux.tar#virtual#\dstore\bin\rsed.sh > (The system cannot find the path specified.) You're right - the folders copy okay but for some reason the files aren't. The hack patch fixes the download aspect of the problem but an equivalent change would be required for the upload aspect of this. I'd still like to investigate possible alternatives to this at the service layer. Created attachment 189614 [details]
patch to local file service that uses system property to check for temp file target
This may be a bit simpler and doesn't involve any augmentation of paths. Samuel, could you try using this patch?
Hi Dave, The update patch still doesn't work. I got the same error message when copying a file in a tar. When I copied a folder in the tar, the files in that folder were not copied. (In reply to comment #8) > Hi Dave, > The update patch still doesn't work. I got the same error message when copying > a file in a tar. When I copied a folder in the tar, the files in that folder > were not copied. Does it make any difference if you clear the temp files cache first? For me it's been working. Hi Dave, It works fine once I export the plugin. Can you please port it back to 3.2.2? Thanks a lot. I've committed the fix to cvs and I've opened bug 338120 for the backport. The LocalFileService#isTempFile() method looks highly suspicious:
1.) The OSGI instance locations can be set by means other than the System
Property. Please use Platform.getInstanceLocation()
2.) Just using substring(6) of a File URL, replacing the separator and
comparing that with an absolute path will fail for any characters that
need escaping in URL's (eg space char)
I cannot release this change into an I-build, please rework.
(In reply to comment #12) > The LocalFileService#isTempFile() method looks highly suspicious: > > 1.) The OSGI instance locations can be set by means other than the System > Property. Please use Platform.getInstanceLocation() > > 2.) Just using substring(6) of a File URL, replacing the separator and > comparing that with an absolute path will fail for any characters that > need escaping in URL's (eg space char) > > I cannot release this change into an I-build, please rework. I notice that Platform.getInstanceLocation().getURL() produces a URL that does not do escaping (i.e. the space is left as a space). Created attachment 190003 [details]
updated patch using URIs
In order to get around the problem of Platform.getInstanceLocation().getURL() returning unescaped, a new java.io.File is constructed:
private boolean isTempFile(File resource){
Location instanceLoc = Platform.getInstanceLocation();
URL workspaceLocation = instanceLoc.getURL(); // currently, Eclipse doesn't escape URLs
String wsLocation = new File(workspaceLocation.getPath()).toURI().toString();
String fileLocation = resource.toURI().toString();
return fileLocation.startsWith(wsLocation);
}
Created attachment 190004 [details]
updated patch with cleaned imports
I've committed the change to cvs. (In reply to comment #14) The approach with URL -> URI conversions and comparing the URI's is better but not yet perfect because URL's don't handle UNC paths well: http://wiki.eclipse.org/Eclipse/UNC_Paths I would suggest doing this: URL workspaceLocation = Platform.getInstanceLocation().getURL(); URI workspaceURI = URIUtil.toURI(workspaceLocation); URI wsPath = URIUtil.makeRelative(resource.toURI(), workspaceURI); return wsPath.isRelative(); (In reply to comment #17) > (In reply to comment #14) > > The approach with URL -> URI conversions and comparing the URI's is better but > not yet perfect because URL's don't handle UNC paths well: > > http://wiki.eclipse.org/Eclipse/UNC_Paths > > I would suggest doing this: > > URL workspaceLocation = Platform.getInstanceLocation().getURL(); > URI workspaceURI = URIUtil.toURI(workspaceLocation); > URI wsPath = URIUtil.makeRelative(resource.toURI(), workspaceURI); > return wsPath.isRelative(); The URIUtil is convenient for converting the URL. I'm not sure what you're trying to do here with the relative stuff since there is a no "isRelative" method in URI. Why would there be a problem with the string comparison at this point if the URI strings are consistent? URIUtil.makeRelative() compares IPath objects when on the file system. Therefore it is case insensitive on Windows, for instance. If you compare just the URI's then user could open workspace "C:/myws", have the RSETempFiles project created, later open workspace "C:/MyWS" and the temp files wouldn't be found inside the WS. I meant using !wsPath.isAbsolute(). We basically check whether the absolute resource path is "under the workspace root", by trying to make it relative to the workspace root. BTW on UNIX when your workspace root is a symbolic link you might want to first make the current check and if it fails also check against resource.getCanonicalPath() to be sure. (In reply to comment #19) > URIUtil.makeRelative() compares IPath objects when on the file system. > Therefore it is case insensitive on Windows, for instance. If you compare just > the URI's then user could open workspace "C:/myws", have the RSETempFiles > project created, later open workspace "C:/MyWS" and the temp files wouldn't be > found inside the WS. > > I meant using !wsPath.isAbsolute(). We basically check whether the absolute > resource path is "under the workspace root", by trying to make it relative to > the workspace root. > > BTW on UNIX when your workspace root is a symbolic link you might want to first > make the current check and if it fails also check against > resource.getCanonicalPath() to be sure. !wsPath.isAbsolute() won't work because you can get relative paths even when the resource is not in the workspace. Take for instance the following workspace URI: file:/D:/Development2/Runtime%20Workspaces/RSE%203.3/ Suppose the URI for the resource is the following (outside of the workspace): file:/D:/Development/Runtime%20Workspaces/RSE%203.3/RemoteSystemsTempFiles/LOCALHOST/d/zip/mysql-connector-java-5.1.14.tar%23virtual%23/mysql-connector-java-5.1.14/src/lib The resulting relative path becomes this: ../../../Development/Runtime%20Workspaces/RSE%203.3/RemoteSystemsTempFiles/LOCALHOST/d/zip/mysql-connector-java-5.1.14.tar%23virtual%23/mysql-connector-java-5.1.14/src/lib so isAbsolute() will return false. Oops :) I'm open to better suggestions. Just pointing out that a lot can go wrong when trying to determine whether path A is "under" path B in the file system hierarchy. Looks like IPath.isPrefixOf(IPath) is a better API to use, assuming that the workspace is always local (only "file://" URL's supported). Or perhaps you'll find an even better way to determine whether the file is in the "RemoteSystemsTempFiles" project, though that might be hard without having access to core.resources. (In reply to comment #21) > Oops :) > > I'm open to better suggestions. Just pointing out that a lot can go wrong when > trying to determine whether path A is "under" path B in the file system > hierarchy. > > Looks like IPath.isPrefixOf(IPath) is a better API to use, assuming that the > workspace is always local (only "file://" URL's supported). Or perhaps you'll > find an even better way to determine whether the file is in the > "RemoteSystemsTempFiles" project, though that might be hard without having > access to core.resources. The following works - do you see any big issues with this? private boolean isTempFile(File resource){ IPath wsPath = new Path(Platform.getInstanceLocation().getURL().getPath() + "RemoteSystemsTempFiles"); //$NON-NLS-1$ IPath resPath = new Path(resource.getAbsolutePath()); return wsPath.isPrefixOf(resPath); } (In reply to comment #22) I'd be very careful with URL.getPath() -- look at the complex code in URIUtil.toURI(URL) to see how much can go wrong. Here's what I suggest: URI wsURI = URIUtil.toURI(Platform.getInstanceLocation().getURL()); File wsRoot = URIUtil.toFile(wsURI); if (wsRoot!=null) { File rsProj = new File(wsRoot, "RemoteSystemsTempFiles"); IPath rsProjPath = new Path(rsProj.getAbsolutePath()); IPath resPath = new Path(resource.getAbsolutePath()); return rsProjPath.isPrefixOf(resPath); //could also compare canonical paths at this point but won't do here //sine it is costly and most likely not needed for the Tempfiles project. } return false; Created attachment 190042 [details]
updated patch
Alright, updating the patch here to use the URIUtil as suggested.
I've committed the change to the head stream. What plugins were affected by this bug? (In reply to comment #26) > What plugins were affected by this bug? The affected plugin is org.eclipse.rse.services.local. |