Re: chmod failure on GVFS mounted CIFS share
- From
- Konrad Bucheli (PSI) <konrad.bucheli@psi.ch>
- Date
- Dec 20, 2024, 15:50 UTC
- Message-ID
- <8af7ff11-c5bb-464b-894f-f1fb30df50e0@psi.ch>
- In-Reply-To
- <xmqq8qsafjfy.fsf@gitster.g>
On 20.12.24 16:44, Junio C Hamano wrote:
Show 9 quoted lines
> "Konrad Bucheli (PSI)" <konrad.bucheli@psi.ch> writes: > >> I have another idea: there is no need for a chmod if both the config >> file and the lock file already have he same mode. Which is the case if >> the filesystem has no proper chmod support. > > And for majority of people who have working chmod(), would it mean > one extra and unnecessary system call? >
I do not have stats, but I guess the chmod call would be needed very rarely as most of the time both files have default permissions. I do not know which call is more expensive.
Show 19 quoted lines
> Instead, how about doing the chmod() first, like we have always done,
> but after seeing it fail, check with lstat() to see if the modes are
> already in the desired state and refrain from complaining if that is
> the case? That way, we'll incur extra overhead only in the error
> code path, which is the usual pattern we would prefer to do things.
>
> So instead of removing this part, ...
>
>> - if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
>> - error_errno(_("chmod on %s failed"), get_lock_file_path(&lock));
>
> ... you'd do an extra lstat() on the lock file (so you can move the
> st_lock inside this block, narrowing its scope) before calling
> error_errno(), and only after finding out that st_lock cannot
> somehow be obtained or the resulting mode is different, you call
> error_errno() and arrange an error to be returned, all inside the
> if(){} block of the original.
>
> Wouldn't it work even better, I wonder?If you think that is the way to go, I will adapt the patch.
Show 21 quoted lines
>
> Thanks.
>
>> + if (stat(get_lock_file_path(&lock), &st_lock) == -1) {
>> + error_errno(_("stat on %s failed"), get_lock_file_path(&lock));
>> ret = CONFIG_NO_WRITE;
>> goto out_free;
>> }
>>
>> + if ((st.st_mode & 07777) != (st_lock.st_mode & 07777)) {
>> + if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
>> + error_errno(_("chmod on %s failed"), get_lock_file_path(&lock));
>> + ret = CONFIG_NO_WRITE;
>> + goto out_free;
>> + }
>> + }
>> +
>> if (store.seen_nr == 0) {
>> if (!store.seen_alloc) {
>> /* Did not see key nor section */
>