[LibOS] Fix filesystem corner cases

[pwmarcz]

Description of the changes

This fixes various small corner cases in filesystem code. As mentioned in #29, our test runner failed to detect these because the LTP test cases actually returned 0.

The LTP runner is also fixed so that this doesn't happen again.

Fixes #29.

The LTP tests fixed are pwrite02, pwrite03 and ftruncate03.

How to test this PR?

Jenkins should pass, you can also run the above tests manually.

---

This change is 

[boryspoplawski]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 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 @pwmarcz and @woju)

---

LibOS/shim/src/fs/chroot/fs.c, line 391 at r1 (raw file):

    if (count == 0) {
        /* exit early: we allow null buffer when count is 0, and `DkStreamRead` doesn't allow it */

Why does DkStreamRead does not allow it? I see no reason for it...

---

LibOS/shim/src/fs/chroot/fs.c, line 433 at r1 (raw file):

    if (count == 0) {
        /* exit early: we allow null buffer when count is 0, and `DkStreamWrite` doesn't allow it */

ditto

---

LibOS/shim/src/sys/shim_open.c, line 564 at r1 (raw file):

    if (!(hdl->acc_mode & MAY_WRITE)) {
        ret = -EINVAL;

Could you add a comment that Linux does this (instead of EBADF like in other places)?

---

LibOS/shim/src/sys/shim_open.c, line 572 at r1 (raw file):

    if (hdl->is_dir || !fs->fs_ops->truncate) {
        ret = -EINVAL;

ret is already set here. Please either set it everywhere or nowhere (i.e. unify).

---

LibOS/shim/test/ltp/runltp_xml.py, line 321 at r1 (raw file):

                raise Fail('returncode={}'.format(returncode))
            else:
                # Parse output anyway: unfortunately some tests do not exit with non-zero code when

I've raised this issue already before, but was told this is done this way intentionally, tho I have no idea why.
cc @mkow @woju

---

LibOS/shim/test/ltp/runltp_xml.py, line 323 at r1 (raw file):

                # Parse output anyway: unfortunately some tests do not exit with non-zero code when
                # failing.
                self._parse_test_output()

Can't we change it to just:

            if must_pass is not None or returncode == 0:
                self._parse_test_output(must_pass)
            else:
                raise Fail('returncode={}'.format(returncode))

[boryspoplawski]

Reviewable status: all files reviewed, 6 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 @pwmarcz and @woju)

---

LibOS/shim/src/fs/chroot/fs.c, line 391 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why does DkStreamRead does not allow it? I see no reason for it...

I mean, why can't we make it accept count == 0?

[pwmarcz]

Reviewable status: 0 of 4 files reviewed, 6 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, @mkow, @pwmarcz, and @woju)

---

LibOS/shim/src/sys/shim_open.c, line 564 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Could you add a comment that Linux does this (instead of EBADF like in other places)?

Done.

---

LibOS/shim/src/sys/shim_open.c, line 572 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ret is already set here. Please either set it everywhere or nowhere (i.e. unify).

OK. I'd rather be explicit.

---

LibOS/shim/test/ltp/runltp_xml.py, line 321 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I've raised this issue already before, but was told this is done this way intentionally, tho I have no idea why.
cc @mkow @woju

Yeah, I think this is a pretty strong argument against. LTP can't be trusted.

---

LibOS/shim/test/ltp/runltp_xml.py, line 323 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Can't we change it to just:

            if must_pass is not None or returncode == 0:
                self._parse_test_output(must_pass)
            else:
                raise Fail('returncode={}'.format(returncode))

Done, almost: I want it to be explicit that we're failing because of non-zero exit code.

(Also I used not must_pass to treat empty set the same as no must-pass entry).

---

LibOS/shim/src/fs/chroot/fs.c, line 391 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I mean, why can't we make it accept count == 0?

I assumed there was a reason, but yes, this is Graphene. If Jenkins passes, then we should be good :)

[boryspoplawski]

Reviewed 4 of 4 files at r2, 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 @mkow and @woju)

---

LibOS/shim/test/ltp/runltp_xml.py, line 321 at r1 (raw file):
Sorry, I did not understand your comment. What is a strong argument against what?

LTP can't be trusted.

This is actually not LTP, but Gramine issue (shared memory does not work, yet MAP_SHARED is accepted, but is silently discarded).

---

LibOS/shim/src/fs/chroot/fs.c, line 391 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I assumed there was a reason, but yes, this is Graphene. If Jenkins passes, then we should be good :)

;)

[mkow]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 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 @pwmarcz and @woju)

---

