"Konrad Bucheli (PSI)" <konrad.bucheli@xxxxxx> 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?
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?
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 */
