[LibOS] Allow setting O_NONBLOCK on not connected UNIX sockets

[boryspoplawski]

Description of the changes

And some code clean ups.

---

This change is 

[boryspoplawski]

Reviewable status: 0 of 6 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL)

---

libos/src/fs/pipe/fs.c line 216 at r1 (raw file):

        log_warning("FIFO (named pipe) '%s' cannot be opened in read-write mode in Gramine. "
                    "Treating it as read-only.", dent->mount->path);
        flags = (flags & ~O_ACCMODE) | O_RDONLY;

FYI: this was a bug.

[dimakuv]

Reviewed 4 of 6 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, 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 @boryspoplawski)

---

-- commits line 5 at r2:
Could you remind again why we talk only about UNIX sockets, even though the change is in a generic function src/fs/socket/fs.c: set_flags()?

Is it simply because handle->pal_handle == NULL can only happen on UNIX sockets?

---

libos/include/shim_fs.h line 168 at r2 (raw file):

     *                (to values taken from \p flags). Must be non zero.
     */
    int (*setflags)(struct shim_handle* handle, unsigned int flags, unsigned int mask);

I don't like that now we have handle name here, but hdl everywhere else in this file (and associated files with callback implementations). Maybe we should revert to hdl for uniformity, and have a proper separate PR will do this name change?

UPDATE: I see that you already have callback implementations with handle. So we already have a non-uniformity. Resolving.

---

libos/src/fs/pipe/fs.c line 167 at r2 (raw file):

    /* TODO: what is this check? Why it has no locking? */
    if (!handle->pal_handle)
        return 0;

Yeah, this check looks wrong. Should be an assert. But can be done in a separate PR, after verifying all callers of pipe_setflags.

---

libos/src/net/unix.c line 136 at r2 (raw file):

    lock(&handle->lock);
    /* `setflags` in "fs/socket/fs.c" is the only way to change this flag and it takes `sock->lock`,
     * so this is race-free. */

I always have to pause and think about this comment. Maybe

so this is race-free -> so using `options` when creating the socket below is race-free

---

libos/src/net/unix.c line 245 at r2 (raw file):

    lock(&handle->lock);
    /* `setflags` in "fs/socket/fs.c" is the only way to change this flag and it takes `sock->lock`,
     * so this is race-free. */

I always have to pause and think about this comment. Maybe

so this is race-free -> so using `options` when creating the socket below is race-free

---

libos/src/sys/shim_fcntl.c line 35 at r2 (raw file):

     * The old version of this code did this, but this seem to be incorrect. */
    lock(&handle->lock);
    handle->flags = (handle->flags & ~mask) | flags;

Incorrect in the sense that this has no "handle-specific" logic (like updating PAL handle's attributes and doing additional locking)?

I agree that this looks incorrect and hopefully is never used (instead the handle-specific callback is always used).

---

libos/src/sys/shim_pipe.c line 58 at r2 (raw file):

    assert(!cli->pal_handle);
    srv->pal_handle = hdl1;
    cli->pal_handle = hdl2;

You just simplified it because who cares about modifications to srv and cli out arguments if this func anyway returns an error?

[boryspoplawski]

Reviewable status: all files reviewed, 5 unresolved discussions, 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 5 at r2:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you remind again why we talk only about UNIX sockets, even though the change is in a generic function src/fs/socket/fs.c: set_flags()?

Is it simply because handle->pal_handle == NULL can only happen on UNIX sockets?

Yes, excatly. Maybe a comment is due, explaining that all implementations allowing pal_handle to be NULL should set the currently enabled flags when they instantiate pal_handle?

---

libos/include/shim_fs.h line 168 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I don't like that now we have handle name here, but hdl everywhere else in this file (and associated files with callback implementations). Maybe we should revert to hdl for uniformity, and have a proper separate PR will do this name change?

UPDATE: I see that you already have callback implementations with handle. So we already have a non-uniformity. Resolving.

I just dislike hdl so I use handle in the new code I write. I wouldn't create a PR just for this rename, it would touch half of the LibOS code...

---

libos/src/net/unix.c line 136 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I always have to pause and think about this comment. Maybe

so this is race-free -> so using `options` when creating the socket below is race-free

Slightly reworded, let me know it it looks ok

---

libos/src/net/unix.c line 245 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I always have to pause and think about this comment. Maybe

so this is race-free -> so using `options` when creating the socket below is race-free

ditto

---

libos/src/sys/shim_fcntl.c line 35 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Incorrect in the sense that this has no "handle-specific" logic (like updating PAL handle's attributes and doing additional locking)?

I agree that this looks incorrect and hopefully is never used (instead the handle-specific callback is always used).

Just that it only saves the value, doesn't actually do anything with it. For some handles and some flags it's probably fine (e.g. because code using them always check them), but generally sounds incorrect. IMO each handle type should have it's own callback and deal with these options as it wishes (then just memorizing them would be fine)>

---

libos/src/sys/shim_pipe.c line 58 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You just simplified it because who cares about modifications to srv and cli out arguments if this func anyway returns an error?

This function is always used on newly created handles. I'm not sure why it doesn't create them itself, why does it take them as arguments (and populate).
Generally I think that having a function which initializes part of an already half initialized object is a bad smell (unless the initial object was fully initialized, the function just changes parts of it and all failures are fatal, i.e. the object is destroyed)>

[dimakuv]

Reviewed 1 of 1 files at r3, 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), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

---

-- commits line 5 at r2:

Previously, boryspoplawski (Borys Popławski) wrote…

Yes, excatly. Maybe a comment is due, explaining that all implementations allowing pal_handle to be NULL should set the currently enabled flags when they instantiate pal_handle?

Yes, such a comment would be good. Probably in shim_handle.h, close to the definition of pal_handle, in that top level comment for shim_handle struct.

[boryspoplawski]

Reviewable status: 6 of 7 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 5 at r2:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes, such a comment would be good. Probably in shim_handle.h, close to the definition of pal_handle, in that top level comment for shim_handle struct.

Added a comment.

[dimakuv]

Reviewed 1 of 1 files at r4, 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

[mkow]

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

---

libos/src/fs/pipe/fs.c line 216 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

FYI: this was a bug.

😬

---

libos/src/fs/pipe/fs.c line 192 at r3 (raw file):

    }

    handle->flags = (handle->flags & ~mask) | flags;

Not sure if it matters, but handle->flags is still int (i.e. signed, and different than the types you used here).

---

libos/src/sys/shim_fcntl.c line 35 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Just that it only saves the value, doesn't actually do anything with it. For some handles and some flags it's probably fine (e.g. because code using them always check them), but generally sounds incorrect. IMO each handle type should have it's own callback and deal with these options as it wishes (then just memorizing them would be fine)>

Could you add this explanation to the comment?

[boryspoplawski]

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), "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

libos/src/fs/pipe/fs.c line 192 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Not sure if it matters, but handle->flags is still int (i.e. signed, and different than the types you used here).

The highest bit cannot be ever set, so it does not. We should change it to unsigned int, but not in this PR.

---

libos/src/sys/shim_fcntl.c line 35 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Could you add this explanation to the comment?

Done.

[mkow]

Reviewed 1 of 1 files at r5, 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), "fixup! " found in commit messages' one-liners

---

libos/src/fs/pipe/fs.c line 192 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

The highest bit cannot be ever set, so it does not. We should change it to unsigned int, but not in this PR.

yeah, I know, it's more like a stylistic issue ;)

[mkow]

Reviewed 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)

[dimakuv]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved