poolpOrg / filter-senderscore

OpenSMTPD filter integration for the SenderScore reputation
ISC License
25 stars 9 forks source link

Refactor message printing #6

Closed lfos closed 4 years ago

lfos commented 4 years ago

Factor out the code for handling legacy versions of the protocol. Even if this is only temporary, I think it makes sense to create a simple abstraction layer here.

poolpOrg commented 4 years ago

I agree a wrapper function needs to be implemented, I didn't because I had to fix three filters before a commit was made to OpenBSD that would break them and I didn't have a clear idea how this should be done.

I'm not too familiar with interface{} so the diff is hard to read for me at this point, I need to get educated first, but surely we could provide the two or three filter response functions needed and hide the version check in them.

Give me a couple days to study a bit the issue and see if I'm ok with your solution :-)

lfos commented 4 years ago

Sounds good. The alternative implementation mentioned above would look like this (untested):

func printMsg(msgType string, sessionId string, token string, format string, a ...interface{}) {
    if version < "0.5" {
        fmt.Printf("%s|%s|%s|", msgType, token, sessionId)
    } else {
        fmt.Printf("%s|%s|%s|", msgType, sessionId, token)
    }

    fmt.Printf(format, a...)
    fmt.Println()
}

It is arguably easier to read and avoids messing with the format string but comes at the expense of multiple Printf/Println calls. I'd be happy to implement it either way.

lfos commented 4 years ago

Another aspect: having three separate calls to print might increase the chances of having mangled output due to them being called from different goroutines. But I think that this issue exists even with the current implementation since fmt.Printf is not guaranteed to be atomic. I will open a separate issue to discuss this and propose solutions.

lfos commented 4 years ago

I think I found an elegant solution that refactors this code in a way that avoids messing with the format string and fixes the atomicity issue at the same time with only a few lines of additional code. Will send a new PR to supersede this one.