Skip to content

Conversation

@SLR9511
Copy link
Contributor

@SLR9511 SLR9511 commented Jul 3, 2018

No description provided.

@cviecco cviecco requested a review from rgooch August 27, 2018 22:31
"sync"

"github.com/Symantec/Dominator/lib/log"
// "github.com/Symantec/keymaster/eventmon/tee_logger"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove commented-out lines.

ErrorKeymasterDaemonNotReady = errors.New("keymasterd not ready")
One = systeelogger.New()
Two = nulllogger.New() //remain for the future use
Teelog teelogger.Logger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless these need to be exported, please make these private to the package.

webLoginChannel := make(chan string, bufferLength)
x509RawCertChannel := make(chan []byte, bufferLength)
x509CertChannel := make(chan *x509.Certificate, bufferLength)
Teelog := teelogger.New(One, Two)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's cleaner to create the two loggers here and then create the teelogger.

x509RawCertChannel := make(chan []byte, bufferLength)
x509CertChannel := make(chan *x509.Certificate, bufferLength)
Teelog := teelogger.New(One, Two)
defer One.Close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will cause system logging to fail once you return from this function.

keymasterStatus: make(map[string]error),
}
go monitor.monitorForever(logger)
go monitor.monitorForever(logger, Teelog)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you adding a second logger parameter? I was expecting that the teelogger would replace the logger in the parameter list.
What is the goal? To send all logs to both loggers? Or is that not the case?

@@ -0,0 +1,39 @@
package systeelogger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a teelogger. I suggest calling this package "syslogger".


const (
priority = syslog.LOG_AUTHPRIV
log_name = "keymaster"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should be passed in when creating the syslogger.

@rgooch
Copy link
Contributor

rgooch commented Aug 31, 2018

@cviecco: Please also take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants