Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Server & Client: Implementing DSN extension (RFC 3461, RFC 6533) #233

Merged
merged 2 commits into from
Nov 3, 2023
Merged

Server & Client: Implementing DSN extension (RFC 3461, RFC 6533) #233

merged 2 commits into from
Nov 3, 2023

Conversation

ikedas
Copy link
Collaborator

@ikedas ikedas commented Sep 10, 2023 •
edited by emersion
Loading

Closes #32.

@ikedas
Copy link
Collaborator Author

ikedas commented Sep 11, 2023 •
edited
Loading

Some notes:

  • This PR depends on Fix broken encodeXtext() #232.
  • If invalid parameter values found in MAIL or RCPT command, reply-code 501 is issued (See RFC3641, 5.1), though for the other parameters currently 500 is issued.
  • Test coverage is not adequate.
  • IMHO encodeXtext(), encodeUTF8AddrXtext() and encodeUTF8AddrUnitext() would be better to be public function, since they may be useful for the other tasks such as generating DSN/MDN messages.

@ikedas ikedas marked this pull request as ready for review September 11, 2023 03:51
emersion
emersion reviewed Oct 10, 2023
View reviewed changes
emersion
emersion reviewed Oct 10, 2023
View reviewed changes
emersion
emersion reviewed Oct 10, 2023
View reviewed changes
@ikedas ikedas force-pushed the dsn_extension-try2 branch 2 times, most recently from 76f990b to fd8acd9 Compare October 18, 2023 03:53
@ikedas
Copy link
Collaborator Author

ikedas commented Oct 18, 2023

I've done what I could.

emersion
emersion reviewed Oct 31, 2023
View reviewed changes
smtp.go Outdated
Comment on lines 69 to 83
type DSNNotify string

const (
NotifyNever DSNNotify = "NEVER"
NotifyDelayed DSNNotify = "DELAY"
NotifyFailure DSNNotify = "FAILURE"
NotifySuccess DSNNotify = "SUCCESS"
)

type DSNAddressType string

const (
AddressTypeRFC822 DSNAddressType = "RFC822"
AddressTypeUTF8 DSNAddressType = "UTF-8"
)
Copy link
Owner

Choose a reason for hiding this comment

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

It seems a bit inconsistent to have the types prefixed with "DSN" but not the constants. Is this deliberate? Should we prefix everything with "DSN", or do you think we can drop the "DSN" prefix from type names?

(Sorry for the delay!)

Copy link
Collaborator Author

@ikedas ikedas Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

These follow the naming conventions adopted by @foxcpp in #135. I have no particular preference. Which do you prefer?

Copy link
Owner

Choose a reason for hiding this comment

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

I've went with the prefix everywhere.

emersion
emersion reviewed Oct 31, 2023
View reviewed changes
conn.go
// present. It also matches disallowed characters in QCHAR and QUCHAR defined
// in above.
// So it allows us to detect malformed values and report them appropriately.
var eUOrDCharRe = regexp.MustCompile(`\\x[{][0-9A-F]+[}]|[[:cntrl:] \\+=]`)
Copy link
Owner

Choose a reason for hiding this comment

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

In general I'm not a fan of using regexp for decoding but we already do that in go-smtp so it's not a big deal…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not a fan too, but could't think of a better way.

emersion
emersion reviewed Oct 31, 2023
View reviewed changes
client.go Outdated
cmdStr := "MAIL FROM:<%s>"

var sb strings.Builder
sb.Grow(510 + 14 + 26 + 11 + 9 + 9 + 39 + 500)
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, this seems a bit like micro-optimization with these magic constants... I'd prefer to leave this out or just pick a reasonably high enough power of 2 value.

@emersion
Copy link
Owner

These are mostly just nits, I think this is overall good to go.

emersion
emersion approved these changes Nov 3, 2023
View reviewed changes
Copy link
Owner

@emersion emersion left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@emersion emersion merged commit faeb4b1 into emersion:master Nov 3, 2023
@emersion emersion mentioned this pull request Nov 3, 2023
Closed
@ikedas ikedas deleted the dsn_extension-try2 branch November 3, 2023 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersion emersion emersion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DSN support
2 participants
@ikedas @emersion