[LibOS] Convert fs.mount to an array

[pwmarcz]

Description of the changes

This converts the fs.mount manifest key to an array instead of table. Similar to sgx.{trusted,allowed,protected}_files, an array can be parsed more efficiently. It's also more natural, because order matters when specifying mount points.

The old (table) syntax is still accepted, but triggers a deprecation warning.

This commit converts the main manifest for regression tests. Other manifests will be converted later.

In addition, Gramine now allows omitting the type key (with "chroot" as default) and the uri key (so that it's easier to use the tmpfs filesystem).

Issue: #23 (although we might want to close it after the deprecated syntax is gone).

Prerequisite for #371: new syntax for trusted/protected files.

How to test this PR?

The manifest.template in LibOS regression tests should exercise the new code. You can modify it to check how things fail.

---

This change is 

[mkow]

Reviewed 3 of 3 files at r1, all commit messages.
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) (waiting on @pwmarcz)

---

Documentation/manifest-syntax.rst, line 335 at r1 (raw file):

^^^^^^^^^^^^^^^

::

It was already like this before, but could you use TOML syntax highlighting here?

---

Documentation/manifest-syntax.rst, line 342 at r1 (raw file):

    ]

    # or, as separate sections:

I'd rather move this comment to normal text and split these snippets into two (it will be more visible that these are alternatives, not just one long snippet).

---

Documentation/manifest-syntax.rst, line 383 at r1 (raw file):

  host-level files. The tmpfs files are created under ``[PATH]`` (this path is
  empty on Gramine instance startup) and are destroyed when a Gramine instance
  terminates. The ``[URI]`` parameter is always ignored, and can be omitted.

Maybe we should actually error out on this? (after some grace period with only a warning)

Code quote:

and can be omitted

---

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

}

