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

NWM_ UDS:Let connected clients handle the eapol packet #3489

Merged
merged 3 commits into from Apr 5, 2018

Conversation

Projects
None yet
4 participants
@B3n30
Contributor

B3n30 commented Mar 8, 2018

This updates the connection_status and the node_info of already connected clients

On a 3ds a special packet is send for that purpose, but thats not necessary for us. We can just use the eapol packet that is sent to the new client


This change is Reviewable

@cluezbot

This comment has been minimized.

cluezbot commented Mar 8, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-08T16:36:17Z: 3cda637...B3n30:02761492a6061804bdd9fee81628667ef6516bd2

@jroweboy jroweboy requested a review from Subv Mar 8, 2018

@B3n30 B3n30 changed the title from Let connected clients handle the eapol packet to NWM_ UDS:Let connected clients handle the eapol packet Mar 8, 2018

@B3n30 B3n30 added the canary-merge label Mar 8, 2018

@B3n30 B3n30 force-pushed the B3n30:UDS_send_node_information branch from 0276149 to 29d6e05 Mar 9, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Mar 9, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-09T18:15:43Z: 33a0e87...B3n30:29d6e050445df9d5c78faaf02e0ea5024a8097b8

@wwylele

code LGTM
@Subv I just want to know, if this approach is considered too hacky, or it is just fine

@Subv

Subv approved these changes Mar 30, 2018

LGTM except for a few nits.

As for the approach, ideally we'd want to replicate the actual 3DS's behavior, could you please add a TODO in the code and open a github issue to keep it in mind?

// On a 3ds the eapol packet is only sent to packet.transmitter_address
// while a packet containing the node information is broadcasted
// For now we will bradcast the eapol packet instead

This comment has been minimized.

@Subv

Subv Mar 30, 2018

Member

broadcast

} else if (connection_status.status == static_cast<u32>(NetworkStatus::ConnectedAsClient)) {
// On a 3ds this packet wouldn't be addressed to already connected clients
// We use this information because in the current implementation the host
// isn't broadcsting the node information

This comment has been minimized.

@Subv

Subv Mar 30, 2018

Member

broadcasting

node_info.clear();
node_info.reserve(network_info.max_nodes);
for (size_t index = 0; index < logoff.connected_nodes; ++index) {
if (!(connection_status.node_bitmask & 1 << index)) {

This comment has been minimized.

@Subv

Subv Mar 30, 2018

Member

imo this can be made clearer with (connection_status.node_bitmask & (1 << index)) == 0

@cluezbot

This comment has been minimized.

cluezbot commented Apr 5, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-04-05T12:43:26Z: 33a0e87...B3n30:2d72b2be8b9a05a707101a8b33a4cc72bfc9c12e

@cluezbot

This comment has been minimized.

cluezbot commented Apr 5, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-04-05T12:57:38Z: 33a0e87...B3n30:f64f118e3b0372e0f0c159e88aecda2df7e916e0

@wwylele wwylele merged commit a2e8707 into citra-emu:master Apr 5, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment