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

Improved status checking in mail.c in accordance to RFC requirements #332

Open
wants to merge 2 commits into
base: rewrite
Choose a base branch
from
Open

Improved status checking in mail.c in accordance to RFC requirements #332

wants to merge 2 commits into from

Conversation

wo-rasp
Copy link
Contributor

@wo-rasp wo-rasp commented Feb 10, 2017

Added Error Checking for 4xx and 5xx codes to mail.c rewrite branch

@CurlyMoo
Copy link
Contributor

I'd rather see a 4xx and a 5xx in the code. That makes it more clear we are checking for all 400 and 500 range codes. Also, can you commit under your account.

I also miss the unit test to check if these errors are correctly handled.

@wo-rasp wo-rasp force-pushed the rew_mail branch 2 times, most recently from a60641f to 4d6ba26 Compare February 10, 2017 15:47
@wo-rasp
Copy link
Contributor Author

wo-rasp commented Feb 11, 2017

The function call tells specifically, that only the 1 digit is checked. 400 is as good as 4xx or 4

Unittest:
Can you elaborate how that unit test shall be look like ?
Which mail server implementation and which RFC paper from the various smtp implemantations around shall serve as a base ?

@CurlyMoo
Copy link
Contributor

Unittest
What i did was using telnet to talk to actual mailservers and simulating it in the unit test. It did not use any protocol papers.

So, simulate a valid 4xx and a valid 5xx response in the unit test and check if the mail function indeed fails as expected. I believe the 4xx is already implemented.

pilino1234
pilino1234 reviewed Mar 5, 2017
View reviewed changes
libs/pilight/core/mail.c Outdated
@@ -206,7 +206,7 @@ static void read_cb(uv_poll_t *req, ssize_t *nread, char *buf) {
}
}

if(val == 501 && ch == 32) {
if( (val >= 400 && val <= 599) && ch == 32) {
Copy link
Member

Choose a reason for hiding this comment

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

Spurious space before brackets, otherwise 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@CurlyMoo
Copy link
Contributor

CurlyMoo commented Mar 9, 2017

Any news on the unit test?

@pilino1234
Copy link
Member

bump, and needs rebasing

@CurlyMoo
Copy link
Contributor

And it needs additional tests.

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

@pilino1234 pilino1234 pilino1234 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@wo-rasp @CurlyMoo @pilino1234