Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 352248 - Indigo upload of executable file on Linux to local system drops executable flags
Summary: Indigo upload of executable file on Linux to local system drops executable flags
Status: NEW
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: 3.3   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: David McKnight CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-15 16:07 EDT by Jeff Johnston CLA
Modified: 2012-01-09 10:28 EST (History)
2 users (show)

See Also:
xuanchen: review+
kjdoyle: review+


Attachments
patch to preserver execute bit on download and upload (6.64 KB, patch)
2011-07-18 17:14 EDT, David McKnight CLA
no flags Details | Diff
patch to save src permissions to target on local upload (1.93 KB, patch)
2011-08-15 15:26 EDT, David McKnight CLA
no flags Details | Diff
patch with copyright statement (2.56 KB, patch)
2011-08-30 10:44 EDT, David McKnight CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff Johnston CLA 2011-07-15 16:07:46 EDT
I am using Indigo RSE and the IRemoteFileSubSystem method to upload a file from my local RHEL6 linux system to a remote system.  If I pick the LOCAL RSE connection and specify a location locally (e.g. /tmp) and try to upload an executable file (a.out), it copies the file without the executable flag (x) and so the file cannot be executed.

Using TCF/RSE, the executable flag is copied over so I don't know why this isn't being done for the local RSE connection.
Comment 1 David McKnight CLA 2011-07-18 17:14:26 EDT
Created attachment 199863 [details]
patch to preserver execute bit on download and upload

