Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape unix socket group in unit tests #1554

Merged
merged 1 commit into from
Jan 14, 2025
Merged

Escape unix socket group in unit tests #1554

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion tests/unit/other.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,8 @@ if {$::verbose} {
set tempFileId [open $tempFileName w]
set group [dict get [file attributes $tempFileName] -group]
if {$group != ""} {
start_server [list tags {"repl external:skip"} overrides [list unixsocketgroup $group unixsocketperm 744]] {
set escaped_group "\"[string map {"\\" "\\\\"} $group]\""
start_server [list tags {"repl external:skip"} overrides [list unixsocketgroup $escaped_group unixsocketperm 744]] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be more correct to do the escaping inside start_server so it works everywhere and for all options?

(proc start_server is defined in tests/support/server.tcl.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes. Do we have a list of all string options that we should escape?
Could we try to escape any string option if we don't have such list?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. :) I think some investigation is needed to find out the best solution here. Now it's already merged so let's not worry too much about it.

test {test unixsocket options are set correctly} {
set socketpath [lindex [r config get unixsocket] 1]
set attributes [file attributes $socketpath]
Expand Down
Loading