-- commits, line 6 at r2:
trailing (?

---

LibOS/shim/test/ltp/runltp_xml.py, line 321 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Sorry, I did not understand your comment. What is a strong argument against what?

LTP can't be trusted.

This is actually not LTP, but Gramine issue (shared memory does not work, yet MAP_SHARED is accepted, but is silently discarded).

Tbh I don't remember this discussion, Paweł's fix looks good.

[dimakuv]

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

---

LibOS/shim/src/sys/shim_open.c, line 30 at r2 (raw file):

ssize_t do_handle_read(struct shim_handle* hdl, void* buf, size_t count) {
    if (!(hdl->acc_mode & MAY_READ))
        return -EBADF;

For my own understanding: so it was always broken? Why does #29 talk only about a regression after your Inodes commit? What did Inodes commit reveal that some LTP tests started failing?

[dimakuv]

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

a discussion (no related file):
Reviewable is being weird: it doesn't show the reverted changes in chroot_fs.c file (though I can see them on GitHub). Anyone knows why this behavior?

---

[pwmarcz]

Reviewable status: all files reviewed, 4 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, @mkow, and @woju)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Reviewable is being weird: it doesn't show the reverted changes in chroot_fs.c file (though I can see them on GitHub). Anyone knows why this behavior?

I'm seeing it in Reviewable: the diff is empty; the change appears when I select a range like ⊥..r1 or r1..r2.

---

---

-- commits, line 6 at r2:

Previously, mkow (Michał Kowalczyk) wrote…

trailing (?

Fixed.

---

LibOS/shim/src/sys/shim_open.c, line 30 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

For my own understanding: so it was always broken? Why does #29 talk only about a regression after your Inodes commit? What did Inodes commit reveal that some LTP tests started failing?

There was an additional check in chroot/fs.c before. I'm trying to move all these checks out of individual filesystems so that there is less duplication.

(I know there is now duplication across different syscalls, but I think that's more manageable.)

---

LibOS/shim/test/ltp/runltp_xml.py, line 321 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Tbh I don't remember this discussion, Paweł's fix looks good.

I meant that the issue we encountered here is a strong argument against not parsing the output (and trusting LTP's return code). Since we now know LTP to sometimes return 0 even if some tests fail, we cannot trust the 0 return code.

[woju]

Reviewed 1 of 4 files at r2.
Reviewable status: all files reviewed, 4 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, @dimakuv, and @mkow)

---

LibOS/shim/test/ltp/runltp_xml.py, line 321 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I meant that the issue we encountered here is a strong argument against not parsing the output (and trusting LTP's return code). Since we now know LTP to sometimes return 0 even if some tests fail, we cannot trust the 0 return code.

The parser is sh*tty, so I didn't want to run it if it's not needed (hence if must-pass is empty, we don't, and also that's why we detect the situation when it should be removed and bother the developer). The underlying assumption was that the returncode correctly reflects the overall test result. IIUC this assumption is false? If so, this change is unfortunately correct.

[pwmarcz]

Reviewable status: 3 of 4 files reviewed, 4 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, @dimakuv, @mkow, and @woju)

---

LibOS/shim/test/ltp/runltp_xml.py, line 321 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

The parser is sh*tty, so I didn't want to run it if it's not needed (hence if must-pass is empty, we don't, and also that's why we detect the situation when it should be removed and bother the developer). The underlying assumption was that the returncode correctly reflects the overall test result. IIUC this assumption is false? If so, this change is unfortunately correct.

Okay, @woju explained to me the issue with MAP_SHARED, I agree it's Gramine's fault.

I'll add an explanation here so that it's written somewhere (instead of remaining oral knowledge).

[boryspoplawski]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv, @mkow, and @pwmarcz)

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I'm seeing it in Reviewable: the diff is empty; the change appears when I select a range like ⊥..r1 or r1..r2.

In that box over file matrix, in the left column you have the number of files. Under it there''s "+1 obsolete" and you can click on "reveal".

---

[dimakuv]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @boryspoplawski, @mkow, and @pwmarcz)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

In that box over file matrix, in the left column you have the number of files. Under it there''s "+1 obsolete" and you can click on "reveal".

No, I don't have it. I know about this "obsolete" thingy in Reviewable, but in this particular PR I don't see it. I can only assume that it's because I was late to the party and haven't seen the original changes in chroot_fs.c, so Reviewable decided I should never learn it.

---

[mkow]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @boryspoplawski, @dimakuv, and @pwmarcz)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

No, I don't have it. I know about this "obsolete" thingy in Reviewable, but in this particular PR I don't see it. I can only assume that it's because I was late to the party and haven't seen the original changes in chroot_fs.c, so Reviewable decided I should never learn it.

Weird, you should have had it in ⊥..r1 if you expand the "Reverted" section. I don't like this auto-hiding.

---

[pwmarcz]

Jenkins, retest Jenkins-Debug-18.04 please (random timeout on LTP tests, we've seen that before)

[dimakuv]

Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

Weird, you should have had it in ⊥..r1 if you expand the "Reverted" section. I don't like this auto-hiding.

I don't :( But whatever

---

[boryspoplawski]

Reviewable status: complete! all files reviewed, all discussions resolved

[mkow]

Reviewable status: complete! all files reviewed, all discussions resolved