[Pal/Linux-SGX] Fix renaming issue with protected files #2681

svenkata9 · 2021-09-07T08:10:37Z

Description of the changes

With certain workloads involving Federated Learning, the frameworks create protected files and rename them as part of the flow. This does not work currently with protected files because every protected file maintains the path name in its metadata (stored when the file was first created) and which is checked during open to make sure the incoming path is the same as what is already stored in the metadata for this file.

This PR is to address this issue by handling rename for protected files. A normal rename call does not need the file to be open in RW mode, but since the metadata needs to be updated with the new path, we pass a special PAL_OPTION_RENAME flag to DkStreamOpen during rename and open the protected file in RW mode with required permissions, update the metadata with the new path and flush it. Then the flow goes through the one for normal file renaming. If the ocall to rename fails, we restore the metadata back to what it was earlier so the file remains usable still.

There is a new regression test in LibOS pf_rename to test this

How to test this PR?

The newly added regression test in LibOS pf_rename should pass.

Signed-off-by: Sankaranarayanan Venkatasubramanian [email protected]

Description of the changes

How to test this PR?

This change is

With certain workloads involving Federated Learning, the frameworks create protected files and renames them as part of the flow. This does not work currently with protected files because every protected file maintains the path name in its metadata (stored when the file was first created) and which is checked during open to make sure the incoming path is the same as what is already stored in the metadata for this file. A normal rename call does not need the file to be open in RW mode, but since the metadata needs to be updated with the new path, we pass a special PAL_OPTION_RENAME flag to DkStreamOpen during rename and open the protected file in RW mode with required permissions, update the metadata with the new path and flush it. Then the flow goes through the one for normal file renaming. If the ocall to rename fails, we restore the metadata back to what it was earlier so the file remains usable still. Signed-off-by: Sankaranarayanan Venkatasubramanian <[email protected]>

svenkata9 · 2021-09-07T09:32:35Z

@duanbing I abandoned PR 2521 because of conflicts after rebasing (it was ~2 months old). Please use this one.

duanbing · 2021-09-08T03:18:36Z

Pal/src/host/Linux-SGX/db_files.c

+        if (PF_FAILURE(pf_ret)) {
+            log_warning("rename failed: %s, the file might be unusable.", pf_strerror(pf_ret));
+            return -PAL_ERROR_DENIED;
+        }
        return unix_to_pal_error(ret);
    }


Need free(tmp); after renaming?

I didn't get your comment exactly. But, tmp needs to be freed up anyway before we return from this function. And, this rename is restoring the metadata back to what it was before. And, tmp is no more of use when we enter this if (ret < 0) condition.

@svenkata9 I mean heap variable tmp is not freed when renaming returns >= 0.

@duanbing It is assigned in this line, so cannot be freed.

graphene/Pal/src/host/Linux-SGX/db_files.c

Line 806 in 33a68bc

handle->file.realpath = tmp;

Got it. Thanks.

dimakuv

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

a discussion (no related file):
@pwmarcz will need to review this. Especially the part with the new rename test -- Pawel also added a similar test in his PR, so I think the two tests can be merged into one.

-- commits, line 5 at r1:
renames -> rename

-- commits, line 15 at r1:
This doesn't seem to wrap on 72-char limit (the default limit for git messages). Could you re-wrap lines to 72 chars?

LibOS/shim/test/regression/.gitignore, line 77 at r1 (raw file):

/openmp
/pf_rename
/pftmp

Please move /pftmp (and also /tmp) in a separate section in this file (i.e., separates from other files by empty lines). These two entries are directories for temporary files, not executables, so they don't belong in this list logically.

LibOS/shim/test/regression/pf_rename.c, line 14 at r1 (raw file):

    char buf[15];
    char input_text[] = "Hello, world!";
    int fd = open("pftmp/foo.txt", O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);

Can you use macros for these two paths? Like this:

#define PF_FOO_PATH "pftmp/foo.txt"
#define PF_BAR_PATH "pftmp/bar.txt"

