[LibOS] Rewrite string-based files #2584

pwmarcz · 2021-07-27T11:25:30Z

Description of the changes

This change decouples the two usages of "str" filesystem: "pseudo" filesystems, and tmpfs. The difference between them is that in "pseudo" filesystems, the file data is stored directly in handle.

Instead of str_* functions, there is a simpler set of mem_file_* functions that do not operate on handles or dentries, just on its own buffer.

The decoupling makes both use-cases simpler: there is no structure "inheritance" and no extra layer of reference counting.

Next steps

I converted file position from a pointer to file_off_t, so it brings us a step closer to pushing the file position upwards (so that it's not handled by individual filesystems).
I got rid of the reference counting here, which unblocks fix to dentry lifecycle (I made a first attempt in [LibOS] Do not reuse dentries after removing a file (fixes mknod crash) #2499 but got blocked by this).
- Instead of a bit field, the dentry state can be an enum; something like "new -> negative (file doesn't exist yet) -> valid -> deleted (after unlink)".
The last filesystem I want to rewrite is chroot, but it turns out that it has some more advanced handling of deleted files, so I'll be trying to include that in dentry lifecycle.
- To be more specific, the chroot filesystem allows you to have a handle to "old" deleted file, and a dentry to the "new" one; this is done using a version field. Instead, I want these files to be described by two separate dentries.

How to test this PR?

The existing tests should be enough.

This change is

mkow

Reviewed 9 of 9 files at r1.
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) (waiting on @pwmarcz)

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

    if ((uintptr_t)dent1 < (uintptr_t)dent2) {
        unlock(&dent1->lock);
        unlock(&dent2->lock);

Not the other way around?

dimakuv

Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 19 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)

LibOS/shim/include/shim_fs_mem.h, line 19 at r1 (raw file):

    char* buf;
    file_off_t size;
    file_off_t buf_size;

Shouldn't this be size_t? Because it is the size of the object in memory, not the size of the file.

LibOS/shim/include/shim_fs_mem.h, line 26 at r1 (raw file):

/*
 * The following operations correspond to the filesystem callbacks (see `shim_fs.h`). Note that the

Correspond? They do not in terms of their arguments...

LibOS/shim/include/shim_fs_mem.h, line 27 at r1 (raw file):

/*
 * The following operations correspond to the filesystem callbacks (see `shim_fs.h`). Note that the
 * caller has to pass the file position, and (in case of `read` and `write` update it after a

Forgot the )

LibOS/shim/include/shim_handle.h, line 34 at r1 (raw file):

    TYPE_FILE,       /* host files, used by `chroot` filesystem */
    TYPE_DEV,        /* emulated devices, used by `dev` filesystem */
    TYPE_STR,        /* string-based files (with data inside handle), handled by `pseudo_*`

Why do string-based files have data inside the handle and not inside the dentry? Is it just because it is simpler to write code? (I understand that with pseudo-files it is impossible to have several handles on the same dentry, so I understand that this solution is possible, just curious as to why have this distinction).

LibOS/shim/include/shim_handle.h, line 35 at r1 (raw file):

    TYPE_DEV,        /* emulated devices, used by `dev` filesystem */
    TYPE_STR,        /* string-based files (with data inside handle), handled by `pseudo_*`
                      * functions, used by several filesystems */

Is it true that it is "used by several filesystems"? I guess you mean sysfs, procfs?

LibOS/shim/include/shim_handle.h, line 181 at r1 (raw file):

struct shim_tmpfs_data {
    struct shim_mem_file mem;
};

This struct looks like an internal detail of tmpfs/fs.c. Maybe move it there?

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

#include "shim_fs_mem.h"

static int mem_file_resize(struct shim_mem_file* mem, file_off_t buf_size) {

Shouldn't buf_size be of type size_t?

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

/* SPDX-License-Identifier: LGPL-3.0-or-later */
/* Copyright (C) 2021 Intel Corporation
 *                    Li Xun <xun.li@intel.com>

Please add your name here

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

/* Get data associated with dentry. This is created on demand, instead of during dentry validation,
 * because dentries restored from checkpoint don't have the `data` field filled. */

I don't understand this second part, dentries restored from checkpoint... -- but we do not checkpoint shim_tmpfs_data during checkpointing. So does it mean that the child process will see the tmpfs dentry, but it will be empty? This is very weird semantics, why use it?

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

    mem_file_init(&data->mem, /*data=*/NULL, /*size=*/0);

    *out_data = dent->data = data;

I highly dislike this pattern (my brain is wired to read it as *out_data = dent->data == data), please split into two lines.

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

    if (!dent->parent) {
        /* This is the root dentry, initialize it. */
        dent->state |= DENTRY_ISDIRECTORY;

This should be removed after #2587 is merged.

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

static int tmpfs_readdir(struct shim_dentry* dent, readdir_callback_t callback, void* arg) {
    struct shim_dentry* child;
    LISTP_FOR_EACH_ENTRY(child, &dent->children, siblings) {

Why don't you grab the dentry lock before this loop? Is it already taken by the caller? If yes, can we add an assert?

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

    if (dent->type == S_IFDIR) {
        struct shim_dentry* child;
        LISTP_FOR_EACH_ENTRY(child, &dent->children, siblings) {

Why don't you grab the dentry lock before this loop? Is it already taken by the caller? If yes, can we add an assert?

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

    /* TODO: our `unlink` wipes the file data, even though there might be still file handles
     * pointing to the file. A proper solution would keep the unlinked file until all handles are
     * closed, but also allow creating a new file with the same name. */

This sounds very dangereous, we currently allow dangling pointers. Firstly, why can't we implement the proper solution yet? Is it because we would need to rewire the handle to shim_tmpfs_data in case of dentry deletion -- and this sounds like it needs a proper, non-trivial code changes?

Secondly, wouldn't it be better to have a memory leak here, instead of dangling pointers? Let's just not free this memory region. UPDATE: Actually, we don't have mmap for tmpfs, so how could the file handle point to the file after dentry deletion?

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

Previously, mkow (Michał Kowalczyk) wrote…

Not the other way around?

+1 to Michal.

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

static int tmpfs_rename(struct shim_dentry* old, struct shim_dentry* new) {
    lock_two_dentries(old, new);
    new->data = old->data;

Is it possible that new->data already points to some data? If it's impossible, can we add an assert about it?

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

    assert(hdl->type == TYPE_TMPFS);

    __UNUSED(hdl);

This can be removed.

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

    tmpfs_data->ctime = time / 1000000;
    tmpfs_data->mtime = tmpfs_data->ctime;

We previously updated this create/modify timestamps on tmpfs operations. We don't want to do this anymore? Is it because they are considered useless?

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

    __UNUSED(hdl);

    lock(&hdl->dentry->lock);

I'm getting confused again regarding the handle and its associated dentry... Can the handle change the dentry in the meantime? If yes, shouldn't you lock(&hdl->lock)?

pwmarcz

Reviewable status: 5 of 13 files reviewed, 19 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, @mkow, and @pwmarcz)

LibOS/shim/include/shim_fs_mem.h, line 19 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Shouldn't this be size_t? Because it is the size of the object in memory, not the size of the file.

Yeah, unfortunately it's awkward either way... Updated.

LibOS/shim/include/shim_fs_mem.h, line 26 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Correspond? They do not in terms of their arguments...

I reworded the comment. I still think the API is pretty self-explanatory, so adding proper Doxygen comments would be an overkill. Do you agree?

LibOS/shim/include/shim_fs_mem.h, line 27 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Forgot the )

Fixed.

LibOS/shim/include/shim_handle.h, line 34 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why do string-based files have data inside the handle and not inside the dentry? Is it just because it is simpler to write code? (I understand that with pseudo-files it is impossible to have several handles on the same dentry, so I understand that this solution is possible, just curious as to why have this distinction).

You can have several file handles on the same entry, say, if you open /proc/cpuinfo multiple times.

However, it's easier to store the data in the dentry because then it can be computed on-the-fly, and changed every time you open it again.

From a (very) brief investigation, this is also what Linux does: if you open a pseudo-file /proc/cpuinfo, it ends up calling a callback that computes the file content and associates it with the file handle.

LibOS/shim/include/shim_handle.h, line 35 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Is it true that it is "used by several filesystems"? I guess you mean sysfs, procfs?

Yes. I think just "handled by pseudo_* functions" will be clearer.

LibOS/shim/include/shim_handle.h, line 181 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This struct looks like an internal detail of tmpfs/fs.c. Maybe move it there?

OK. I thought there was a precedent for keeping these shim_*_data definitions in shim_handle.h as well, but right now that's only for chroot (which I want to also change).

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Shouldn't buf_size be of type size_t?

Done.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please add your name here

Done.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I don't understand this second part, dentries restored from checkpoint... -- but we do not checkpoint shim_tmpfs_data during checkpointing. So does it mean that the child process will see the tmpfs dentry, but it will be empty? This is very weird semantics, why use it?

Well, this is the default behaviour of Graphene: dentries are checkpointed but the data field is wiped. It seems hard to do something else right now.

Longer term, the filesystem probably should have callbacks for checkpointing data. There is something like that for handles (checkin / checkout), but it's almost unused, and I'm afraid to build anything advanced on top of the current checkpointing system.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I highly dislike this pattern (my brain is wired to read it as *out_data = dent->data == data), please split into two lines.

OK, I'll keep that in mind. Done.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why don't you grab the dentry lock before this loop? Is it already taken by the caller? If yes, can we add an assert?

It is taken. I'm adding an assert.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why don't you grab the dentry lock before this loop? Is it already taken by the caller? If yes, can we add an assert?

Unfortunately it's not taken. I'm taking the lock here, but this means the unlink implementation is racy :( I added a TODO.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This sounds very dangereous, we currently allow dangling pointers. Firstly, why can't we implement the proper solution yet? Is it because we would need to rewire the handle to shim_tmpfs_data in case of dentry deletion -- and this sounds like it needs a proper, non-trivial code changes?

Secondly, wouldn't it be better to have a memory leak here, instead of dangling pointers? Let's just not free this memory region. UPDATE: Actually, we don't have mmap for tmpfs, so how could the file handle point to the file after dentry deletion?

Edited: s/pointing to the file/associated with the dentry/

Does that make more sense? There are no dangling pointers, it's just that after unlink the open file suddenly becomes empty.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

+1 to Michal.

Right! Fixed.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Is it possible that new->data already points to some data? If it's impossible, can we add an assert about it?

Right, new can be an existing file, with associated data. I need to delete it here.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This can be removed.

My mistake. Removed.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We previously updated this create/modify timestamps on tmpfs operations. We don't want to do this anymore? Is it because they are considered useless?

Looks like this got more attention than I initially assumed: in the original PR for tmpfs there is some discussion about updating the times. I'll keep them.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I'm getting confused again regarding the handle and its associated dentry... Can the handle change the dentry in the meantime? If yes, shouldn't you lock(&hdl->lock)?

hdl->dentry doesn't change during the dentry's lifetime. Then again, it's less confusing to take the lock (since this is the only place that doesn't do it). Updated.

dimakuv

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

LibOS/shim/include/shim_fs_mem.h, line 26 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I reworded the comment. I still think the API is pretty self-explanatory, so adding proper Doxygen comments would be an overkill. Do you agree?

LGTM. Yes, no need for detailed comments.

LibOS/shim/include/shim_handle.h, line 34 at r1 (raw file):

You can have several file handles on the same entry, say, if you open /proc/cpuinfo multiple times.

Oops, true, my mistake.

But then doesn't it mean that we will have 2 copies of the same string data when we open /proc/cpuinfo twice? And what happens if we open e.g. /dev/attestation/user_report_data twice for write? We can end up with two disparate copies?

However, it's easier to store the data in the dentry...

Is this a typo? Did you mean "handle" instead of "dentry"?

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Well, this is the default behaviour of Graphene: dentries are checkpointed but the data field is wiped. It seems hard to do something else right now.

Longer term, the filesystem probably should have callbacks for checkpointing data. There is something like that for handles (checkin / checkout), but it's almost unused, and I'm afraid to build anything advanced on top of the current checkpointing system.

Fair enough. But could you add a comment that this is "conforming to the current behavior of Graphene ..."?

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Edited: s/pointing to the file/associated with the dentry/

Does that make more sense? There are no dangling pointers, it's just that after unlink the open file suddenly becomes empty.

Yes, I understand what happens now.

LibOS/shim/test/fs/test_fs.py, line 316 at r2 (raw file):

        self.verify_copy(stdout, stderr, '/mounted/input', executable)

    def test_300_rename(self):

This test won't work on protected files right now: #2521

You may want to disable this test on protected files (I believe currently all tests from test_fs.py are inherited by test_pf.py).

LibOS/shim/test/fs/test_fs.py, line 321 at r2 (raw file):

        stdout, stderr = self.run_binary(['rename', file1, file2])
        self.assertIn('TEST OK', stdout)

This whole test doesn't seem to belong to test/fs/. Maybe better to move it under test/regression/?

The test/fs/ tests are mainly to check copying of a bunch of files (created in setUpClass() above). But in this test, you just have two files.

pwmarcz

Reviewable status: 6 of 18 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, @mkow, and @pwmarcz)

LibOS/shim/include/shim_handle.h, line 34 at r1 (raw file):

But then doesn't it mean that we will have 2 copies of the same string data when we open /proc/cpuinfo twice?

Yes, you will have two copies of the same string data, each living as long as the handle is open.

And what happens if we open e.g. /dev/attestation/user_report_data twice for write? We can end up with two disparate copies?

Yes. A close or fsync on a handle will cause the attestation driver to update the report data, but will not update what is visible in the other handle.

This is, apparently, not how writable pseudo-files work under Linux: I just did an experiment with /proc/sys/kernel/hostname, and changes to one handle are immediately visible in the other one.

However, it's easier to store the data in the dentry...

Is this a typo? Did you mean "handle" instead of "dentry"?

Sorry, typo, I meant in the handle.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Fair enough. But could you add a comment that this is "conforming to the current behavior of Graphene ..."?

Reworded to "Graphene's checkpointing system currently omits the data field", I hope that's clear enough.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This should be removed after #2587 is merged.

I rebased the branch, this is removed now.

LibOS/shim/test/fs/test_fs.py, line 321 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This whole test doesn't seem to belong to test/fs/. Maybe better to move it under test/regression/?

The test/fs/ tests are mainly to check copying of a bunch of files (created in setUpClass() above). But in this test, you just have two files.

Okay, I thought fs/ was a directory for filesystem tests in general, but then I see a lot of filesystem tests are in regression/. Sounds like it would be good to get rid of these fs tests, and also parametrize a bunch of regression tests by filesystem...

For now, I moved the rename test to regression and created two versions of it (for chroot and tmpfs).

dimakuv

Reviewed 2 of 4 files at r3, 10 of 10 files at r4.
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 @mkow and @pwmarcz)

LibOS/shim/include/shim_handle.h, line 34 at r1 (raw file):

This is, apparently, not how writable pseudo-files work under Linux: I just did an experiment with /proc/sys/kernel/hostname, and changes to one handle are immediately visible in the other one.

How do they work? Will it be hard to do a similar thing in our code?

Anyway, this is not a huge deal. We can just put a TODO comment somewhere in the code. I don't believe anyone opens the same pseudo-file twice for writing.

LibOS/shim/test/regression/rename.manifest.template, line 2 at r4 (raw file):

loader.preload = "file:{{ graphene.libos }}"
libos.entrypoint = "{{ env.PWD }}/rename"

Why not just rename?

LibOS/shim/test/regression/rename.manifest.template, line 9 at r4 (raw file):

fs.root.type = "chroot"
fs.root.path = "/"
fs.root.uri = "file:/"

Why mounting root?

LibOS/shim/test/regression/rename.manifest.template, line 20 at r4 (raw file):


sgx.trusted_files.runtime = "file:{{ graphene.runtimedir() }}/"
sgx.trusted_files.host_root_fs = "file:{{ env.PWD }}/host_root_fs"

What is this?

pwmarcz

Reviewable status: 17 of 19 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)

LibOS/shim/include/shim_handle.h, line 34 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is, apparently, not how writable pseudo-files work under Linux: I just did an experiment with /proc/sys/kernel/hostname, and changes to one handle are immediately visible in the other one.

How do they work? Will it be hard to do a similar thing in our code?

Anyway, this is not a huge deal. We can just put a TODO comment somewhere in the code. I don't believe anyone opens the same pseudo-file twice for writing.

I added a TODO (to shim_fs_pseudo.h), but I also don't think it's very important.

As an aside: even if we stored the data per-dentry for both of these implementations (pseudo and tmpfs), I would still want them to be separate. These are different use-cases, and they were a pain to work with when they were coupled.

LibOS/shim/test/regression/rename.manifest.template, line 2 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why not just rename?

Looks like I copied the wrong template (host_root_fs). Fied.

LibOS/shim/test/regression/rename.manifest.template, line 9 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why mounting root?

Same as above. Fixed.

LibOS/shim/test/regression/rename.manifest.template, line 20 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What is this?

Same as above. Fixed.

dimakuv

Reviewed 2 of 2 files at r5.
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 @mkow)

mkow

Reviewed 4 of 8 files at r2, 2 of 4 files at r3, 9 of 10 files at r4, 1 of 2 files at r5, 1 of 1 files at r6.
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), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

LibOS/shim/src/fs/shim_fs_mem.c, line 11 at r6 (raw file):

#define OVERFLOWS_FILE_OFF_T(n) __builtin_add_overflow_p((n), 0, (file_off_t)0)
#define OVERFLOWS_SIZE_T(n) __builtin_add_overflow_p((n), 0, (size_t)0)

I like these macros :)

