[LibOS] Add support for two new pseudo files in /proc dir

[NirjharRoyiitk]

The files which are added are:

• /proc/sys/fs/pipe-max-size
• /proc/sys/fs/lease-break-time

The values are hard-coded and taken from Linux v6.2, see:

• https://elixir.bootlin.com/linux/v6.2/source/fs/pipe.c#L54
• https://elixir.bootlin.com/linux/v6.2/source/fs/locks.c#L93

Description of the changes

How to test this PR?

---

This change is 

[dimakuv]

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

a discussion (no related file):
Wait, why do we have a test under test/fs/? This test doesn't use any "real" files, so this test should be under test/regression/.

---

---

-- commits line 6 at r1:
It would be good to know which workload (and why) requires this file. I can understand the need for pipe-max-size, but lease-break-time file seems super-specific. (I read its description here: http://www.linux-admins.net/2010/09/all-you-need-to-know-about-procsys.html)

---

libos/include/libos_types.h line 167 at r1 (raw file):

/* The following values are taken from Linux v6.2 */
#define PIPE_MAX_SIZE 1048576
#define LEASE_BREAK_TIME_MAX 45

These two macros are only used in libos/src/fs/proc/fs.c -- let's move them there.

---

libos/include/libos_types.h line 167 at r1 (raw file):

/* The following values are taken from Linux v6.2 */
#define PIPE_MAX_SIZE 1048576
#define LEASE_BREAK_TIME_MAX 45

Why do you call it LEASE_BREAK_TIME_MAX, why not just LEASE_BREAK_TIME?

---

libos/src/fs/proc/fs.c line 36 at r1 (raw file):

static int proc_lease_break_time_load(struct libos_dentry* dent, char** out_data,
    size_t* out_size) {

Please fix indentation (must be aligned to the first argument of the func)

---

libos/src/fs/proc/fs.c line 39 at r1 (raw file):

    __UNUSED(dent);

    size_t buffer_size = 8; /* enough to hold LEASE_BREAK_TIME_MAX */

Please use 16, because you assume that LEASE_BREAK_TIME_MAX can be up to UINT_MAX below

---

libos/src/fs/proc/fs.c line 57 at r1 (raw file):

}

static int proc_pipe_max_load(struct libos_dentry* dent, char** out_data, size_t* out_size) {

Please rename to proc_pipe_max_size_load

---

libos/src/fs/proc/fs.c line 75 at r1 (raw file):

    struct pseudo_node* kernel = pseudo_add_dir(sys, "kernel");
    pseudo_add_str(kernel, "pid_max", &proc_pid_max_load);

Please don't delete this line -- it separates kernel/ files from proc/ (root) files

---

libos/src/fs/proc/fs.c line 120 at r1 (raw file):

    pseudo_add_str(fs, "pipe-max-size", &proc_pipe_max_load);
    pseudo_add_str(fs, "lease-break-time", &proc_lease_break_time_load);

Please add an empty line so that fs/ files are separated from kernel/ files visually

---

libos/test/fs/proc_pseudo_files.c line 22 at r1 (raw file):

#define BUF_SZ 1024
/* The following values are defined in gramine/libos/include/libos_types.h */

This comment will be changed once you move the macros.

---

libos/test/fs/test_fs.py line 348 at r1 (raw file):

class TC_02_Proc_Pseudo_Files(RegressionTestCase):
    TEST_DIR = 'tmp'

What's this whole deal with tmp dir? It seems completely unnecessary for the test.

[NirjharRoyiitk]

Reviewable status: 0 of 11 files reviewed, 11 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Wait, why do we have a test under test/fs/? This test doesn't use any "real" files, so this test should be under test/regression/.

Done.

---

---

-- commits line 6 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

It would be good to know which workload (and why) requires this file. I can understand the need for pipe-max-size, but lease-break-time file seems super-specific. (I read its description here: http://www.linux-admins.net/2010/09/all-you-need-to-know-about-procsys.html)

Trying to find out

---

libos/src/fs/proc/fs.c line 36 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please fix indentation (must be aligned to the first argument of the func)

Done.

---

libos/src/fs/proc/fs.c line 39 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please use 16, because you assume that LEASE_BREAK_TIME_MAX can be up to UINT_MAX below

Done.

---

libos/src/fs/proc/fs.c line 57 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please rename to proc_pipe_max_size_load

Done.

---

libos/src/fs/proc/fs.c line 75 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please don't delete this line -- it separates kernel/ files from proc/ (root) files

Done.

---

libos/src/fs/proc/fs.c line 120 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please add an empty line so that fs/ files are separated from kernel/ files visually

Done.

---

libos/include/libos_types.h line 167 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

These two macros are only used in libos/src/fs/proc/fs.c -- let's move them there.

Done.

---

libos/include/libos_types.h line 167 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why do you call it LEASE_BREAK_TIME_MAX, why not just LEASE_BREAK_TIME?

Done.

---

libos/test/fs/test_fs.py line 348 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What's this whole deal with tmp dir? It seems completely unnecessary for the test.

Done.

---

libos/test/fs/proc_pseudo_files.c line 22 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This comment will be changed once you move the macros.

Done.

[dimakuv]

Reviewed 11 of 11 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @NirjharRoyiitk)

---

libos/src/fs/proc/fs.c line 35 at r2 (raw file):

    *out_data = buffer;
    *out_size = buffer_size;

Shouldn't we also fix here to ret?

---

libos/test/regression/test_libos.py line 1021 at r2 (raw file):

        self.assertIn('/proc/1/root: link: /', lines)

    def test_001_proc(self):

Let's have a more specific name: test_001_proc_hardcoded_files

---

libos/test/regression/test_libos.py line 1025 at r2 (raw file):

        self.assertIn('TEST OK', stdout)

    def test_001_devfs(self):

You'll need to shift this test's number to 002 (and the one below it too)

---

libos/test/regression/proc_pseudo_files.c line 9 at r2 (raw file):

 * contents. */

#include <assert.h>

I think we should rename this test from very generic proc_pseudo_files to a more specific proc_hardcoded_files -- this name implies that we're opening these files and checking their hard-coded contents.

This means that all the corresponding filenames and test names should be also changed in this PR

---

libos/test/regression/proc_pseudo_files.c line 48 at r2 (raw file):

    for (size_t i = 0; i < sizeof(tc)/sizeof(*tc); i++) {
        memset(buf, 0, sizeof(buf));
        int fd = open(tc[i].path, O_RDONLY);

Please apply CHECK() macro here

---

libos/test/regression/proc_pseudo_files.c line 51 at r2 (raw file):

        if (fd < 0)
            errx(1,"opening file %s failed", tc[i].path);
        ssize_t read_bytes = read(fd, buf, sizeof(buf));

Please apply CHECK() macro here

[NirjharRoyiitk]

Reviewable status: 6 of 11 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv)

---

libos/src/fs/proc/fs.c line 35 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Shouldn't we also fix here to ret?

yes but since it wasnt't related to this PR I didn't touch it.

---

libos/test/regression/test_libos.py line 1025 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You'll need to shift this test's number to 002 (and the one below it too)

Done.

---

libos/test/regression/proc_pseudo_files.c line 9 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I think we should rename this test from very generic proc_pseudo_files to a more specific proc_hardcoded_files -- this name implies that we're opening these files and checking their hard-coded contents.

This means that all the corresponding filenames and test names should be also changed in this PR

Done.

---

libos/test/regression/proc_pseudo_files.c line 48 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please apply CHECK() macro here

As discussed, since we are checking for multiple files here, using CHECK here won't tell us exactly for which the system call is failing. In this case it won't be enough just to print the line number and the error number and we also need the file name for which the failure is occurring.

---

libos/test/regression/proc_pseudo_files.c line 51 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please apply CHECK() macro here

ditto

[dimakuv]

Reviewed 4 of 5 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @NirjharRoyiitk)

---

libos/src/fs/proc/fs.c line 35 at r2 (raw file):

Previously, NirjharRoyiitk wrote…

yes but since it wasnt't related to this PR I didn't touch it.

Well, looks like it's a benign thing anyway, let me resolve it.

---

libos/test/regression/test_libos.py line 1038 at r4 (raw file):

        self.assertIn('TEST OK', stdout)

    def test_003_proc_hardcoded_files(self):

Yep, moving the new test to the end of the list also works :)

---

libos/test/regression/test_libos.py line 1039 at r4 (raw file):

    def test_003_proc_hardcoded_files(self):
        stdout, stderr = self.run_binary(['proc_hardcoded_files'])

You don't use stderr, so just replace it with _. See other tests for example.

---

libos/test/regression/proc_hardcoded_files.c line 50 at r4 (raw file):

        int fd = open(tc[i].path, O_RDONLY);
        if (fd < 0)
            errx(1,"opening file %s failed", tc[i].path);

Please use err() -- this will also print the error number/description

---

libos/test/regression/proc_hardcoded_files.c line 50 at r4 (raw file):

        int fd = open(tc[i].path, O_RDONLY);
        if (fd < 0)
            errx(1,"opening file %s failed", tc[i].path);

Please add a space after 1,

---

libos/test/regression/proc_hardcoded_files.c line 64 at r4 (raw file):

        int ret = stat(tc[i].path, &sb);
        if (ret < 0)
            errx(1, "stat failed for file %s with error code = %d", tc[i].path, errno);

You should replace it with err() -- this way you can remove the "with error code ..." and errno -- this standard function automatically prints the error number/description

---

libos/test/regression/proc_hardcoded_files.c line 70 at r4 (raw file):

        if (ret < 0)
            errx(1, "close() failed for file %s fd = %d with error code = %d", tc[i].path, fd,
                 errno);

