Pop & misc cleanup#113
Merged
Merged
Conversation
Trolled yet again by local artifacts within the container >:(
Watchtower already had is own gitignore and each folder needs its own dockerignore file. This also helps to avoid conflicts in the main one. We should reserve the top level gitignore for stuff actually in the top level directory or wildcards like the swap files.
Containers should be able to consume these programs separately. There isn't really an relation between them except that they were developed out of tree together at the same time using the same repo for convenience.
Even though the underlying file should be opened O_RDONLY we still want to allow PROT_WRITE on the mapping of it so that the active field of the mail can be updated in response to DELE or RSET commands. Fixes: 710b051 ("pop: switch to caching email data in journal file")
The `send` function was originally imported wholesale from smtp and so the comment refers to particulars of the smtp protocol that are irrelevant to a pop implementation, though the gist is correct.
The original password code was copy pasted from the username code and I forgot to change the string. The only important part is the `+OK` at the beginning so this doesn't really matter, but I have been meaning to fix it for ages. Fixes: 76e03e6 ("Initial implementation using code from smtp server")
All commands want to consume a line of input whether or not they process further, even if it might be marginally faster to skip reading the data into the buffer when the command will just ignore it, cutting down on repetition in each case is worth it.
some commands are valid in all states and can be processed first, then we can filter based on states because for some states there is really only one non generic command that is valid, and then the bulk of the commands that are only valid once you are logged in can just rely on the prior logic to block unauthenticated users from reaching that point in the code. Note: there is a single semicolon added to the beginning of the `stat` command case. This is intentional because you cannot start a case label with a declaration (at least not until C23 which is not fully supported yet). This wasn't a problem previously because there were two conditions checked before the decl, but now they are both gone so the error occurs. This command will get refactored shortly, so that semicolon is not a long term thing, just a stopgap to prevent this commit from breaking git-bisect
Handle the partial writes and error conditions in one place
All commands that take an argument take a single integer (except for top) that refers to an email. We can try parsing that argument in all cases if it exists and subsequent code can check whether or not it exists by checking whether a pointer is null.
In order to be able to introduce an enumeration of commands, it is useful we need to save the labels `QUIT` and `USER` for that purpose.
This will centralize the error checking so that an "invalid command" message can properly be generated if the command is unrecognized before further processing. The enumeration values being small and sequential probably makes switching over them have better code-gen (i.e. jump-tables instead of if else comparisons)
we can just use `dprintf` to send unbuffered data to `stdout` and let libc deal with managing a buffer for storing the intermediate data.
Avoids replication of format strings. DRY.
This split allows a lot of the code to be deindented even though the overall number of lines remains basically the same. It also helps to conceptually separate the control flow so that it is easier to grok.
charliemirabile
force-pushed
the
pop_n_misc_cleanup
branch
from
089d37d to
153000f
Compare
theyoyojo
reviewed
May 21, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Various fixes and cleanups including a substantial improvement for pop to remove duplicate code and various copy paste errors that came with it.
Fixes #17