Could you try out this patch to see if it makes a difference for you?
Comment 2 Jeff Johnston CLA 2011-08-10 14:35:27 EDT
(In reply to comment #1)
> Created attachment 199863 [details]
> patch to preserver execute bit on download and upload
> 
> Could you try out this patch to see if it makes a difference for you?

Hello, sorry the late reply.

I couldn't get it working.  I also couldn't get the debugger to break in any of the new code you added in the patch (i.e. I set a breakpoint at every check of "os.name" and none of them caught when I did the upload of the file.

I broke after the upload and sure enough, in my /tmp directory, a.out was 664.  Is there something else I can check for you?  The only RSE host I have available is the local one.
Comment 3 David McKnight CLA 2011-08-10 16:38:44 EDT
(In reply to comment #2)
> (In reply to comment #1)
> > Created attachment 199863 [details] [details]
> > patch to preserver execute bit on download and upload
> > 
> > Could you try out this patch to see if it makes a difference for you?
> 
> Hello, sorry the late reply.
> 
> I couldn't get it working.  I also couldn't get the debugger to break in any of
> the new code you added in the patch (i.e. I set a breakpoint at every check of
> "os.name" and none of them caught when I did the upload of the file.
> 
> I broke after the upload and sure enough, in my /tmp directory, a.out was 664. 
> Is there something else I can check for you?  The only RSE host I have
> available is the local one.

Rereading your original post, I gather that you're calling an IRemoteFileSubSystem method to transfer the file.  Rather than call from the subsystem layer, could you go a layer above and instead use UniversalFileTransferUtility API?
Comment 4 Jeff Johnston CLA 2011-08-11 16:27:39 EDT
(In reply to comment #3)
> 
> Rereading your original post, I gather that you're calling an
> IRemoteFileSubSystem method to transfer the file.  Rather than call from the
> subsystem layer, could you go a layer above and instead use
> UniversalFileTransferUtility API?

Yes, I was calling an IRemoteFileSubSystem method.

When I change my code to use the UniversalFileTransferUtility, the new logic you added works.  However, it doesn't work if the file isn't part of the workspace.  My logic was passing around a File so I initially tried just converting it to an IResource using getTempFileFor(File).  This ultimately failed because the timestamp ends up being set to -1 and the upload gives an exception (Negative Timestamp).  I changed the code over to assume the file is in the workspace and to try and find it from the workspaceRoot and this did work, but I can't guarantee that all files must be part of the workspace since this is a generic framework to run Linux tools remotely and there could be config files, etc.., that need to be copied over that don't exist in the workspace.

IMO, it would be desirable to add the logic fix as well to the IRemoteFileSubSystem for local since this works fine for a remote TCF connected system and it doesn't seem to make sense that the local RSE file subsystem cannot achieve this as well.
Comment 5 David McKnight CLA 2011-08-15 15:26:24 EDT
Created attachment 201518 [details]
patch to save src permissions to target on local upload

For file transfer operations that are run within a single host, I'd normally expect clients to use the IRemoteFileSubSystem.copy() APIs rather than upload and download calls.  Perhaps the uploads in your scenario are there for the sake of uniformity.  Can you try your original approach using this patch?
Comment 6 Jeff Johnston CLA 2011-08-26 17:55:42 EDT
(In reply to comment #5)
> Created attachment 201518 [details]
> patch to save src permissions to target on local upload
> 
> For file transfer operations that are run within a single host, I'd normally
> expect clients to use the IRemoteFileSubSystem.copy() APIs rather than upload
> and download calls.  Perhaps the uploads in your scenario are there for the
> sake of uniformity.  Can you try your original approach using this patch?

Sorry for the delay.

The code in question is implementing Valgrind profiling of a local executable on a remote system.

Yes, the code for remote access is set up to be uniform.  There is already a separate plug-in to profile locally via Valgrind.  This has separate code and a slightly different launch interface.  The user picks an RSE connection to run and profile and specifies remote directories to use.  

IMO, I just expect this to work for an RSE connection, regardless if it is really the local connection.

The patch you have provided solves that issue and the upload works fine with the executable bits set properly.  Thanks.
Comment 7 David McKnight CLA 2011-08-30 10:44:27 EDT
Created attachment 202426 [details]
patch with copyright statement
Comment 8 David McKnight CLA 2011-08-30 10:45:09 EDT
Xuan and Kevin, could you please review the patch?
Comment 9 Xuan Chen CLA 2011-08-30 11:35:07 EDT
The fix looks good.  Thanks.
Comment 10 Kevin Doyle CLA 2011-08-30 11:42:41 EDT
Looks good to me as well.
Comment 11 David McKnight CLA 2011-08-30 13:04:54 EDT
Thanks for the reviews.  I've committed the fix to the HEAD stream.
Comment 12 Martin Oberhuber CLA 2011-09-13 07:59:58 EDT
Just for the records,
  - on TCF connections, permissions are copied according to Jeff
  - this patch fixes "Local" connections

But looking at the code, SSH and DStore connections still don't preserve permissions (DStore maybe does when in supertransfer mode, I didn't check).

This doesn't quite seem to be what Jeff wanted. Plus, the chown / chgrp calls in LocalFileService#setPermissions() look scary, what will happen if the current user doesn't have the right to chown? Has anybody tested this on files that don't belong to the current user? Are we sure that this extra code doesn't kill performance? (There's a minimum of 4 additional programs executed for each file being transferred!)

This doesn't seem to be ready for a maintenance release, reopening the defect for now.
Comment 13 Martin Oberhuber CLA 2011-09-14 12:27:19 EDT
We discussed this in the TM monthly meeting:
http://wiki.eclipse.org/TM/Meetings/14-Sep-2011

Committers agreed to not include these changes in Indigo SR1 since we cannot rule out performance / functionality regressions.

Actually as far as we know, TCF is the only subsystem that maintains permissions on upload - none of the other maintains permissions today; therefore it may be more appropriate to set exec bit from the client side with a setPermissions() call where necessary. Also note that in the conntext of a Windows-local / Linux-remote scenario (which is very common!) a different approach will be necessary.

Jeff we'd like to pass the ball to you - how urgent and important is that change for you? Can you work around it on the client side? Why are you interested in the "Local" system type?

I'm clearing the target milestone for now until we get more information.
Comment 14 Jeff Johnston CLA 2011-09-30 16:38:53 EDT
(In reply to comment #13)
> We discussed this in the TM monthly meeting:
> http://wiki.eclipse.org/TM/Meetings/14-Sep-2011
> 
> Committers agreed to not include these changes in Indigo SR1 since we cannot
> rule out performance / functionality regressions.
> 
> Actually as far as we know, TCF is the only subsystem that maintains
> permissions on upload - none of the other maintains permissions today;
> therefore it may be more appropriate to set exec bit from the client side with
> a setPermissions() call where necessary. Also note that in the conntext of a
> Windows-local / Linux-remote scenario (which is very common!) a different
> approach will be necessary.
> 
> Jeff we'd like to pass the ball to you - how urgent and important is that
> change for you? Can you work around it on the client side? Why are you
> interested in the "Local" system type?
> 
> I'm clearing the target milestone for now until we get more information.

I am interested in the permissions because I am using RSE to transfer an executable to run remote profiling using valgrind.  It must be executable or Valgrind cannot profile it.  Surely somebody else must have wanted to send an executable file across an RSE connection.  Without executable permissions, my code cannot work using RSE.

The user is given a list of RSE hosts to send the executable to be profiled remotely.  I don't filter the list to remove the local host, nor should I have to.  So, the user sees "Local" as one of the choices.  In fact I use "Local" for basic testing of the RSE code path (I am using RSE interfaces and expect to see the same results as running it via the regular UI).

What setPermissions() call are you referring to?  I don't see that option with the IRemoteFile.
Comment 15 David McKnight CLA 2012-01-09 10:28:28 EST
(In reply to comment #14)
> 
> What setPermissions() call are you referring to?  I don't see that option with
> the IRemoteFile.

In order to set permissions on an IRemoteFile, you need to get at the IFilePermissionsService that is associated with the file.  The following code illustrates how to use this for setting execute permissions:

IFilePermissionsService service = (IFilePermissionsService((IAdaptable)remoteFile).getAdapter(IFilePermissionsService.class);
if (service != null){
  IHostFilePermissions permissions = service.getFilePermissions(remoteFile.getHostFile(), monitor);
  if (permissions != null){
	permissions.setPermission(IHostFilePermissions.PERM_ANY_EXECUTE, true);					
	service.setFilePermissions(remoteFile.getHostFile(), permissions, monitor);						
  }
}