Skip to content

Communication protocol for Dive: Socket connection #221

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

Merged
merged 1 commit into from
Jun 3, 2025

Conversation

elviscapiaq
Copy link
Collaborator

  • The socket connection header and its implementation were added.
  • An initial cmake setup was added for transfer_client and transfer_server libraries.

@elviscapiaq elviscapiaq force-pushed the socket branch 2 times, most recently from 92c4447 to 1c4e808 Compare May 28, 2025 19:59
@elviscapiaq elviscapiaq force-pushed the socket branch 3 times, most recently from a91026b to 140a6de Compare May 29, 2025 08:39
@elviscapiaq
Copy link
Collaborator Author

I fixed the issue with Windows when targeting shared libraries for both: client and server. For shared libraries I added the linking with the WIN Sockets.

@elviscapiaq elviscapiaq force-pushed the socket branch 2 times, most recently from 82b4d21 to ed98e3a Compare May 29, 2025 17:32
@elviscapiaq elviscapiaq changed the title Transfer Infrastructure for Dive: Socket connection Communication protocol for Dive: Socket connection May 30, 2025
@elviscapiaq
Copy link
Collaborator Author

I changed the commit title to "Communication protocol for Dive: Socket connection".

Copy link
Collaborator

@angela28chen angela28chen left a comment

Choose a reason for hiding this comment

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

Have you considered using different files rather than #ifdef _WIN32 for the platform-specific code, with the cmake file dictating the appropriate files to include during the build? My impression is that having it structured like that would be easier to read and maintain, especially since seeing the socket_connection.cc logic here is so different between Linux and Windows.

EDIT: For example, how capture_service/command_utils_win32.cc is set up

- The socket connection header and its implementation were added.
- An initial cmake setup was added for transfer_client and
  transfer_server libraries.
@elviscapiaq
Copy link
Collaborator Author

I updated the error handling style to absl::Status and absl::StatusOr. I think now it is more clear the error handling process along the files.

@elviscapiaq elviscapiaq requested a review from RenfengLiu June 3, 2025 08:23
@elviscapiaq elviscapiaq requested a review from angela28chen June 3, 2025 08:23
Copy link
Collaborator

@angela28chen angela28chen left a comment

Choose a reason for hiding this comment

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

LGTM and thanks for switching to using abseil!

I don't know if you saw my earlier comment but I was wondering if there is a reason you are leaning towards using many ifdef statements for the differences between windows/linux VS doing something like implementing separate platform-specific files for implementations of SocketConnection and NetworkInitializer? My impression is that the ifdef approach works but is not generally recommended because it makes code harder to read and you're more likely to run into issues with defines.

@elviscapiaq elviscapiaq merged commit 8916923 into google:main Jun 3, 2025
7 checks passed
@elviscapiaq
Copy link
Collaborator Author

LGTM and thanks for switching to using abseil!

I don't know if you saw my earlier comment but I was wondering if there is a reason you are leaning towards using many ifdef statements for the differences between windows/linux VS doing something like implementing separate platform-specific files for implementations of SocketConnection and NetworkInitializer? My impression is that the ifdef approach works but is not generally recommended because it makes code harder to read and you're more likely to run into issues with defines.

I think I misunderstood your comment; I thought it was about _WIN32 modification. I'll keep your recommendation in mind, and once I finish all the implementation, I can work on that if there are no higher priorities. Thanks!

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.

4 participants