LibOS/shim/src/fs/tmpfs/fs.c, line 53 at r6 (raw file):

    mem_file_init(&data->mem, /*data=*/NULL, /*size=*/0);

    uint64_t time;

Could you add _us suffix to this? (to say which unit it uses)

LibOS/shim/src/fs/tmpfs/fs.c, line 241 at r6 (raw file):

static int tmpfs_rename(struct shim_dentry* old, struct shim_dentry* new) {
    uint64_t time;

ditto

LibOS/shim/src/fs/tmpfs/fs.c, line 305 at r6 (raw file):

    assert(hdl->type == TYPE_TMPFS);

    uint64_t time;

ditto

LibOS/shim/src/fs/tmpfs/fs.c, line 333 at r6 (raw file):

    int ret;

    uint64_t time;

ditto

pwmarcz

Reviewable status: 18 of 19 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), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @pwmarcz)

LibOS/shim/src/fs/tmpfs/fs.c, line 53 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Could you add _us suffix to this? (to say which unit it uses)

Done.

mkow

Reviewed 1 of 1 files at r7.
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 r7.
Reviewable status: all files reviewed, 1 unresolved discussion, 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 (waiting on @pwmarcz)

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

sgx.trusted_files.entrypoint = "file:{{ entrypoint }}"

sgx.allowed_files.tmp_dir = "file:tmp/"

What is this? I don't see how this is relevant. Accidental change?

pwmarcz

Reviewable status: all files reviewed, 1 unresolved discussion, 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 (waiting on @dimakuv)

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What is this? I don't see how this is relevant. Accidental change?

This test renames tmp/file1 to tmp/file2, so it needs to be allowed to access them. It fails on SGX otherwise.

dimakuv

Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

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

Previously, pwmarcz (Paweł Marczewski) wrote…

This test renames tmp/file1 to tmp/file2, so it needs to be allowed to access them. It fails on SGX otherwise.

Ah, we have two new tests in test_libos.py. Sorry, forgot about this. I thought that this test just works on /mnt/tmpfs/

This change decouples the two usages of "str" filesystem: "pseudo" filesystems, and `tmpfs`. The difference between them is that in "pseudo" filesystems, the file data is stored directly in handle. Instead of `str_*` functions, there is a simpler set of `mem_file_*` functions that do not operate on handles or dentries, just on its own buffer. The decoupling makes both use-cases simpler: there is no structure "inheritance" and no extra layer of reference counting. Signed-off-by: Paweł Marczewski <pawel@invisiblethingslab.com>

Signed-off-by: Paweł Marczewski <pawel@invisiblethingslab.com>

dimakuv

Reviewed 4 of 4 files at r8.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)