LibOS/shim/test/regression/pf_rename.c, line 16 at r1 (raw file):

    int fd = open("pftmp/foo.txt", O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
    if (fd < 0)
        err(1, "Cannot create file pftmp/foo.txt.");

Please remove the trailing dots (.) from err() messages -- err() function automatically adds the necessary chars.

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

        err(1, "Cannot create file pftmp/foo.txt.");

    int num_bytes = write(fd, input_text, strlen(input_text) + 1);

Please wrap the write() call in a while loop (to continue on partial writes). See example here: https://github.com/gramineproject/graphene/pull/2646/files#diff-7dcf98136ae60e9b2d915bf6e8548e9a3251dea52bbe975633cbc0ebbbeb2945

LibOS/shim/test/regression/pf_rename.c, line 24 at r1 (raw file):

    }

    close(fd);

Please add error checking for close() functions too.

LibOS/shim/test/regression/pf_rename.c, line 28 at r1 (raw file):

    int ret = rename("pftmp/foo.txt", "pftmp/bar.txt");
    if (ret < 0) {
        fd = open("pftmp/foo.txt", O_RDONLY);

To be honest, I don't see a point in this logic of "rename failed, let's verify that the original file is correct". You will never hit this logic in this test, because you don't explicitly make rename() above to fail (like, why would it fail?). So all this code is dead code. Let's just remove it (or you figure out a way to make rename() above fail).

LibOS/shim/test/regression/pf_rename.c, line 30 at r1 (raw file):

        fd = open("pftmp/foo.txt", O_RDONLY);
        if (fd < 0)
            err(1, "Rename failed, Original file corrupted & unusable.");

Original -> original, here and below

LibOS/shim/test/regression/pf_rename.c, line 32 at r1 (raw file):

            err(1, "Rename failed, Original file corrupted & unusable.");

        num_bytes = read(fd, buf, sizeof(buf));

Please wrap the read() call in a while loop (to continue on partial reads). See example here: https://github.com/gramineproject/graphene/pull/2646/files#diff-7dcf98136ae60e9b2d915bf6e8548e9a3251dea52bbe975633cbc0ebbbeb2945

LibOS/shim/test/regression/test_libos.py, line 840 at r1 (raw file):

                os.remove("pftmp/bar.txt")
        else:
            os.mkdir("pftmp")

You can simplify it like this (didn't actually run, may contain small bugs):

for path in ['pftmp/foo.txt', 'pftmp/bar.txt']:
    if os.path.exists(path):
        os.remove(path)
os.makedirs('pftmp/', exist_ok=True)

Pal/src/host/Linux-SGX/db_files.c, line 128 at r1 (raw file):

         * the metadata in the file, so open with RDWR mode with necessary share permissions.
         * For normal files, reset this option.
         */

The comment seems stale.

If it is a protected file and -- you can just remove this part. It is obvious here that we work on the PF.

For normal files, reset this option. -- stale comment, we don't do anything in this code snippet.

Pal/src/host/Linux-SGX/db_files.c, line 797 at r1 (raw file):

Why this newline? Remove it.

Pal/src/host/Linux-SGX/db_files.c, line 811 at r1 (raw file):

    /* TODO: Handle the case of renaming a file that has a file handle already open, so that the
     * file operations work on both the handles properly
     */

What is the scenario for this TODO, and why it doesn't work currently? I don't remember this...

Pal/src/host/Linux-SGX/db_files.c, line 850 at r1 (raw file):

    /* If the rename ocall fails, restore the metadata back to current file path, so that the
     * file does not become unusable.
     */

This comment is not needed because you already have another comment about it below. Please just remove this comment.

Pal/src/host/Linux-SGX/db_files.c, line 854 at r1 (raw file):

        free(tmp);
        /* restore the original file name in pf metadata */
        pf_status_t pf_ret =  pf_rename(pf->context, handle->file.realpath);

Remove double whitespace.

Pal/src/host/Linux-SGX/db_files.c, line 856 at r1 (raw file):

        pf_status_t pf_ret =  pf_rename(pf->context, handle->file.realpath);
        if (PF_FAILURE(pf_ret)) {
            log_warning("rename failed: %s, the file might be unusable.", pf_strerror(pf_ret));

Please start with capital letter (Rename...) and remove the final dot -- just to be uniform with the other log message.

Pal/src/host/Linux-SGX/db_files.c, line 857 at r1 (raw file):

        if (PF_FAILURE(pf_ret)) {
            log_warning("rename failed: %s, the file might be unusable.", pf_strerror(pf_ret));
            return -PAL_ERROR_DENIED;

Please remove this return. We will already return with the error code in the next line, so no need for this one.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 376 at r1 (raw file):

static bool ipf_rename_file(pf_context_t* pf, const char* new_path) {
    if (!new_path) {

Actually, can we move this check to pf_rename()? Sounds more logical to check it there.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1347 at r1 (raw file):

    if (new_path && strlen(new_path) > PATH_MAX_SIZE - 1) {
        pf->last_error = PF_STATUS_PATH_TOO_LONG;

You don't need to set it here. Just remove this line. pf->last_error is a way to propagate the error to the upper levels of the PF implementation, but here you are already at the upmost level.

With certain workloads involving Federated Learning, the frameworks create protected files and rename them as part of the flow. This does not work currently with protected files because every protected file maintains the path name in its metadata (stored when the file was first created) and which is checked during open to make sure the incoming path is the same as what is already stored in the metadata for this file. A normal rename call does not need the file to be open in RW mode, but since the metadata needs to be updated with the new path, we pass a special PAL_OPTION_RENAME flag to DkStreamOpen during rename and open the protected file in RW mode with required permissions, update the metadata with the new path and flush it. Then the flow goes through the one for normal file renaming. If the ocall to rename fails, we restore the metadata back to what it was earlier so the file remains usable. Signed-off-by: Sankaranarayanan Venkatasubramanian <[email protected]>

svenkata9

Reviewable status: 6 of 11 files reviewed, 22 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 and @svenkata9)

-- commits, line 5 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

renames -> rename

Done.

-- commits, line 15 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This doesn't seem to wrap on 72-char limit (the default limit for git messages). Could you re-wrap lines to 72 chars?

Done.

LibOS/shim/test/regression/.gitignore, line 77 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please move /pftmp (and also /tmp) in a separate section in this file (i.e., separates from other files by empty lines). These two entries are directories for temporary files, not executables, so they don't belong in this list logically.

Done.

LibOS/shim/test/regression/pf_rename.c, line 14 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can you use macros for these two paths? Like this:
#define PF_FOO_PATH "pftmp/foo.txt"
#define PF_BAR_PATH "pftmp/bar.txt"

Done.

LibOS/shim/test/regression/pf_rename.c, line 16 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please remove the trailing dots (.) from err() messages -- err() function automatically adds the necessary chars.

Done.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please wrap the write() call in a while loop (to continue on partial writes). See example here: https://github.com/gramineproject/graphene/pull/2646/files#diff-7dcf98136ae60e9b2d915bf6e8548e9a3251dea52bbe975633cbc0ebbbeb2945

Done.

LibOS/shim/test/regression/pf_rename.c, line 24 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please add error checking for close() functions too.

Done.

LibOS/shim/test/regression/pf_rename.c, line 28 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

To be honest, I don't see a point in this logic of "rename failed, let's verify that the original file is correct". You will never hit this logic in this test, because you don't explicitly make rename() above to fail (like, why would it fail?). So all this code is dead code. Let's just remove it (or you figure out a way to make rename() above fail).

Done.

LibOS/shim/test/regression/pf_rename.c, line 30 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Original -> original, here and below

Removed the code. So, this is NA

LibOS/shim/test/regression/pf_rename.c, line 32 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please wrap the read() call in a while loop (to continue on partial reads). See example here: https://github.com/gramineproject/graphene/pull/2646/files#diff-7dcf98136ae60e9b2d915bf6e8548e9a3251dea52bbe975633cbc0ebbbeb2945

Done.

LibOS/shim/test/regression/test_libos.py, line 840 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You can simplify it like this (didn't actually run, may contain small bugs):
for path in ['pftmp/foo.txt', 'pftmp/bar.txt']:
    if os.path.exists(path):
        os.remove(path)
os.makedirs('pftmp/', exist_ok=True)

Done.

Pal/src/host/Linux-SGX/db_files.c, line 128 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

The comment seems stale.

If it is a protected file and -- you can just remove this part. It is obvious here that we work on the PF.

For normal files, reset this option. -- stale comment, we don't do anything in this code snippet.

Done.

Pal/src/host/Linux-SGX/db_files.c, line 797 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why this newline? Remove it.

Done.

Pal/src/host/Linux-SGX/db_files.c, line 811 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What is the scenario for this TODO, and why it doesn't work currently? I don't remember this...

https://reviewable.io/reviews/gramineproject/graphene/2521#-Mi6_Ynm5qI5oJuEvpt8

Pal/src/host/Linux-SGX/db_files.c, line 850 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This comment is not needed because you already have another comment about it below. Please just remove this comment.

Done.

Pal/src/host/Linux-SGX/db_files.c, line 856 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please start with capital letter (Rename...) and remove the final dot -- just to be uniform with the other log message.

Done.

Pal/src/host/Linux-SGX/db_files.c, line 857 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please remove this return. We will already return with the error code in the next line, so no need for this one.

Done.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 376 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Actually, can we move this check to pf_rename()? Sounds more logical to check it there.

Done.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1347 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You don't need to set it here. Just remove this line. pf->last_error is a way to propagate the error to the upper levels of the PF implementation, but here you are already at the upmost level.

Done.

dimakuv

Reviewed 5 of 5 files at r2, 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 @svenkata9)

-- commits, line 5 at r1:

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Done.

That's not what I meant. I was expecting you to git commit --amend your first commit, instead you have two commits now: the first one with the old text, and the second one (which should have been a fixup commit) with the new text.

Let's keep it as-is, I'll fix it during final rebase. But for the future, changing commit messages is only meamingful using the git rebase or git commit --amend flows.

LibOS/shim/test/regression/pf_rename.c, line 23 at r2 (raw file):

        err(1, "File creation failed");

    int size = strlen(input_text) + 1;

int -> size_t

LibOS/shim/test/regression/pf_rename.c, line 57 at r2 (raw file):

        if (n == 0) {
            if (size > 0) {
                warnx("read less bytes than expected");

read -> Read

LibOS/shim/test/regression/pf_rename.c, line 65 at r2 (raw file):

    } while (pos < BUF_SIZE);

    close(fd);

You forgot ret = close(fd)

LibOS/shim/test/regression/pf_rename.c, line 76 at r2 (raw file):

    printf("TEST OK\n");

Remove this empty line.

LibOS/shim/test/regression/pf_rename.c, line 79 at r2 (raw file):

    return 0;
}

Remove this empty line.

Pal/src/host/Linux-SGX/db_files.c, line 811 at r1 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

https://reviewable.io/reviews/gramineproject/graphene/2521#-Mi6_Ynm5qI5oJuEvpt8

OK. Thanks.

Pal/src/host/Linux-SGX/db_files.c, line 856 at r1 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Done.

Forgot to remove the final dot :)

Pal/src/host/Linux-SGX/db_files.c, line 127 at r2 (raw file):

        /* The file is being opened for renaming. We will need to update the metadata in the file,
         * so open with RDWR mode with necessary share permissions.
         */

Please move this */ to the end of the previous line (personal preference, but we do it almost everywhere in our codebase)

Pal/src/host/Linux-SGX/db_files.c, line 809 at r2 (raw file):

    /* TODO: Handle the case of renaming a file that has a file handle already open, so that the
     * file operations work on both the handles properly
     */

Please move this */ to the end of the previous line (personal preference, but we do it almost everywhere in our codebase)

pwmarcz

Reviewable status: all files reviewed, 12 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 and @svenkata9)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@pwmarcz will need to review this. Especially the part with the new rename test -- Pawel also added a similar test in his PR, so I think the two tests can be merged into one.

I hoped I would be able to point to the new repo, but we have some technical issues with Reviewable and could not merge the new branch yet...

Anyway, the PR (gramineproject/gramine#5) already contains a test suite for rename; it's skipped for protected files but you can easily un-skip it if you want to check your code.

LibOS/shim/test/regression/pf_rename.c, line 1 at r2 (raw file):

/* Protected file renaming. Renaming a file without closing its handle. */

This comment says that the test is "Renaming a file without closing its handle.", but the test is actually closing the handle and then renaming the file. Why? I think renaming a file without closing its handle is actually the right thing to test here.

Anyway, as mentioned elsewhere, you can try using the test from gramineproject/gramine#5 (hopefully soon to be merged). The chroot/fs.c file has been rewritten, but since this PR changes only one line in it, the change should not be hard to adapt.

svenkata9

Reviewable status: 9 of 11 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @pwmarcz, and @svenkata9)

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I hoped I would be able to point to the new repo, but we have some technical issues with Reviewable and could not merge the new branch yet...

Anyway, the PR (gramineproject/gramine#5) already contains a test suite for rename; it's skipped for protected files but you can easily un-skip it if you want to check your code.

Thanks. I can work on this after this PR gets merged

-- commits, line 5 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

That's not what I meant. I was expecting you to git commit --amend your first commit, instead you have two commits now: the first one with the old text, and the second one (which should have been a fixup commit) with the new text.

Let's keep it as-is, I'll fix it during final rebase. But for the future, changing commit messages is only meamingful using the git rebase or git commit --amend flows.

I see. I misunderstood the sequence.

LibOS/shim/test/regression/pf_rename.c, line 1 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

This comment says that the test is "Renaming a file without closing its handle.", but the test is actually closing the handle and then renaming the file. Why? I think renaming a file without closing its handle is actually the right thing to test here.

Anyway, as mentioned elsewhere, you can try using the test from gramineproject/gramine#5 (hopefully soon to be merged). The chroot/fs.c file has been rewritten, but since this PR changes only one line in it, the change should not be hard to adapt.

Thanks. I will update the code once your PR is merged. For now, I will keep it like this.

Like I indicated, renaming a protected file without closing the handle needs to be worked out in a separate PR. There are few problems that need to be ironed out. (1) the g_protected_files uses a single key UT, can can return a wrong output when find_protected_file(handle) is used. Further, if the file is opened in write mode, it will fail with the latest code because we allow only one write fd, and rename opens it in RDWR mode.

For the current issue, let us keep it like this to unblock the customer who is waiting for this change.

LibOS/shim/test/regression/pf_rename.c, line 23 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

int -> size_t

Done.

LibOS/shim/test/regression/pf_rename.c, line 57 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

read -> Read

Done. But, I just reused from the reference you pointed out. It is all using lower case error message starts. https://github.com/gramineproject/graphene/pull/2646/files#

LibOS/shim/test/regression/pf_rename.c, line 65 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You forgot ret = close(fd)

Done.

LibOS/shim/test/regression/pf_rename.c, line 76 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Remove this empty line.

Done.

LibOS/shim/test/regression/pf_rename.c, line 79 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Remove this empty line.

Done, but I follow leaving a new line at the end (I guess C11 still has that mention, though the compilers work even when we don't leave a new line at the end).

Pal/src/host/Linux-SGX/db_files.c, line 856 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Forgot to remove the final dot :)

Done.

Pal/src/host/Linux-SGX/db_files.c, line 127 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please move this */ to the end of the previous line (personal preference, but we do it almost everywhere in our codebase)

Done. But, appearance-wise, having it in the next line looks good. Anyway, I'll follow what the majority follow.

Pal/src/host/Linux-SGX/db_files.c, line 809 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please move this */ to the end of the previous line (personal preference, but we do it almost everywhere in our codebase)

Done. But, appearance-wise, having it in the next line looks good. Anyway, I'll follow what the majority follow.

svenkata9

Reviewable status: 9 of 11 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @duanbing, and @pwmarcz)

Pal/src/host/Linux-SGX/db_files.c, line 860 at r1 (raw file):

Previously, duanbing (Bing) wrote…

Got it. Thanks.

Thanks

dimakuv

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @duanbing, @pwmarcz, and @svenkata9)

a discussion (no related file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Thanks. I can work on this after this PR gets merged

gramineproject/gramine#5 -- the PR was merged.

I suggest to now rebase your PR locally, check that the tests are working for your cases, and if everything works as expected, push your rebase to GitHub so everyone can see it. Maybe it will even make more sense to recreate the PR in the new repository now.

LibOS/shim/test/regression/pf_rename.c, line 57 at r2 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Done. But, I just reused from the reference you pointed out. It is all using lower case error message starts. https://github.com/gramineproject/graphene/pull/2646/files#

OK. I understand your point, but generally we don't have a strong preference on lower-case vs upper-case. It's just a matter of uniformity with the surrounding code. You have upper-case messages in all other places in this file, so we should be uniform and also have upper-case here.

mkow · 2021-10-18T12:01:28Z

This was superseded by gramineproject/gramine#31, closing.

duanbing reviewed Sep 8, 2021

View reviewed changes

dimakuv suggested changes Sep 8, 2021

View reviewed changes

svenkata9 commented Sep 9, 2021

View reviewed changes

dimakuv suggested changes Sep 10, 2021

View reviewed changes

pwmarcz suggested changes Sep 10, 2021

View reviewed changes

fixup! [Pal/Linux-SGX] Fix renaming issue with protected files

4957b99

svenkata9 commented Sep 13, 2021

View reviewed changes

dimakuv suggested changes Sep 13, 2021

View reviewed changes

mkow closed this Oct 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pal/Linux-SGX] Fix renaming issue with protected files #2681

[Pal/Linux-SGX] Fix renaming issue with protected files #2681

svenkata9 commented Sep 7, 2021 •

edited

Loading

svenkata9 commented Sep 7, 2021 •

edited

Loading

duanbing Sep 8, 2021

svenkata9 Sep 8, 2021

duanbing Sep 8, 2021

svenkata9 Sep 8, 2021

duanbing Sep 8, 2021

dimakuv left a comment

svenkata9 left a comment

dimakuv left a comment

pwmarcz left a comment

svenkata9 left a comment

svenkata9 left a comment

dimakuv left a comment

mkow commented Oct 18, 2021

[Pal/Linux-SGX] Fix renaming issue with protected files #2681

[Pal/Linux-SGX] Fix renaming issue with protected files #2681

Conversation

svenkata9 commented Sep 7, 2021 • edited Loading

Description of the changes

How to test this PR?

Description of the changes

How to test this PR?

svenkata9 commented Sep 7, 2021 • edited Loading

duanbing Sep 8, 2021

Choose a reason for hiding this comment

svenkata9 Sep 8, 2021

Choose a reason for hiding this comment

duanbing Sep 8, 2021

Choose a reason for hiding this comment

svenkata9 Sep 8, 2021

Choose a reason for hiding this comment

duanbing Sep 8, 2021

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

svenkata9 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

svenkata9 left a comment

Choose a reason for hiding this comment

svenkata9 left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow commented Oct 18, 2021

svenkata9 commented Sep 7, 2021 •

edited

Loading

svenkata9 commented Sep 7, 2021 •

edited

Loading