status-im / status-go

The Status module that consumes go-ethereum
https://status.im
Mozilla Public License 2.0
727 stars 245 forks source link

SetLogLevel geth.log permission denied regression #482

Closed oskarth closed 6 years ago

oskarth commented 6 years ago

Problem

status-go logs stopped being written to geth.log when running status-react from iOS Simulator.

Implementation

Fix permission denied in SetLogLevel.

Acceptance Criteria

Logs persisted to geth.log showing up in device filesystem.

Notes

There is a confusion about where responsibility lies for filesystem operations and layout.

status-react creates log file and directory for status-go, see https://github.com/status-im/status-react/blob/f4082951b7eeffcde5a7e5ff90331e46e55ed8fd/modules/react-native-status/ios/RCTStatus/RCTStatus.m#L161-L199. This was introduced as part of introducing network switching in https://github.com/status-im/status-react/commit/cf7a9845c6083b8c621a20187f2ed82ee6fe3a1e#diff-ac58c458dd48cf68b223cde6acfdbd5e. From what I can gather, idea is that we have aa directory for each network configuration where we store logs and possibly chaindata (?), e.g.: ./Documents/ethereum/testnet_rpc/geth.log

On the other hand, status-go also creates file it doesn't exist via SetLogFile and log.FileHandler (go-ethereum log):

// FileHandler returns a handler which writes log records to the give file
// using the given format. If the path
// already exists, FileHandler will append to the given file. If it does not,
// FileHandler will create the file with mode 0644.
func FileHandler(path string, fmtr Format) (Handler, error) {
    f, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0644)
    if err != nil {
        return nil, err
    }
    return closingHandler{f, StreamHandler(f, fmtr)}, nil
}

It is unclear to me if this also applies to directories, but I suspect not. It also isn't clear to me what expectationsSetLogFile have for CWD or environment, such as specific network configurations.

It would be a good idea to disambiguate the various terms we use here: file, path (relative and absolute as well) and url (in RCTStatus.m so we know what refers to what.

For the corresponding issue on status-react side, see: https://github.com/status-im/status-react/issues/2534. Note by the file sizes that this used to work a few months ago with logFile geth.log and logLevel DEBUG (same as now).

Future Steps

Resolve confusion alluded to in notes above.

Logs

With the following patch:

Oskars-MBP:status-go oskarth$ cat log-log-debug.patch
diff --git a/geth/log/log.go b/geth/log/log.go
index 0b494cb7..3dd5a9ac 100644
--- a/geth/log/log.go
+++ b/geth/log/log.go
@@ -78,8 +78,16 @@ func SetLevel(level string) {
 // SetLogFile configures logger to write output into file.
 // This call preserves current logging level.
 func SetLogFile(filename string) error {
+   dir, err2 := os.Getwd()
+   if err2 != nil {
+       fmt.Println("SetLogFile err getwd:", err2)
+   }
+   fmt.Println("SetLogFile filename: ", filename)
+   fmt.Println("SetLogFile cwd: ", dir)
+
    handler, err := log.FileHandler(filename, log.TerminalFormat(false))
    if err != nil {
+       fmt.Println("SetLogFile err filehandler:", err)
        return err
    }

We get a permission denied:

SetLogFile filename:  geth.log
SetLogFile cwd:  /
SetLogFile err filehandler: open geth.log: permission denied
Failed to open log file, using stdout:  open geth.log: permission denied
oskarth commented 6 years ago

https://github.com/status-im/status-react/issues/2534#issuecomment-347597671

divan commented 6 years ago

@oskarth so the problem is in fact that actual working directory is / and os.OpenFile() doesn't do any magic behind the scenes — it just opens a file by given path, relative or absolute. It's technically possible to change working dir from within Go code (using os.Chdir()), but it's a suboptimal solution and I believe the proper fix would be to set working directory correctly in iOS simulator scripts/code.

oskarth commented 6 years ago

@divan I understand, but again, why did it work before? This is a regression.

divan commented 6 years ago

why did it work before?

Any chance to check that it really did?

AFAIK, there were no changes in log-related code since August. Maybe there were changes in status-react or scripting code for running in iOS simulator, maybe Xcode update broke something, I don't know. But currently, I'm pretty sure that this regression introduced not by changes in status-go.

oskarth commented 6 years ago

Fixed in status-react develop.