mkow

Reviewed 4 of 4 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved

pwmarcz force-pushed the pawel/fs branch from a133f06 to 6ce34fb Compare July 27, 2021 13:39

Base automatically changed from pawel/fs to master July 27, 2021 14:08

pwmarcz force-pushed the pawel/tmpfs branch from 4b15e8e to fb324d9 Compare July 27, 2021 14:09

pwmarcz marked this pull request as ready for review July 27, 2021 14:10

mkow suggested changes Jul 28, 2021

View reviewed changes

dimakuv suggested changes Jul 28, 2021

View reviewed changes

pwmarcz commented Jul 28, 2021

View reviewed changes

dimakuv suggested changes Jul 28, 2021

View reviewed changes

pwmarcz force-pushed the pawel/tmpfs branch from 940cc48 to afaf9d3 Compare July 28, 2021 13:20

pwmarcz commented Jul 28, 2021

View reviewed changes

dimakuv suggested changes Jul 28, 2021

View reviewed changes

pwmarcz commented Jul 28, 2021

View reviewed changes

dimakuv previously approved these changes Jul 29, 2021

View reviewed changes

pwmarcz dismissed dimakuv’s stale review via b27d390 July 29, 2021 10:07

mkow suggested changes Jul 29, 2021

View reviewed changes

pwmarcz commented Jul 29, 2021

View reviewed changes

mkow previously approved these changes Jul 29, 2021

View reviewed changes

dimakuv suggested changes Jul 30, 2021

View reviewed changes

pwmarcz commented Jul 30, 2021

View reviewed changes

dimakuv previously approved these changes Jul 30, 2021

View reviewed changes

pwmarcz added 2 commits July 30, 2021 11:04

[LibOS] Add a test for file rename

45f5f12

Signed-off-by: Paweł Marczewski <pawel@invisiblethingslab.com>

pwmarcz dismissed stale reviews from dimakuv and mkow via 45f5f12 July 30, 2021 09:06

pwmarcz force-pushed the pawel/tmpfs branch from 99c5dbe to 45f5f12 Compare July 30, 2021 09:06

dimakuv approved these changes Jul 30, 2021

View reviewed changes

mkow approved these changes Jul 30, 2021

View reviewed changes

pwmarcz merged commit 45f5f12 into master Jul 30, 2021

pwmarcz deleted the pawel/tmpfs branch July 30, 2021 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LibOS] Rewrite string-based files #2584

[LibOS] Rewrite string-based files #2584

pwmarcz commented Jul 27, 2021 •

edited

Loading

mkow left a comment

dimakuv left a comment

pwmarcz left a comment

dimakuv left a comment

pwmarcz left a comment

dimakuv left a comment

pwmarcz left a comment

dimakuv left a comment

mkow left a comment

pwmarcz left a comment

mkow left a comment

dimakuv left a comment

pwmarcz left a comment

dimakuv left a comment

dimakuv left a comment

mkow left a comment

[LibOS] Rewrite string-based files #2584

[LibOS] Rewrite string-based files #2584

Conversation

pwmarcz commented Jul 27, 2021 • edited Loading

Description of the changes

Next steps

How to test this PR?

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

pwmarcz commented Jul 27, 2021 •

edited

Loading