I have mount point: sshfs root@172.16.0.254:/ '/home/rim/mnt/172.16.0.254' -o reconnect -o workaround=all -o transform_symlinks -o default_permissions -o hard_remove -o noauto_cache chmod 0777 /home/rim/mnt/172.16.0.254/test.txt change file permission, but in Thunar all controls on Permissions tab in file Properties - disabled.
Created attachment 182427 [details] permission fix and respect HAVE_GIO_UNIX This fix my issue with unable to set permissions. Also some build issue then GIO not set.
ping
Hi, The GIO part of your patch seems to be already present in the newer upstream code, even if the implementation is a little different there: https://git.xfce.org/xfce/thunar/commit/thunar/thunar-sendto-model.c?id=1f5eed407c762bf7ca118e377f91c6cb1e63eb31 regarding the rest of your patch, you are removing some upstream logic and replacing it with a very simplistic condition. While I believe it works for your use case, I don't feel comfortable changing it for all users in such a way without thorough testing, or at least a good understanding of why the upstream had those more complex conditions. I'll leave this open and try to study the source, time allowing, but it will take some time to figure out. Also, I don't know sshfs, and maybe it's a very specific thing. BTW have you seen the wrong behavior without this patch appear in any other filesystem besides sshfs?
IMHO they dont try build without HAVE_GIO_UNIX: GDesktopAppInfo is part of gio and hey will get undefined struct/type compiler error. They patch does not work (without mine) - 100%. Upstream do many insane and crappy things, I dont want understand why :) I replace effective_user_id == g_file_info_get_attribute_uint32 (file->info, G_FILE_ATTRIBUTE_UNIX_UID) to thunar_file_is_writable(file) All other logic not changed, just refactored to human understanding. You can use both checks via ||, like: return (effective_user_id == g_file_info_get_attribute_uint32 (file->info, G_FILE_ATTRIBUTE_UNIX_UID) || thunar_file_is_writable(file)) May be in some cases there different access levels for file data and metadata and original code will return TRUE is user is file owner or root. I dont know what use cases this cover, probably none. My check is simple: if file writeable than we can write metadata. This is good for most cases. I dont know how to allow user write data and restrict write metadata in unix with chmod(). In my case sshfs mounted to me (simple user), by authorized on remote side as root.
(In reply to rozhuk.im from comment #4) > IMHO they dont try build without HAVE_GIO_UNIX: GDesktopAppInfo is part of > gio and hey will get undefined struct/type compiler error. > They patch does not work (without mine) - 100%. > > Upstream do many insane and crappy things, I dont want understand why :) First of all a general clarification: I can understand but keep in mind that ports are just that, portings. The development should happen upstream and bugfixes should go there where there are developers who know how the existing code works and also usually have better ability to test changes. This kind of changes which diverge from upstream tend to make maintain a port difficult and could also cause regressions which end up being unjustly reported upstream casting shadows on FreeBSD porting methods. I think you understand divergence with upstream should be kept to a minimum. So as a general rule I'm accepting such changes only if they are for very important bugs and have good chance of being merged upstream, or are very FreeBSD specific (which sometimes hinders upstream inclusion). For the GIO part, how do I test in the ports tree with GIO disabled? The port does not have such an option, so it looks like we don't cover this condition either. > > I replace > effective_user_id == g_file_info_get_attribute_uint32 (file->info, > G_FILE_ATTRIBUTE_UNIX_UID) > to > thunar_file_is_writable(file) > > All other logic not changed, just refactored to human understanding. > > You can use both checks via ||, like: > return (effective_user_id == g_file_info_get_attribute_uint32 (file->info, > G_FILE_ATTRIBUTE_UNIX_UID) || thunar_file_is_writable(file)) > > May be in some cases there different access levels for file data and > metadata and original code will return TRUE is user is file owner or root. > I dont know what use cases this cover, probably none. > > My check is simple: if file writeable than we can write metadata. > This is good for most cases. > I dont know how to allow user write data and restrict write metadata in unix > with chmod(). As I said while I hear and understand your explanation I have no explanation about the existing code. I'm not refusing your code, and have no specific objection about it. I just need time to understand it properly. In the while if you can get your code included upstream or at least reviewed by an upstream developer that would greatly help this request. > > In my case sshfs mounted to me (simple user), by authorized on remote side > as root. Not sure I understand, you mean that you are a plain user on the local machine but authenticated as root on the ssh mount on the remote one?
(In reply to Guido Falsi from comment #5) https://bugzilla.xfce.org/show_bug.cgi?id=12863 year+ - no activity in upstream, is there are life? Yes, our port does not have GIO option, I do tests by add to makefile GIO option and uncheck it. I dont know what I need to do with upstream, they do not react on bug tacker. Yes, I mean that Im plain user that have sshfs root@remote mount. PS: miss null check return (thunar_file_is_writable(file) || (NULL != file->info && effective_user_id == g_file_info_get_attribute_uint32 (file->info, G_FILE_ATTRIBUTE_UNIX_UID)))
(In reply to rozhuk.im from comment #6) > (In reply to Guido Falsi from comment #5) > > https://bugzilla.xfce.org/show_bug.cgi?id=12863 > year+ - no activity in upstream, is there are life? This is irrelevant and in fact against your interest to point this out. If a patch is not accepted upstream maybe there are reasons for that. Also the fact the patch is not accepted upstream, even if accepting without question it to be good, is no reason to push it in FreeBSD. It could add unexpected effects. It will also make updating the port more difficult. The patched code could be modified upstream and the patch need reconciliation, or changes upstream can cause the patch to break. > > Yes, our port does not have GIO option, I do tests by add to makefile GIO > option and uncheck it. So the GIO case is irrelevant to our ports tree. No need to diverge to fix a problem that cannot happen. > > > I dont know what I need to do with upstream, they do not react on bug tacker. > While I understand your frustration I have no connection with the upstream. If you get your code accepted by upstream that is proof of a minimum quality of the change and also will mean thee code will anyway get in the ports tree, so there is no reason to hold the change. If you cannot some FreeBSD committer needs to test check and make a decision, this takes time. This is a volunteer project and no one is paid for it or working full time on port bugs. Things may require time. Please give me time to evaluate your code properly. This is a "port" of the upstream code not original code. Any divergence needs a good motivation.
(In reply to rozhuk.im from comment #4) > My check is simple: if file writeable than we can write metadata. > This is good for most cases. > I dont know how to allow user write data and restrict write metadata in unix > with chmod(). Looking at the upstream code and checking how UFS works I can now say this is definitely a wrong assumption. you can chmod any file you own, even if it is not writable, otherwise there would be no way to modify permissions on a file with 0444 permissions. obviously root can change permissions on any file. The upstream code, while a little convoluted, checks just for this, which is correct for local file systems. > > In my case sshfs mounted to me (simple user), by authorized on remote side > as root. Considering the previous correction this use case is quite peculiar and difficult to accommodate. Thunar clearly checks file permissions based on the local user. But the remote user is different, so it gets wrong conclusions. But I cannot see a way to fix this except give thunar detailed knowledge of the specific remote file system and the actual remote user. Anyway your patch as is cannot be accepted since it is actually wrong for the UFS semantics. It will work most of the time but fail in important situations (files with 0444 permissions). Looks like for some specific sshfs semantics thunar_file_is_writable(file) returns true and lets you go ahead. But it looks like a coincidence. I need to find some information on why that checks gives a different result. The only acceptable option would be to add an || thunar_file_is_writable(file) right before the "&& !thunar_file_is_trashed (file));" in the last return in the upstream code. But I still have to evaluate the consequences. I need to study sshfs a little. Your suggeestion about adding an ||
(In reply to Guido Falsi from comment #8) > (In reply to rozhuk.im from comment #4) > > > My check is simple: if file writeable than we can write metadata. > > This is good for most cases. > > I dont know how to allow user write data and restrict write metadata in unix > > with chmod(). > > Looking at the upstream code and checking how UFS works I can now say this > is definitely a wrong assumption. > > you can chmod any file you own, even if it is not writable, otherwise there > would be no way to modify permissions on a file with 0444 permissions. > obviously root can change permissions on any file. > > The upstream code, while a little convoluted, checks just for this, which is > correct for local file systems. > > > > > In my case sshfs mounted to me (simple user), by authorized on remote side > > as root. > > Considering the previous correction this use case is quite peculiar and > difficult to accommodate. > > Thunar clearly checks file permissions based on the local user. But the > remote user is different, so it gets wrong conclusions. But I cannot see a > way to fix this except give thunar detailed knowledge of the specific remote > file system and the actual remote user. > > Anyway your patch as is cannot be accepted since it is actually wrong for > the UFS semantics. It will work most of the time but fail in important > situations (files with 0444 permissions). > > Looks like for some specific sshfs semantics thunar_file_is_writable(file) > returns true and lets you go ahead. But it looks like a coincidence. I need > to find some information on why that checks gives a different result. > > The only acceptable option would be to add an || > thunar_file_is_writable(file) right before the "&& !thunar_file_is_trashed > (file));" in the last return in the upstream code. But I still have to > evaluate the consequences. This is wrong too. thunar_file_is_writable(file) could return true for files you don't own (think write bit on a group you are part of), so on which you can't change attributes. Checking for write permissions to change attributes is unrelated. I don't know a proper way to cater for a remote file system were you access as a different user...especially if that user is root and you are not root locally. If you find a solution please file a new bug report.
False positive - not problem, at least for me. I dont care even if it always return true without any checks. On access deny user will get error on apply, it much better than do not try chmod() because of false-negative.
Unable to change file permissions on sshfs mounted files/dirs - cant be "work as intended".
(In reply to rozhuk.im from comment #10) > False positive - not problem, at least for me. > I dont care even if it always return true without any checks. > > On access deny user will get error on apply, it much better than do not try > chmod() because of false-negative. I agree that having that check return true would work for you, with your simplistic definition of "works". First of all consider that the ports tree is not yours, and all use cases must be kept in consideration. You also must consider that the ports tree contains that, ported software. Original development must be done upstream. If the upstream does a stupid thing the port will also do a stupid thing. The only logical exception is when the stupid thing is caused by a specific FreeBSD difference from other OSes. In that case too at least trying to get a correct patch accepted upstream is a requirement. The upstream code shows that the xfce guys actually don't want to hit system call errors and that's the very reason why they apply such a check. Your patch is anyway definitely wrong and would give false positives and false negatives on UFS and ZFS, since write permissions are completely unrelated to chmod usage. Returning true would be a clear breakage of upstream code. This patch is not acceptable and it looks like your use case is not catered by upstream. So the software is "working as intended", you happen to disagree with the intended way the upstream software should work. regarding comment 11, since you disagree on the "works as intended" resolution I'll change it to a more arbitrary "rejected", so you will have one excuse less to open this up again. I encourage you to push a correct solution with the upstream thunar developers.
(In reply to Guido Falsi from comment #12) > I encourage you to push a correct solution with the upstream thunar > developers. BTW I can review proposed patches but they have to be correct for the common case, which is local file systems, usually UFS and/or ZFS. The thunar guys will care a lot for ext[2-4], btrfs and other common linux file systems. Forcibly allowing a corner case in sshfs by breaking the software for all others by deliberately removing checks the upstream put in the code is definitely not acceptable. (hitting errors in chmod(2) is clearly accounted as breakage by upstream, and I tend to agree with that interpretation)
I'd like to suggest a solution idea: If you can find a way, from inside the thunar_file_get_deletion_date() function to discover with good confidence that you are in a sshfs filesystem and just in that special case return an unconditional true or another check that fits your use case, that could be an acceptable strategy.
Created attachment 188531 [details] patch using statfs(2) Hi again. I thought of a possible solution to this issue. I prepared a patch to the thunar port, adding an option which adds an extra patch. This patches the thunar code to check if the file is on sshfs through the statfs(2) system call. I have only tested building the patch. I will not be able to test this on a running system before the week end. Anyway you could give it a spin and see if it fixes your problem. Thanks.
1. Original code is ureadable die to strange logic. It can be rewriten to: gboolean thunar_file_is_chmodable (const ThunarFile *file) { _thunar_return_val_if_fail (THUNAR_IS_FILE (file), FALSE); /* we can only change the mode if we the euid is * a) equal to the file owner id * or * b) the super-user id * and the file is not in the trash. */ if (thunar_file_is_trashed (file)) return (FALSE); if (effective_user_id == 0) return (TRUE); if (file->info != NULL && effective_user_id == g_file_info_get_attribute_uint32 (file->info, G_FILE_ATTRIBUTE_UNIX_UID)) return (TRUE); return (FALSE); } 2. Only one check really needed: thunar_file_is_trashed (file), all other is waste of time. I dont understand what problem return always true (except thunar_file_is_trashed()), and see error (or nothing?) later? 3. If you want to play around chmod() failsafe then there is more things from statfs can be added, see https://bugs.freebsd.org/bugzilla/attachment.cgi?id=188261&action=diff is_fs_local() + /* Exclude from file changes monitoring, watch only for dirs. */ static const char *non_local_fs[] = { "fusefs.sshfs", NULL }; There is possible also curl and some others fusefs plugins.
(In reply to rozhuk.im from comment #16) > 1. Original code is ureadable die to strange logic. > It can be rewriten to: > > gboolean > thunar_file_is_chmodable (const ThunarFile *file) > { > _thunar_return_val_if_fail (THUNAR_IS_FILE (file), FALSE); > > /* we can only change the mode if we the euid is > * a) equal to the file owner id > * or > * b) the super-user id > * and the file is not in the trash. > */ > if (thunar_file_is_trashed (file)) > return (FALSE); > if (effective_user_id == 0) > return (TRUE); > if (file->info != NULL && > effective_user_id == g_file_info_get_attribute_uint32 (file->info, > > G_FILE_ATTRIBUTE_UNIX_UID)) > return (TRUE); > return (FALSE); > } I agree the original code is ugly, but, as I have already explained, this is just a port, so, as long the original code works, I'm not willing to modify it. The upstream code works for the common case of the local file systems. My proposed patch avoids that code for the sshfs special case in exactly the way you suggest. > > > 2. Only one check really needed: thunar_file_is_trashed (file), all other is > waste of time. > I dont understand what problem return always true (except > thunar_file_is_trashed()), and see error (or nothing?) later? > The problem is that thunar is clearly programmed by the author to not behave in such a way, and does not behave in such a way on other OSes. There is no reason why a port (not original development) should behave differently. Thunar is using this check to choose if the file permissions interface should be enabled. Thunar users expect it to be disabled when they can't change file permissions. It would at least break POLA. I understand you'd like thunar to act in such a way but why should you impose this preference of yours on all FreeBSD ports users? Finally since this is just a porting, there's no reason to intentionally make the port software act differently, in porting we just adapt the software and keep it's strategic choices. You noticed a problem which I agree is a problem and tried to suggest a solution which keeps the standard behaviour and skips it in the way you ask for others. isn't that an acceptable compromise? > > 3. If you want to play around chmod() failsafe then there is more things > from statfs can be added, see > https://bugs.freebsd.org/bugzilla/attachment.cgi?id=188261&action=diff > is_fs_local() > > + > > /* Exclude from file changes monitoring, watch only for dirs. */ > static const char *non_local_fs[] = { > "fusefs.sshfs", > NULL > }; > > There is possible also curl and some others fusefs plugins. Good point. I'll rework my hack so that it uses a list that can be updated if needed.
(In reply to rozhuk.im from comment #16) > 1. Original code is ureadable die to strange logic. > It can be rewriten to: [...] > > There is possible also curl and some others fusefs plugins. BTW you did not tell me if you tested my patch and it did allow you to work on sshfs. I'd like that information, which would confirm or not that the general approach is viable.
(In reply to Guido Falsi from comment #17) 1. I dont like patch bad/ugly code. 2. I do not insist than my patch must be committed to ports. I insist that my patch is best for me. :) I understand and accept that if some one with commit bit here or in upstream not commit my patch then I have no other options except continue use my patch as private patch. No problem. No, I do not test your patch. I think I will update mine to: gboolean thunar_file_is_chmodable (const ThunarFile *file) { _thunar_return_val_if_fail (THUNAR_IS_FILE (file), FALSE); if (thunar_file_is_trashed (file)) return (FALSE); return (TRUE); } and continue use it as private patch.
Being unable to properly test patches for this specific issue alone and the original submitter not willing to test further patches I'm sending this bug report back to the heap.
Keyword: patch or patch-ready – in lieu of summary line prefix: [patch] * bulk change for the keyword * summary lines may be edited manually (not in bulk). Keyword descriptions and search interface: <https://bugs.freebsd.org/bugzilla/describekeywords.cgi>
"if (strncmp(sfs.f_fstypename, "fusefs.sshfs", 12) == 0)" this does not cover: 1. sshfs mounted with RO 2. sshfs mounted with user that does not have perms 3. all other FUSE possible mount types
(In reply to Ivan Rozhuk from comment #22) Problem is thunar is a local file system graphics UI and respect common local UNIX Filesystem semantics. shhfs and many fuse provided filesystems have different semantics and there is no simple way to cater in thunar for all their peculiarities. It is these filesystems responsibility to properly map their semantics to the unix filesystem interface, providing reasonable mapping to the unix flags where possible. SSHFS is problematic because the remote user can be differnet from the local one and have completely different permissions, and the filesystem is not properly translating those in the mounting machine. I'm not sure if SSHFS could r should do things differently, but thunar has no way to know the remote permissions. Simply disabling the functionality is not a real fix. AFAIK thunar does not support extended flags nor ACL style permissions (like what is supported by NFSv4 or NTFS). There is no way to have thunar act correctly in these situations, short of coding a bunch of exceptions based on the type of filesystem, which IMHO is anyway not the correct approach. This is way out of the scope of the ports tree, also because this issue will show up on any operating system. This should be addressed with XFCE developers. But if they were to reply that such exceptions are out of the scope of thunar I would be unable to disagree. Anyway fixing this is beyond my abilities, and adding patches which significantly modify the software behaviour is not appropriate for the ports tree, unless there is certainty the patches will be included (or even better have already been included) upstream.
Ok, I will continue use it as my private patch, and probably try luck with upstream some time later. My patches can be found at: https://github.com/rozhuk-im/freebsd-ports/commits/main
(In reply to Ivan Rozhuk from comment #24) Please note that your patch to `thunar/thunar-file.c` cannot be accepted upstream because it is wrong according to common UNIX filesystem logic. Users can change permission on files they own, even if they have no write permission. The problem you are trying to solve with that patch is not solvable in thunar, in fact it is not file manager job to do that, the problem is with sshfs that fails to properly map remote ownership/permission. Maybe this can be solved in sshfs via configuration o patching it, but there is no definitive solution in thunar, since the information you are trying to guess requires knowledge of the state at the remote end. Either sshfs conveys that in some way via filesystem interfaces or it is completely impossible to solve it the way you'd like. Have you tested with other file managers? If you find one that works "right" (for any definition of right) maybe we can look at what it does and get some ideas. (my interpretation is that other file managers will show similar behaviour to thunar or equally broken but in different ways behaviour, but I could be wrong)
(In reply to Guido Falsi from comment #25) In an unrelated conversation with a friend I also now remembered that thunar has support for the ssh:// schema, which may or may not be a better interface for your use case, but you should test that too.
(In reply to Guido Falsi from comment #26) There is too many time spent to fix simple issue: "enable or disable button" :) It can be always enabled, no one care it. (At least me :) ) It always may fail later on apply, no matter how we check. This is true for all FM on all OS: no one does not check all affected files/subdirs to enable button. PCManFM works better for my usecases, without any additional changes in glib20 and in any code. I give up to fix all Thunar issues, like high CPU usage and freezes on intensive FS/dir add/remove/update. ssh:// - less useful that navigate in mounts tree, since it mounted always on my host. I switch to sshfs in time of FreeBSD 12, because NFS was ugly implemented and some times mountpoints hang until system reboot. I do not like reboot :) Now I have my own SSHFS fork https://github.com/rozhuk-im/sshfs since upstream is dead and I need (time to time) to work with very remote shares (rtt: 60+ms). > Have you tested with other file managers? If you find one that works "right" (for any definition of right) maybe we can look at what it does and get some ideas. PCManFM - I suspect it does not waste of time trying to guess "is chmodable" :) Same for mc.
(In reply to Ivan Rozhuk from comment #27) I understand your frustration but the issues you point out are all very edge cases. As you state sshfs is abandoned upstream, I would not expect other projects to care much for it's peculiarities (most of your problems stem from sshfs not properly mapping remote system information to the local virtual file system it privides. Thunar intentionally implemented logic to know if permissions can be modified (again this is different from writable, please don't mix unrelated things), you can't expect upstream to remove intentional functionality to cater to edge cases. If you really want to improve upstream software you need to propose patches that at least don't change behaviour for the main/most common case (local filesystems, in this case). You could propose a configuration option, but obviously implementing that requires much more work, so I'd investigate with upstream if it is acceptable in principle. Expect the process of getting this kind of change included upstream to take months with various iterations of the patches. There is no other way to get patches, especially big ones like this, accepted in upstream projects without spending this kind of effort. Anyway the ports tree is not the place to perform original development especially to this kind of extent, I'm unable to review such patches, and this would cause the port to diverge too much from upstream.