-
Notifications
You must be signed in to change notification settings - Fork 243
repair: refactor to remove callbacks #4886
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
Conversation
1a0b692
to
2a832c3
Compare
2a832c3
to
5c56d94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally my idea here is to make fd_repair more library vs. "framework-y" by filling values that are returned to the caller
benefits include makes it easier to unit test, reduces nesting, separates concerns in the function
src/discof/repair/fd_repair_tile.c
Outdated
FD_TEST(0 == fd_repair_protocol_encode(&protocol, &ctx)); | ||
ulong buflen = (ulong)((uchar*)ctx.data - buf); | ||
|
||
repair_send_intake_packet( buf, buflen, peer_addr, self_ip4_addr, repair_tile_ctx ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what i actually was thinking is this would just fill the packet inside a caller provided buf
arg
fd_repair_handle_ping( fd_repair_tile_ctx_t * repair_tile_ctx,
fd_repair_t * glob,
fd_gossip_ping_t const * ping,
fd_gossip_peer_addr_t const * peer_addr,
uint self_ip4_addr,
uchar * buf,
ulong * buf_sz ) {
and then the caller would be responsible for calling repair_send_intake_packet
(which can probably be deleted and just directly call send_packet
)
as opposed to invoking the function itself
src/discof/repair/fd_repair_tile.c
Outdated
if( shred == NULL ) { | ||
FD_LOG_WARNING(("invalid shread")); | ||
} else { | ||
repair_shred_deliver(shred, shredlen, src_addr, &val->id, repair_tile_ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to above, i think a cleaner API would be to not actually call repair_shred_deliver
(also we can probably clean this up. the mutation of active table should be managed by caller separately vs. inside this function. but this can happen in a later PR)
src/discof/repair/fd_repair_tile.c
Outdated
fd_memcpy( buf + 4U, &sig, 64U ); | ||
|
||
uint src_ip4_addr = 0U; /* unknown */ | ||
repair_send_intake_packet( buf, buflen, addr, src_ip4_addr, repair_tile_ctx ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
src/discof/repair/fd_repair_tile.c
Outdated
FD_TEST(0 == fd_repair_response_encode(&gmsg, &ctx)); | ||
ulong buflen = (ulong)((uchar*)ctx.data - buf); | ||
|
||
repair_send_serve_packet( buf, buflen, dst_addr, src_ip4_addr, repair_tile_ctx ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
src/discof/repair/fd_repair_tile.c
Outdated
} | ||
fd_hash_copy( &val->id, &header->sender ); | ||
val->good = 0; | ||
fd_repair_send_ping( repair_tile_ctx, glob, peer_addr, self_ip4_addr, val ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, but dont worry about changing this function since we don't need it right now
c8eab47
to
0acb199
Compare
pushed directly to repair-replay-link