static int mount_others_from_toml_array(void) {

What does this "others" mean here and in the function above? I was always puzzled by this mount_one_other.

---

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

        }

        /* Prepare a key for `mount_one_other`, so that it in case of errors it will display paths

Too many "it"s?

Code quote:

it in case of errors it will

[dimakuv]

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

---

Documentation/manifest-syntax.rst, line 342 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'd rather move this comment to normal text and split these snippets into two (it will be more visible that these are alternatives, not just one long snippet).

+1

---

Documentation/manifest-syntax.rst, line 361 at r1 (raw file):

The ``fs.mount`` option is a TOML array of tables. As shown in the examples
above, you can either specify its value using the inline syntax, or use multiple
``[[fs.mount]]`` sections.

Why is this paragraph useful? I think it only duplicates the example above. Developers (who know TOML) won't get anything new from this para, and casual users will only get confused. So I think we should remove this para.

---

Documentation/manifest-syntax.rst, line 366 at r1 (raw file):

   If you use the inline table syntax (``{ ... }``), each table should fit in
   a single line. While multi-line tables are currently accepted by Gramine,
   they are not valid TOML and Gramine might disallow them in the future.

I would remove this note as well. This is a controversial feature of TOML, and chances are high that it will be the TOML spec that will allow this, not Gramine that will disallow this. Anyway, the users don't read these fine prints anyway, they just look at the examples under CI-Examples :)

---

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

        ret = mount_fs("chroot", fs_root_uri, "/");
    } else {
        ret = mount_fs(fs_root_type, fs_root_uri ?: "", "/");

What is this fs_root_uri ?: ""? This case is like this:

fs_root_type = "tmpfs"
fs_root_uri = NULL

I guess you're replacing NULL with an empty string to not deal with a NULL corner case?

---

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

        ret = mount_fs("chroot", mount_uri, mount_path);
    } else {
        ret = mount_fs(mount_type, mount_uri ?: "", mount_path);

ditto (why empty string instead of NULL?)

---

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

            "Root mount / already exists, verify that there are no duplicate mounts in manifest\n"
            "(note that root / is automatically mounted in Gramine and can be changed via "
            "'fs.root' manifest entry).\n");

Why did you remove this?

---

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

    if (!strcmp(mount_path, ".") || !strcmp(mount_path, "..")) {
        log_error("Mount points '.' and '..' are not allowed, remove them from manifest.");

Why did you remove this?

---

LibOS/shim/test/regression/manifest.template, line 18 at r1 (raw file):

  { path = "/usr/{{ arch_libdir }}", uri = "file:/usr/{{ arch_libdir }}" },
  { path = "/bin", uri = "file:/bin" },
  { type = "tmpfs", path = "/mnt/tmpfs" },

Maybe separate this tmpfs entry with a newline? For readability.

---

LibOS/shim/test/regression/manifest.template, line 19 at r1 (raw file):

  { path = "/bin", uri = "file:/bin" },
  { type = "tmpfs", path = "/mnt/tmpfs" },
]

Could we indent uri fields at the same level, so it's easier to read?

Right now it's messy and very hard to read: https://github.com/gramineproject/gramine/blob/pawel/mount-array-2/LibOS/shim/test/regression/manifest.template

[pwmarcz]

Reviewable status: 0 of 3 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "squash! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @pwmarcz)

---

Documentation/manifest-syntax.rst, line 335 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

It was already like this before, but could you use TOML syntax highlighting here?

Done (the highlight directive at the top).

---

Documentation/manifest-syntax.rst, line 342 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

+1

Done.

---

Documentation/manifest-syntax.rst, line 361 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why is this paragraph useful? I think it only duplicates the example above. Developers (who know TOML) won't get anything new from this para, and casual users will only get confused. So I think we should remove this para.

OK, I agree it doesn't add much.

---

Documentation/manifest-syntax.rst, line 366 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I would remove this note as well. This is a controversial feature of TOML, and chances are high that it will be the TOML spec that will allow this, not Gramine that will disallow this. Anyway, the users don't read these fine prints anyway, they just look at the examples under CI-Examples :)

The actual problem here is that we disallow a trailing comma in tables. This will work in Gramine:

fs.mount = [
  {
    path = "/bin",
    uri = "file:/usr/bin"
  },
]

This will break with a cryptic message from the Python library (when parsed by gramine-manifest):

fs.mount = [
  {
    path = "/bin",
    uri = "file:/usr/bin",
  },
]

If I don't discourage multi-line tables here, I think I need to at least add a warning about the traling comma. Do you think that's a better option?

---

Documentation/manifest-syntax.rst, line 383 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Maybe we should actually error out on this? (after some grace period with only a warning)

Should we? I don't think it hurts us to allow it, and it only makes the rules/code more complicated.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What is this fs_root_uri ?: ""? This case is like this:

fs_root_type = "tmpfs"
fs_root_uri = NULL

I guess you're replacing NULL with an empty string to not deal with a NULL corner case?

That's right. This parameter goes to the individual filesystems (chroot, pseudo-FS) and I don't want them all to check against NULL too.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why did you remove this?

Right, this should stay (I reworded it though).

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why did you remove this?

That one was by accident too. I restored it.

Actually, this rule is kind of arbitrary. As discussed on Gitter, I'd like to deprecate all relative paths: they are interpreted relative to root anyway.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

What does this "others" mean here and in the function above? I was always puzzled by this mount_one_other.

Filesystems other than the root one (fs.root). I can't think of a better name now, but I can change if you suggest one. :)

---

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

Previously, mkow (Michał Kowalczyk) wrote…

Too many "it"s?

Right. Fixed.

---

LibOS/shim/test/regression/manifest.template, line 18 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Maybe separate this tmpfs entry with a newline? For readability.

Done.

---

LibOS/shim/test/regression/manifest.template, line 19 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could we indent uri fields at the same level, so it's easier to read?

Right now it's messy and very hard to read: https://github.com/gramineproject/gramine/blob/pawel/mount-array-2/LibOS/shim/test/regression/manifest.template

I don't like this. It might not matter too much for this specific manifest, but I don't think that should be our recommended practice.

In the case of fs.mount, the paths can get quite long, e.g. if you specify individual files. Adding a new path will likely mean having to realign the whole array, and break git diff/blame in the process.

If the inline syntax is hard to read without such special formatting, maybe that means we should not use it? We can instead use the multi-line syntax (but it's non-standard, and annoying because of the trailing comma thing), or TOML sections.

[dimakuv]

Reviewed 3 of 3 files at r2, 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), "squash! " found in commit messages' one-liners (waiting on @mkow and @pwmarcz)

---

Documentation/manifest-syntax.rst, line 366 at r1 (raw file):

If I don't discourage multi-line tables here, I think I need to at least add a warning about the traling comma. Do you think that's a better option?

Yes, I would prefer this warning (about the trailing comma).

---

LibOS/shim/test/regression/manifest.template, line 19 at r1 (raw file):

Adding a new path will likely mean having to realign the whole array, and break git diff/blame in the process.

That's a good point. Let's not re-align then, but keep your current version.

If the inline syntax is hard to read without such special formatting, maybe that means we should not use it? We can instead use the multi-line syntax (but it's non-standard, and annoying because of the trailing comma thing), or TOML sections.

No, the current version is definitely better than the verbose TOML sections. I like it very much that this manifest is short and crisp.

[pwmarcz]

Reviewable status: 2 of 3 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 @dimakuv and @mkow)

---

Documentation/manifest-syntax.rst, line 366 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

If I don't discourage multi-line tables here, I think I need to at least add a warning about the traling comma. Do you think that's a better option?

Yes, I would prefer this warning (about the trailing comma).

Done.

[dimakuv]

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

[pwmarcz]

Reviewable status: 2 of 3 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 and @mkow)

---

Documentation/manifest-syntax.rst, line 335 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Done (the highlight directive at the top).

Sorry, I actually cannot :) See the TODO I added.

Probably the best solution is to change the "meta-syntax" in this file, but I'm out of ideas, and I don't want to solve it right now...

[mkow]

Reviewed 2 of 3 files at r2, 1 of 1 files at r4, 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)

---

Documentation/manifest-syntax.rst, line 335 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Sorry, I actually cannot :) See the TODO I added.

Probably the best solution is to change the "meta-syntax" in this file, but I'm out of ideas, and I don't want to solve it right now...

Ah, too bad :( Let's leave it for later then.

---

Documentation/manifest-syntax.rst, line 383 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Should we? I don't think it hurts us to allow it, and it only makes the rules/code more complicated.

My idea was that it could prevent user mistakes / confusion when they misunderstand how it works and set it, but then it's silently ignored. But not blocking.

---

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Filesystems other than the root one (fs.root). I can't think of a better name now, but I can change if you suggest one. :)

Maybe just nonroot?

[pwmarcz]

Reviewable status: 2 of 3 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 and @mkow)

---

Documentation/manifest-syntax.rst, line 383 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

My idea was that it could prevent user mistakes / confusion when they misunderstand how it works and set it, but then it's silently ignored. But not blocking.

OK. I want to leave it like this for now, because this section of manifest is going to change anyway (with trusted/protected files) and keeping the rules simple will help me refactor. But let's watch out for user problems.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

Maybe just nonroot?

OK. 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

[dimakuv]

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

[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 all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved