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:

rsync/rsyncd/rsyncd.go

Line 9 in de1cff5

"log"

And it's calling fatal during the execution:

rsync/rsyncd/filoio.go

Lines 104 to 105 in c271006

	// TODO: zero the buffer, file has changed mid-transfer
	log.Fatalf("file has changed mid-transfer")

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