reftable: adjust permissions of compacted tables
cover reftable: adjust permissions of compacted tables
Hi,
this small patch series fixes a bug in the reftable stack implementation where the "default_permissions" write option is not honored for newly written compacted tables. The effect is that the "core.sharedRepository" config is ignored when performing compaction.
The series runs into two minor conflicts with jc/reftable-core-fsync that can be fixed like follows. As this series has been merged to next already I'd also be happy to rebase these patches on top of it. Please let me know your preference.
Patrick
reftable/stack.c +0 -0
@@@ -633,13 -643,13 +643,13 @@@ int reftable_addition_add(struct reftab err =REFTABLE_IO_ERROR ; goto done; } - if (add->stack->config.default_permissions) { - if (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) { - err =REFTABLE_IO_ERROR ; - goto done; - } + if (add->stack->config.default_permissions && + fchmod(tab_fd, add->stack->config.default_permissions) < 0) { + err =REFTABLE_IO_ERROR ; + goto done; } + - wr =reftable_new_writer (reftable_fd_write, &tab_fd, + wr =reftable_new_writer (reftable_fd_write, reftable_fd_flush, &tab_fd, &add->stack->config); err =write_table (wr, arg);if (err <0 )strbuf_addstr (temp_tab,".temp.XXXXXX" );
tab_fd = mkstemp(temp_tab->buf);
+ if (st->config.default_permissions &&
+ fchmod(tab_fd, st->config.default_permissions) < 0) {
+ err = REFTABLE_IO_ERROR;
+ goto done;
+ }
+
- wr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);
+ wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &st->config);
err = stack_write_compact(st, wr, first, last, config);
if (err < 0)Patrick Steinhardt (2):
reftable/stack: use fchmod(3P) to set permissions reftable/stack: adjust permissions of compacted tables
reftable/stack.c | 35 ++++++++++++++++++++--------------- reftable/stack_test.c | 25 +++++++++++++++++++++++-- 2 files changed, 43 insertions(+), 17 deletions(-)
Signature · 1 line
2.43.GIT
PATCH 1/2 reftable/stack: use fchmod(3P) to set permissions
We use chmod(3P) to modify permissions of "tables.list" locks as well as temporary new tables we're writing. In all of these cases we do have a file descriptor readily available though. So instead of using chmod(3P) we can use fchmod(3P), which should both be more efficient while also avoiding a potential race where we change permissions of the wrong file in case it was swapped out after we have created it.
Refactor the code to do so.
--- reftable/stack.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)
reftable/stack.c +14 -15
@@ -467,11 +467,11 @@ static int reftable_stack_init_addition(struct reftable_addition *add, } goto done; } -if (st->config.default_permissions) { -if (chmod (add->lock_file->filename.buf, st->config.default_permissions) <0 ) { - err =REFTABLE_IO_ERROR ; - goto done; - } +if (st->config.default_permissions && +fchmod (get_tempfile_fd (add->lock_file), + st->config.default_permissions) <0 ) { + err =REFTABLE_IO_ERROR ; + goto done; } err =stack_uptodate (st);
@@ -633,12 +633,12 @@ int reftable_addition_add(struct reftable_addition *add, err =REFTABLE_IO_ERROR ; goto done; } -if (add->stack->config.default_permissions) { -if (chmod (temp_tab_file_name.buf, add->stack->config.default_permissions)) { - err =REFTABLE_IO_ERROR ; - goto done; - } +if (add->stack->config.default_permissions && +fchmod (tab_fd, add->stack->config.default_permissions) <0 ) { + err =REFTABLE_IO_ERROR ; + goto done; } + wr =reftable_new_writer (reftable_fd_write, &tab_fd, &add->stack->config); err =write_table (wr, arg);
@@ -967,11 +967,10 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last, goto done; } have_lock =1 ; -if (st->config.default_permissions) { -if (chmod (lock_file_name.buf, st->config.default_permissions) <0 ) { - err =REFTABLE_IO_ERROR ; - goto done; - } +if (st->config.default_permissions && +fchmod (lock_file_fd, st->config.default_permissions) <0 ) { + err =REFTABLE_IO_ERROR ; + goto done; } format_name(&new_table_name, st->readers[first]->min_update_index,
Signature · 1 line
2.43.GIT
PATCH 2/2 reftable/stack: adjust permissions of compacted tables
In reply to Patrick SteinhardtWhen creating a new compacted table from a range of preexisting ones we don't set the default permissions on the resulting table when specified by the user. This has the effect that the "core.sharedRepository" config will not be honored correctly.
Fix this bug and add a test to catch this issue.
--- reftable/stack.c | 6 ++++++ reftable/stack_test.c | 25 +++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 2 deletions(-)
reftable/stack.c +6 -0
@@ -731,6 +731,12 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,strbuf_addstr (temp_tab,".temp.XXXXXX" ); tab_fd =mkstemp (temp_tab->buf); +if (st->config.default_permissions && +fchmod (tab_fd, st->config.default_permissions) <0 ) { + err =REFTABLE_IO_ERROR ; + goto done; + } + wr =reftable_new_writer (reftable_fd_write, &tab_fd, &st->config); err =stack_write_compact (st, wr, first, last, config);
reftable/stack_test.c +23 -2
@@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)int err =0 ;struct reftable_write_options cfg = { .exact_log_message =1 , + .default_permissions =0660 , };struct reftable_stack *st =NULL ;char *dir =get_tmp_dir (__LINE__); -struct reftable_ref_record refs[2 ] = { {NULL } };struct reftable_log_record logs[2 ] = { {NULL } }; +struct strbuf scratch =STRBUF_INIT ; +struct stat stat_result;int N =ARRAY_SIZE (refs); - err =reftable_new_stack (&st, dir, cfg);EXPECT_ERR (err); st->disable_auto_compact =1 ;
@@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)reftable_log_record_release (&dest); } +#ifndef GIT_WINDOWS_NATIVE +strbuf_addstr (&scratch, dir); +strbuf_addstr (&scratch,"/tables.list" ); + err =stat (scratch.buf, &stat_result); +EXPECT (!err); +EXPECT ((stat_result.st_mode &0777 ) == cfg.default_permissions); + +strbuf_reset (&scratch); +strbuf_addstr (&scratch, dir); +strbuf_addstr (&scratch,"/" ); +/* do not try at home; not an external API for reftable. */ +strbuf_addstr (&scratch, st->readers[0 ]->name); + err =stat (scratch.buf, &stat_result); +EXPECT (!err); +EXPECT ((stat_result.st_mode &0777 ) == cfg.default_permissions); +#else + (void ) stat_result; +#endif +/* cleanup */ reftable_stack_destroy (st);for (i =0 ; i <N ; i++) {reftable_ref_record_release (&refs[i]);reftable_log_record_release (&logs[i]); } +strbuf_release (&scratch);clear_dir (dir); }
Signature · 1 line
2.43.GIT
PATCH v2 1/1 reftable/stack: adjust permissions of compacted tables
In reply to Patrick SteinhardtWhen creating a new compacted table from a range of preexisting ones we don't set the default permissions on the resulting table when specified by the user. This has the effect that the "core.sharedRepository" config will not be honored correctly.
Fix this bug and add a test to catch this issue.
---
Ugh, I totally forgot about the fact that fchmod(3P) isn't actually available on Windows. I've thus evicted the first patch and changed the second patch to use chmod(3P) instead. Too bad.
reftable/stack.c | 6 ++++++ reftable/stack_test.c | 25 +++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 2 deletions(-)
reftable/stack.c +6 -0
@@ -731,6 +731,12 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,strbuf_addstr (temp_tab,".temp.XXXXXX" ); tab_fd =mkstemp (temp_tab->buf); +if (st->config.default_permissions && +chmod (temp_tab->buf, st->config.default_permissions) <0 ) { + err =REFTABLE_IO_ERROR ; + goto done; + } + wr =reftable_new_writer (reftable_fd_write, &tab_fd, &st->config); err =stack_write_compact (st, wr, first, last, config);
reftable/stack_test.c +23 -2
@@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)int err =0 ;struct reftable_write_options cfg = { .exact_log_message =1 , + .default_permissions =0660 , };struct reftable_stack *st =NULL ;char *dir =get_tmp_dir (__LINE__); -struct reftable_ref_record refs[2 ] = { {NULL } };struct reftable_log_record logs[2 ] = { {NULL } }; +struct strbuf scratch =STRBUF_INIT ; +struct stat stat_result;int N =ARRAY_SIZE (refs); - err =reftable_new_stack (&st, dir, cfg);EXPECT_ERR (err); st->disable_auto_compact =1 ;
@@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)reftable_log_record_release (&dest); } +#ifndef GIT_WINDOWS_NATIVE +strbuf_addstr (&scratch, dir); +strbuf_addstr (&scratch,"/tables.list" ); + err =stat (scratch.buf, &stat_result); +EXPECT (!err); +EXPECT ((stat_result.st_mode &0777 ) == cfg.default_permissions); + +strbuf_reset (&scratch); +strbuf_addstr (&scratch, dir); +strbuf_addstr (&scratch,"/" ); +/* do not try at home; not an external API for reftable. */ +strbuf_addstr (&scratch, st->readers[0 ]->name); + err =stat (scratch.buf, &stat_result); +EXPECT (!err); +EXPECT ((stat_result.st_mode &0777 ) == cfg.default_permissions); +#else + (void ) stat_result; +#endif +/* cleanup */ reftable_stack_destroy (st);for (i =0 ; i <N ; i++) {reftable_ref_record_release (&refs[i]);reftable_log_record_release (&logs[i]); } +strbuf_release (&scratch);clear_dir (dir); }
base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c
Signature · 1 line
2.43.GIT
Re: [PATCH v2] reftable/stack: adjust permissions of compacted tables
Quoted 5 lines · Patrick Steinhardt
When creating a new compacted table from a range of preexisting ones we don't set the default permissions on the resulting table when specified by the user. This has the effect that the "core.sharedRepository" config will not be honored correctly.
Quoted 9 lines
Fix this bug and add a test to catch this issue.
Signed-off-by Patrick Steinhardt <[email protected]>---
Ugh, I totally forgot about the fact that fchmod(3P) isn't actually available on Windows. I've thus evicted the first patch and changed the second patch to use chmod(3P) instead. Too bad.
Good catch. This version looks good to me! Nothing to add :)
Re: [PATCH v2] reftable/stack: adjust permissions of compacted tables
In reply to Patrick SteinhardtQuoted 7 lines · Patrick Steinhardt
reftable/stack_test.c +0
-0@@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)int err =0 ;
reftable/stack_test.c static void test_reftable_stack_add(void) · line 452
Quoted 10 lines
reftable/stack_test.c +2
-1struct reftable_write_options cfg = { .exact_log_message =1 , + .default_permissions =0660 , };struct reftable_stack *st =NULL ;char *dir =get_tmp_dir (__LINE__); -struct reftable_ref_record refs[2 ] = { {NULL } };struct reftable_log_record logs[2 ] = { {NULL } }; +struct strbuf scratch =STRBUF_INIT ;
The variable name scratch seems rather vague to me as opposed to something like path. After a quick search though, scratch appears to be a fairly common name used in similar scenarios. So probably not a big deal, but something
I thought I'd mention.
Quoted 19 lines
reftable/stack_test.c +9
-1+struct stat stat_result;int N =ARRAY_SIZE (refs); - err =reftable_new_stack (&st, dir, cfg);EXPECT_ERR (err); st->disable_auto_compact =1 ;@@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)reftable_log_record_release (&dest); } +#ifndef GIT_WINDOWS_NATIVE +strbuf_addstr (&scratch, dir); +strbuf_addstr (&scratch,"/tables.list" ); + err =stat (scratch.buf, &stat_result); +EXPECT (!err); +EXPECT ((stat_result.st_mode &0777 ) == cfg.default_permissions); + +strbuf_reset (&scratch);
reftable/stack_test.c static void test_reftable_stack_add(void) · line 530
Quoted 10 lines
reftable/stack_test.c +10
-0+strbuf_addstr (&scratch, dir); +strbuf_addstr (&scratch,"/" ); +/* do not try at home; not an external API for reftable. */ +strbuf_addstr (&scratch, st->readers[0 ]->name); + err =stat (scratch.buf, &stat_result); +EXPECT (!err); +EXPECT ((stat_result.st_mode &0777 ) == cfg.default_permissions); +#else + (void ) stat_result; +#endif
Why do we ignore Windows here? And would it warrant explaining in the commit message?
-Justin
Re: [PATCH v2] reftable/stack: adjust permissions of compacted tables
Quoted 16 lines · Justin Tobler
Quoted 15 lines · Patrick Steinhardt
reftable/stack_test.c +1
-1@@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)int err =0 ;struct reftable_write_options cfg = { .exact_log_message =1 , + .default_permissions =0660 , };struct reftable_stack *st =NULL ;char *dir =get_tmp_dir (__LINE__); -struct reftable_ref_record refs[2 ] = { {NULL } };
Quoted 8 lines
reftable/stack_test.c static void test_reftable_stack_add(void) · line 452
Quoted 2 lines
reftable/stack_test.c +1
-0struct reftable_log_record logs[2 ] = { {NULL } }; +struct strbuf scratch =STRBUF_INIT ;The variable name
scratchseems rather vague to me as opposed to something likepath. After a quick search though,scratchappears to be a fairly common name used in similar scenarios. So probably not a big deal, but something
I thought I'd mention.
Yeah. I basically copied the below checks from another test where we already had the permission checks, and also adopted the name of the scratch variable. I agree though that path would be a better name, so let me change it.
Quoted 24 lines
Quoted 24 lines
reftable/stack_test.c +14
-1+struct stat stat_result;int N =ARRAY_SIZE (refs); - err =reftable_new_stack (&st, dir, cfg);EXPECT_ERR (err); st->disable_auto_compact =1 ;@@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)reftable_log_record_release (&dest); } +#ifndef GIT_WINDOWS_NATIVE +strbuf_addstr (&scratch, dir); +strbuf_addstr (&scratch,"/tables.list" ); + err =stat (scratch.buf, &stat_result); +EXPECT (!err); +EXPECT ((stat_result.st_mode &0777 ) == cfg.default_permissions); + +strbuf_reset (&scratch); +strbuf_addstr (&scratch, dir); +strbuf_addstr (&scratch,"/" ); +/* do not try at home; not an external API for reftable. */ +strbuf_addstr (&scratch, st->readers[0 ]->name); + err =stat (scratch.buf, &stat_result);
Quoted 8 lines
reftable/stack_test.c static void test_reftable_stack_add(void) · line 530
Quoted 5 lines
reftable/stack_test.c +5
-0+EXPECT (!err); +EXPECT ((stat_result.st_mode &0777 ) == cfg.default_permissions); +#else + (void ) stat_result; +#endif Why do we ignore Windows here? And would it warrant explaining in the commit message?
Because Windows has a different acccess control model for files and doesn't natively use POSIX permissions. I'm not a 100% sure whether we do emulate the permission bits or not, but I cannot test on Windows and the other test where this was ripped out of also makes the code conditional.
Will explain in the commit message.
Thanks!
Patrick
PATCH v3 1/1 reftable/stack: adjust permissions of compacted tables
In reply to Patrick SteinhardtWhen creating a new compacted table from a range of preexisting ones we don't set the default permissions on the resulting table when specified by the user. This has the effect that the "core.sharedRepository" config will not be honored correctly.
Fix this bug and add a test to catch this issue. Note that we only test on non-Windows platforms because Windows does not use POSIX permissions natively.
---
Changes compared to v2:
- Extended the commit message to say why we don't test on Windows systems.
- Renamed the scratch variable to path.
Thanks!
Patrick
reftable/stack.c | 6 ++++++ reftable/stack_test.c | 25 +++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 2 deletions(-)
reftable/stack.c +6 -0
@@ -731,6 +731,12 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,strbuf_addstr (temp_tab,".temp.XXXXXX" ); tab_fd =mkstemp (temp_tab->buf); +if (st->config.default_permissions && +chmod (temp_tab->buf, st->config.default_permissions) <0 ) { + err =REFTABLE_IO_ERROR ; + goto done; + } + wr =reftable_new_writer (reftable_fd_write, &tab_fd, &st->config); err =stack_write_compact (st, wr, first, last, config);
reftable/stack_test.c +23 -2
@@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)int err =0 ;struct reftable_write_options cfg = { .exact_log_message =1 , + .default_permissions =0660 , };struct reftable_stack *st =NULL ;char *dir =get_tmp_dir (__LINE__); -struct reftable_ref_record refs[2 ] = { {NULL } };struct reftable_log_record logs[2 ] = { {NULL } }; +struct strbuf path =STRBUF_INIT ; +struct stat stat_result;int N =ARRAY_SIZE (refs); - err =reftable_new_stack (&st, dir, cfg);EXPECT_ERR (err); st->disable_auto_compact =1 ;
@@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)reftable_log_record_release (&dest); } +#ifndef GIT_WINDOWS_NATIVE +strbuf_addstr (&path, dir); +strbuf_addstr (&path,"/tables.list" ); + err =stat (path.buf, &stat_result); +EXPECT (!err); +EXPECT ((stat_result.st_mode &0777 ) == cfg.default_permissions); + +strbuf_reset (&path); +strbuf_addstr (&path, dir); +strbuf_addstr (&path,"/" ); +/* do not try at home; not an external API for reftable. */ +strbuf_addstr (&path, st->readers[0 ]->name); + err =stat (path.buf, &stat_result); +EXPECT (!err); +EXPECT ((stat_result.st_mode &0777 ) == cfg.default_permissions); +#else + (void ) stat_result; +#endif +/* cleanup */ reftable_stack_destroy (st);for (i =0 ; i <N ; i++) {reftable_ref_record_release (&refs[i]);reftable_log_record_release (&logs[i]); } +strbuf_release (&path);clear_dir (dir); }
base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c
Signature · 1 line
2.43.GIT
Re: [PATCH v3] reftable/stack: adjust permissions of compacted tables
Quoted 11 lines · Patrick Steinhardt
When creating a new compacted table from a range of preexisting ones we don't set the default permissions on the resulting table when specified by the user. This has the effect that the "core.sharedRepository" config will not be honored correctly.
Fix this bug and add a test to catch this issue. Note that we only test on non-Windows platforms because Windows does not use POSIX permissions natively.
Signed-off-by Patrick Steinhardt <[email protected]>
Quoted 8 lines
---
Changes compared to v2:
- Extended the commit message to say why we don't test on Windows systems.
- Renamed the
scratchvariable topath.
Thanks Patrick! This version looks good to me. I have nothing else to add
-Justin