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

Add version negotiation to network protocol #98

Merged
merged 4 commits into from
Aug 30, 2020
Merged

Conversation

newsch
Copy link
Member

@newsch newsch commented Aug 29, 2020

                                               d
       RRR                                     d
    RRR  R                                     d
    RRRRRR      eeeeee      aaaaa       dddddddd
    RRRR       ee    ee    aa   a       d      d
    R   RR     eeeeeee     a    a       d      d
    RR   RR     e          a    aa      ddd   dd
          R     eeeeeeeee  aaaaaaaaaa     ddddddd
                                            dd  ddd
     t    h                                                             t
   tttt   h         eeee                                               tt
     ttt  hhhhhh   ee  e                                          ii  ttttt
     t   hh    h   ee  e         cc     oooo   m mmmmmmm  mmmmm   i    tt
     t    hh   h    ee          cc      o oo  mm   m   m  m m m   i     t
                     eeeee      ccccc   ooo   m    m   m  m m m  ii     t


@newsch
Copy link
Member Author

newsch commented Aug 29, 2020

I'm proud to say this is the first time I've thought "this is a good use-case for GOTO"

@newsch newsch added enhancement New feature or request network labels Aug 29, 2020
@newsch newsch self-assigned this Aug 29, 2020
@@ -154,6 +156,35 @@ void *handle_client(void *arg) {
print_client_addr(cli->addr);
printf(" referenced by %d\n", cli->uid);

// protocol negotiation
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree it's not a bad use of GOTO, but I'm not convinced it's a good one either?
You could do the exact same thing with a "negotiate protocol" void function, and at a glance the function signature shouldn't even be too bad (especially if you pass rlen as a parameter).
Also if else statements could work, at the cost of some duplicate code (which I assume is why you got stoked about GOTO in the first place).
I vote for the function, but if you do want to keep the GOTO statement, watch your 6.

Copy link
Member Author

Choose a reason for hiding this comment

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

You could do the exact same thing with a "negotiate protocol" void function, and at a glance the function signature shouldn't even be too bad (especially if you pass rlen as a parameter).

Fair. I'm trying to hold off on refactoring any server code to avoid more merge conflicts with #80, which already restructures a fair bit of it.

I'll rewrite this bit as part of #80.

newsch added 3 commits August 30, 2020 13:03
Before making more breaking changes to the networking protocol (like
sending user coordinates back and forth for cursor support), it seemed
best to add a method of negotiating a protocol version so things can
fail or downgrade more gracefully.

As implemented here, previous versions of servers and clients will not
work with these updated versions. This would be a pretty big problem,
except that as far as I know nobody uses this project regularly,
especially in a networked/remote capacity.

Because previously the client expected canvas data to be sent over
immediately, the only way that I can think of to support older clients
while not requiring that canvas send is to add a timeout on the server
that waits for a version number and then sends the canvas if nothing is
sent, which seems pretty unideal.
Bonus: if you squint really hard it looks like ASCII
@newsch newsch force-pushed the newsch/protocol-version branch from 03cb87f to 1e4c0df Compare August 30, 2020 20:05
Oddly the increased compiler warnings didn't reveal this issue...
the iteration on line 308 gave me a warning in my editor.
@newsch newsch merged commit f69205c into master Aug 30, 2020
@newsch newsch deleted the newsch/protocol-version branch August 30, 2020 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request network
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants