[LibOS] Clean up `select` and `poll` implementations #1067

boryspoplawski · 2022-11-30T01:27:42Z

Description of the changes

Fixed some bugs, hopefully the code easier to manage too now.

This change is

boryspoplawski

Reviewable status: 0 of 15 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)

libos/src/sys/libos_poll.c line 113 at r2 (raw file):

            continue;

            dev_tty_hack:;

Alternative version (I dislike both):

Suggestion:

            if (!(ret == -EAGAIN && handle->uri && !strcmp(handle->uri, "dev:tty"))) {
                if (ret < 0) {
                    unlock(&map->lock);
                    goto out;
                }

                fds[i].revents = events;
                if (events) {
                    ret_events_count++;
                }

                continue;
            }

dimakuv

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

libos/src/fs/libos_fs_mem.c line 117 at r2 (raw file):

int mem_file_poll(struct libos_mem_file* mem, file_off_t pos, int events, int* out_events) {
    *out_events = events & (POLLOUT | POLLWRNORM);
    if ((events & (POLLIN | POLLRDNORM)) && (pos < mem->size))

Why do you need the first condition? This will be cleared anyway in the below assignment if false.

libos/src/fs/libos_fs_pseudo.c line 487 at r2 (raw file):

            *out_events = 0;
            if ((events & POLLIN) && node->dev.dev_ops.read)
                *out_events |= POLLIN;

Why no POLLRDNORM?

libos/src/fs/libos_fs_pseudo.c line 489 at r2 (raw file):

                *out_events |= POLLIN;
            if ((events & POLLOUT) && node->dev.dev_ops.write)
                *out_events |= POLLOUT;

Why no POLLWRNORM?

libos/src/sys/libos_poll.c line 95 at r2 (raw file):

             * Initial 0,1,2 fds in Gramine are represented by "/dev/tty" (whatever that means)
             * and have `generic_inode_poll` set as poll callback, which returns `-EAGAIN` on
             * non-regular-file handles. In such case we let PAL do the actuall polling.

actual

libos/src/sys/libos_poll.c line 113 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Alternative version (I dislike both):

Yeah, both versions are ugly. Let's keep the current one.

libos/src/sys/libos_poll.c line 120 at r2 (raw file):

            pal_handle = __atomic_load_n(&handle->info.sock.pal_handle, __ATOMIC_ACQUIRE);
            if (!pal_handle) {
                /* UNIX sockets that are still not connected have no `pal_handle`. */

Why only UNIX sockets? Can't other kinds of sockets (INET) have the same problem?

libos/src/sys/libos_poll.c line 239 at r2 (raw file):

    if (tsp) {
        if (is_user_memory_writable_no_skip(tsp, sizeof(*tsp))) {

Actually, why do we care about skip vs no_skip versions? Isn't libos.check_invalid_pointers = false already gives you no guarantees what happens at runtime, so the user is on his own?

libos/src/sys/libos_poll.c line 344 at r2 (raw file):

    long ret = do_select(nfds, read_set, write_set, except_set, tv ? &timeout_us : NULL);

    if (tv && is_user_memory_writable_no_skip(tv, sizeof(*tv))) {

ditto

libos/src/sys/libos_poll.c line 382 at r2 (raw file):

    long ret = do_select(nfds, read_set, write_set, except_set, tsp ? &timeout_us : NULL);

    if (tsp && is_user_memory_writable_no_skip(tsp, sizeof(*tsp))) {

ditto

libos/test/ltp/ltp.cfg line 1364 at r2 (raw file):

[personality02]
skip = yes

Could you add a comment why you added this?

boryspoplawski

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv)

libos/src/fs/libos_fs_mem.c line 117 at r2 (raw file):