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.

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

Introduce Request::reply_parser #475

Merged
merged 7 commits into from
Jun 11, 2020
Merged

Conversation

khuey
Copy link
Contributor

@khuey khuey commented Jun 8, 2020

When parsing the X11 protocol stream without the benefit of the client state (e.g. for something like xtrace) we need to keep track of the various Requests in flight, their serial numbers, and their types so that we can parse responses into the corresponding Replys. While I previously added a Request trait that contains Reply type, in practice it is difficult to work with because a) some replies are created by try_parse and others by try_parse_fd and b) the library consumer needs to go through every case in the Request themselves. This solves (a) by introducing TryParseFd and adding a blanket impl<T: TryParse> TryParseFd and (b) by implementing a Request::reply_parser that gives the appropriate reply parser as a function pointer for the Request variant.

One thing that I did realize as a result of this is that the Request enum and the Request trait have a name conflict. We may want to do something about that.

I also think an easy followup to this would be dropping the From<&[u8]> and From<(&[u8], &mut Vec<RawFdContainer>)> implementations for replies and replacing them with TryParse and TryParseFd respectively in the cookie code.

Copy link
Owner

@psychon psychon left a comment

Choose a reason for hiding this comment

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

Basically 👍 except for one small nitpick about the API docs for TryParseFD. The rest is mostly random rambling.

Sorry for the delay in looking at this. Life happened. @eduardosm Feel free to approve this when you think my comments are addressed. (Also: Feel free to just approve this if you disagree with my comments. I think mergify is configured to ignore "request changes" reviews.)

src/x11_utils.rs Show resolved Hide resolved
@@ -50,6 +50,15 @@ pub trait TryParseFd: Sized {
) -> Result<(Self, &'a [u8]), ParseError>;
}

impl<T: TryParse> TryParseFd for T {
Copy link
Owner

Choose a reason for hiding this comment

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

Random thought: Would it be a simplification to remove the TryParse trait? (Afterwards TryParseFD could be renamed to TryParse)

Downside: Lots of code needs to be changed to add an extra argument that is just ignored (oh and the use for RawFdContainer also becomes necessary everywhere).
Upside: One less trait. Does that count as a simplification?

I tend towards "keep both TryParse and TryParseFD" since FDs are such a special case. Thus, they shouldn't "infect" everything. Still, I am leaving this in case anyone feels like this is a great idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought about this and came to a similar conclusion.

src/protocol/mod.rs Outdated Show resolved Hide resolved
src/protocol/mod.rs Show resolved Hide resolved
@khuey
Copy link
Contributor Author

khuey commented Jun 11, 2020

Sorry for the delay in looking at this. Life happened.

No worries.

@mergify mergify bot merged commit 7ec568b into psychon:master Jun 11, 2020
@psychon
Copy link
Owner

psychon commented Jun 11, 2020

Thanks for the PR and thanks for showing me once again the magic of Rust. I wouldn't have thought of using the trait's default method in this way. Nice trick.

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