You should replace it with err() -- this way you can remove the "with error code ..." and errno -- this standard function automatically prints the error number/description

Also, there is no reason to print fd -- this number is not meaningful to end users anyway.

---

libos/test/regression/proc_pseudo_files.c line 48 at r2 (raw file):

Previously, NirjharRoyiitk wrote…

As discussed, since we are checking for multiple files here, using CHECK here won't tell us exactly for which the system call is failing. In this case it won't be enough just to print the line number and the error number and we also need the file name for which the failure is occurring.

Ah, I forgot about this discussion :)

[NirjharRoyiitk]

Reviewable status: 10 of 11 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv)

---

libos/test/regression/proc_hardcoded_files.c line 50 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please use err() -- this will also print the error number/description

Done.

---

libos/test/regression/proc_hardcoded_files.c line 50 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please add a space after 1,

Done.

---

libos/test/regression/proc_hardcoded_files.c line 64 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You should replace it with err() -- this way you can remove the "with error code ..." and errno -- this standard function automatically prints the error number/description

Done.

---

libos/test/regression/proc_hardcoded_files.c line 70 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You should replace it with err() -- this way you can remove the "with error code ..." and errno -- this standard function automatically prints the error number/description

Also, there is no reason to print fd -- this number is not meaningful to end users anyway.

Done.

[NirjharRoyiitk]

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

---

libos/test/regression/test_libos.py line 1039 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You don't use stderr, so just replace it with _. See other tests for example.

Done.

[dimakuv]

Reviewed 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @NirjharRoyiitk)

---

libos/test/regression/proc_hardcoded_files.c line 56 at r6 (raw file):

        if (read_bytes != tc[i].expected_length) {
            errx(1, "Content length mismatch for file = %s. Expected %ld got %ld", tc[i].path,
                tc[i].expected_length, read_bytes);

Alignment is wrong now (please add one more space)

---

libos/test/regression/proc_hardcoded_files.c line 59 at r6 (raw file):

        }
        if (strcmp(tc[i].expected_value, buf)) {
            errx(1,"Content mismatch for file = %s. Expected %s got %s", tc[i].path,

Here you also need to add a space

---

libos/test/regression/proc_hardcoded_files.c line 68 at r6 (raw file):

            err(1, "stat failed for file %s", tc[i].path);
        if (!S_ISREG(sb.st_mode))
            errx(1,"Unexpected type for file = %s. Expected S_ISREG", tc[i].path);

Here you also need to add a space

[NirjharRoyiitk]

Reviewable status: 10 of 11 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv)

---

libos/test/regression/proc_hardcoded_files.c line 56 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Alignment is wrong now (please add one more space)

Done.

---

libos/test/regression/proc_hardcoded_files.c line 59 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Here you also need to add a space

Done.

---

libos/test/regression/proc_hardcoded_files.c line 68 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Here you also need to add a space

Done.

[dimakuv]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: )

[mkow]

Reviewed 5 of 11 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 (2 more required, approved so far: ) (waiting on @dimakuv and @NirjharRoyiitk)

---

-- commits line 38 at r7:
@NirjharRoyiitk: Please read https://gramine.readthedocs.io/en/latest/devel/contributing.html#pr-life-cycle :)

---

libos/src/fs/proc/fs.c line 16 at r7 (raw file):

/* The following values are taken from Linux v6.2 */
#define PIPE_MAX_SIZE 1048576
#define LEASE_BREAK_TIME 45

But these values are incorrect for Gramine, we don't have these limits set this way...