logger usage in `rsyncd` package

[bcho]

Hey, thanks for creating this great package! I am trying to import the rsyncd package in one of my project. I am trying to create the rsyncd server directly. However, seems like the rsyncd server is using the builtin logger package:

https://github.com/gokrazy/rsync/blob/de1cff573bd22217f6d5669b89a70f84c3a51829/rsyncd/rsyncd.go#L9

And it's calling fatal during the execution:

https://github.com/gokrazy/rsync/blob/c271006f2d3fa756367a856165d349862ae44aa6/rsyncd/filoio.go#L104-L105

Is that possible to:

1. define a logger interface to be used inside the server
2. expose the error handle in above code (I didn't have time to go through the code, but I think we should be able to fix it to avoid panic)

[stapelberg]

Yeah, in general we should try to eliminate any log.Fatal calls.

The one you link to can be removed once the TODO is addressed.

I don’t think we need to introduce an interface, instead we can remove the calls.

[bcho]

Thanks for your reply! In my project, I want to replace the logger to adjust the logging behavior. Without using a logger interface, how can we archive that?

[stapelberg]

Oh, I think I read your 2 questions as 1 question.

For log.Printf calls (which should remain), we can introduce an interface.

For log.Fatal calls, we should remove the calls.

[bcho]

I did some basic searches on the code base, seems like we are using log's global logger directly across the code base. So to make the conversion easier, I propose to convert the code base with following steps:

1. create a new package github.com/gokrazy/log to abstract out the logging interface
2. create a logging interface with following method:

type Logger interface {
  Printf(msg string, a ...interface{})
}

3. create a hook function for setting up the global logger
4. replace those log.Fatalf calls with internal.log.Printf + os.Exit(1)

I also see there is a plan for using structured logging in #5 , so I think in the future, with the logging interface, we should be able to:

1. add new methods for branching out the logger like what logrus/logr/zap do to add tag
2. create separated logger instances for each server instance (and other sub-routines) instead of using a global logger

What do you think of this proposal? I can help create the package and replace the existing logging calls.

[bcho]

I created a draft PR in #20, PTAL

[stapelberg]

This issue can be closed now since your PR #20 was merged, right? Or is there anything specific left to do?

[bcho]

Yeah let’s close it. I will open other prs if further changes needed