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

[LibOS] Add support for SO_BROADCAST socket option #686

Merged
merged 1 commit into from
Jun 30, 2022

Conversation

boryspoplawski
Copy link
Contributor

@boryspoplawski boryspoplawski commented Jun 27, 2022

Description of the changes

As in the title.
I don't think it's work adding a test for this and spam CI with UDP broadcast packets.


This change is Reviewable

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @boryspoplawski)


-- commits line 3 at r1:
You don't make a difference between TCP (for which this option is a no-op) and UDP (for which this option is useful) in Gramine code, you just pass-through to the host and let the host decide.

Maybe add this design choice in the commit message? Because my expectation was that LibOS will do nothing if it is a TCP socket. The tradeoff in that case would be less syscalls/OCALLs but at the cost of more code lines. I can imagine we'll ask ourselves in the future why we didn't do it, and without at least a commit message (or maybe a comment in the code), our future selves be puzzled.

Copy link
Contributor Author

@boryspoplawski boryspoplawski left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)


-- commits line 3 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You don't make a difference between TCP (for which this option is a no-op) and UDP (for which this option is useful) in Gramine code, you just pass-through to the host and let the host decide.

Maybe add this design choice in the commit message? Because my expectation was that LibOS will do nothing if it is a TCP socket. The tradeoff in that case would be less syscalls/OCALLs but at the cost of more code lines. I can imagine we'll ask ourselves in the future why we didn't do it, and without at least a commit message (or maybe a comment in the code), our future selves be puzzled.

How about we add the check? :)

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 11 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)


libos/src/net/ip.c line 320 at r2 (raw file):

        case SO_BROADCAST:
            if (sock->type == SOCK_STREAM) {
                /* This option has no effect on stream oriented sockets. */

Suggestion:

stream-oriented

libos/src/sys/shim_socket.c line 40 at r2 (raw file):

 */

struct libos_handle* get_new_socket_handle(int family, int type, int protocol,

Now we have both get_new_socket_handle() and create_sock_handle(), which for me are synonymous. Could you at least describe what they do in a comment, so one could notice the difference easier?

Also, sock vs socket in their names.

dimakuv
dimakuv previously approved these changes Jun 30, 2022
Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

Copy link
Contributor Author

@boryspoplawski boryspoplawski left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)


libos/src/net/ip.c line 320 at r2 (raw file):

        case SO_BROADCAST:
            if (sock->type == SOCK_STREAM) {
                /* This option has no effect on stream oriented sockets. */

Done.


libos/src/sys/shim_socket.c line 40 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Now we have both get_new_socket_handle() and create_sock_handle(), which for me are synonymous. Could you at least describe what they do in a comment, so one could notice the difference easier?

Also, sock vs socket in their names.

I went with get_new to match other functions we have for creating handles (which are also called get)
I removed create_sock_handle, it was just a simple wrapper for get_new + call create callback. It was not used in many places and most of them called other callbacks anyway, so I simply inlined it.

dimakuv
dimakuv previously approved these changes Jun 30, 2022
Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)

mkow
mkow previously approved these changes Jun 30, 2022
Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 15 of 15 files at r4, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL)

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@boryspoplawski boryspoplawski merged commit 1d47d50 into master Jun 30, 2022
@boryspoplawski boryspoplawski deleted the borys/so_broadcast branch June 30, 2022 18:13
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