Opened 2 years ago

Closed 2 years ago

#25471 closed enhancement (implemented)

"log to pt_state directory" option for snowflake-client

Reported by: dcf Owned by:
Priority: Medium Milestone:
Component: Circumvention/Snowflake Version:
Severity: Normal Keywords:
Cc: dcf, arlolra Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


From #25449:


My removal of default logging in 12922a232b was arguably rash: while obviously eventually we would have to stop logging by default, requiring a -log option with a path prevents writing to a log file that's inside the pt_state directory, which, in sandbox situations, may be the only place the transport plugin is allowed to write. obfs4proxy just has a boolean option -enableLogging that doesn't take a path, unconditionally writing to pt_state/obfs4proxy.log. Perhaps something like that would be better, even if it's less convenient for development.


Maybe just add another flag, -logToStateDir or the like

Child Tickets

Attachments (1)

0001-Log-to-state-dir.patch (1.3 KB) - added by arlolra 2 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 2 years ago by dcf

Status: newneeds_review

arlolra's patch:

--- a/client/snowflake.go
+++ b/client/snowflake.go
@@ -10,6 +10,7 @@ import (
+       "path"
@@ -126,12 +127,22 @@ func main() {
        brokerURL := flag.String("url", "", "URL of signaling broker")
        frontDomain := flag.String("front", "", "front domain")
        logFilename := flag.String("log", "", "name of log file")
+       logToStateDir := flag.Bool("logToStateDir", false, "supersedes log file")
        max := flag.Int("max", DefaultSnowflakeCapacity,
                "capacity for number of multiplexed WebRTC peers")
        log.SetFlags(log.LstdFlags | log.LUTC)
+       if *logToStateDir {
+               stateDir, err := pt.MakeStateDir()
+               if err != nil {
+                       log.Fatal(err)
+               }
+               *logFilename = path.Join(stateDir, "snowflake.log")
+       }
        if *logFilename != "" {
                logFile, err := os.OpenFile(*logFilename,
                        os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0600)

comment:2 Changed 2 years ago by dcf

Status: needs_reviewmerge_ready

I don't want to bikeshed too hard, but one other possibility is to have the option take an argument that's the name of the file to create within the state directory. Like -logToStateDir snowflake.log.

But, ehh, on second thought, it's just my tendency to overcomplicate talking--realistically speaking we're never going to have filename collisions, it just makes the command line longer, fixing "snowflake.log" makes it easier to interact with users, so let's just go ahead with it as is :)

comment:3 Changed 2 years ago by arlolra

I don't want to bikeshed too hard


What if we just made -logToStateDir mean resolving the log file relative to the state dir,

path.Join(stateDir, *logFilename)

So, you'd have to do,

-log snowflake.log -logToStateDir

Changed 2 years ago by arlolra

Attachment: 0001-Log-to-state-dir.patch added

comment:4 Changed 2 years ago by arlolra

Status: merge_readyneeds_review

comment:5 Changed 2 years ago by dcf

Looks fine, except for one thing: I think it's better to use path/filepath (i.e. filepath.Join) instead of path, because the former will use backslashes on Windows.

comment:6 Changed 2 years ago by arlolra

Resolution: implemented
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.