-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Docs] Fix typos and improve wording #1348
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 7 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: Intel)
Documentation/sgx-intro.rst
line 13 at r1 (raw file):
off the CPU package (e.g., cold-boot attacks on RAM). Gramine is able to run unmodified applications inside SGX enclaves, without the toll of manually porting the application to the SGX environment.
I'm not sure we need all this text... Maybe only the first sentence? But this is how the tech writer wanted it.
Documentation/pal/host-abi.rst
line 337 at r1 (raw file):
The ABI includes calls to get wall clock time, generate cryptographically-strong random bits, to obtain an attestation report and quote, etc.
FYI: This file is ridiculously outdated...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 7 files at r1, 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: Intel) (waiting on @dimakuv)
Documentation/devel/building.rst
line 364 at r1 (raw file):
can skip this step. For older kernels it is available as `separate patches <https://github.com/oscarlab/graphene-sgx-driver/tree/master/fsgsbase_patches>`__.
What's the reason of this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 6 of 7 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)
Documentation/devel/building.rst
line 364 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
What's the reason of this change?
Done. I don't know why the original authors did this. But I agree, I see no reason to have it as a separate paragraph. So reverted the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, all commit messages.
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: Intel), "fixup! " found in commit messages' one-liners
ed09f8d
to
52d317f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
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: Intel)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)
Documentation/manifest-syntax.rst
line 97 at r3 (raw file):
default, or ``fs.start_dir`` if specified). The recommended usage is to provide an absolute path and mount the executable
The "and" here is weird. I think we mean that the executable must also be mounted.
Proposed edit:
"...absolute path. This executable must also be in the library OS's file system, either directly mounted or within a directory that is mounted. For example, if one wishes to execute the python 3.8 interpreter, one would specify this as the entrypoint and mount the python executable at the expected path within the libOS file system."
Documentation/manifest-syntax.rst
line 697 at r3 (raw file):
This syntax specifies the files to be cryptographically hashed at build time and allowed to be accessed by the app in runtime only if their hashes match.
I suggest we rework the second half of this sentence:
"...build time; at runtime, these files may only be accessed by the app if the files' hashes match what is stored in the manifest."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 7 files at r1, 1 of 1 files at r2.
Reviewable status: 6 of 7 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)
Documentation/quickstart.rst
line 96 at r3 (raw file):
git clone --depth 1 |stable-checkout| \https://github.com/gramineproject/gramine.git Don't build Gramine as it is already installed on the system. Instead,
I would put a comma after Gramine, although it is not strictly required. Maybe change "as" to "because".
Documentation/devel/debugging.rst
line 57 at r3 (raw file):
Building Gramine with ``--buildtype=debug`` enables debug symbols and GDB integration but disables optimizations. This is usually the right thing to do:
i would recommend keeping the comma for clarity in parsing.
Documentation/pal/host-abi.rst
line 77 at r3 (raw file):
All PALs in Gramine expose a structure that provides static immutable information about the current process and its host. The address of the structure can be retrieved via :func:`PalGetPalPublicState()` and can be memorized in a
memorized -> memoized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 7 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @donporter and @kailun-qin)
Documentation/manifest-syntax.rst
line 97 at r3 (raw file):
Previously, donporter (Don Porter) wrote…
The "and" here is weird. I think we mean that the executable must also be mounted.
Proposed edit:
"...absolute path. This executable must also be in the library OS's file system, either directly mounted or within a directory that is mounted. For example, if one wishes to execute the python 3.8 interpreter, one would specify this as the entrypoint and mount the python executable at the expected path within the libOS file system."
Done. With slight modifications.
Documentation/manifest-syntax.rst
line 697 at r3 (raw file):
Previously, donporter (Don Porter) wrote…
I suggest we rework the second half of this sentence:
"...build time; at runtime, these files may only be accessed by the app if the files' hashes match what is stored in the manifest."
Done.
Documentation/quickstart.rst
line 96 at r3 (raw file):
Previously, donporter (Don Porter) wrote…
I would put a comma after Gramine, although it is not strictly required. Maybe change "as" to "because".
Done.
Documentation/pal/host-abi.rst
line 77 at r3 (raw file):
Previously, donporter (Don Porter) wrote…
memorized -> memoized
Done.
Documentation/devel/debugging.rst
line 57 at r3 (raw file):
Previously, donporter (Don Porter) wrote…
i would recommend keeping the comma for clarity in parsing.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 7 files at r1, 1 of 1 files at r2, 1 of 1 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @donporter)
Documentation/manifest-syntax.rst
line 98 at r5 (raw file):
The recommended usage is to provide an absolute path of the executable. This executable must also be in the Gramine's file system, either directly mounted or
ditto for the whole PR
Suggestion:
filesystem
Documentation/manifest-syntax.rst
line 138 at r5 (raw file):
containing output of :ref:`gramine-argv-serializer<gramine-argv-serializer>`. If none of the above argument-handling manifest options are specified in the
Is this change correct? You can only specify one of these options, not multiple of them.
Documentation/sgx-intro.rst
line 13 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I'm not sure we need all this text... Maybe only the first sentence? But this is how the tech writer wanted it.
I don't understand this whole change here. This documentation page is an introduction to SGX, not to Gramine-on-SGX.
Also, half of the new text is basically "what is Gramine".
I'd revert this whole hunk.
Documentation/devel/building.rst
line 22 at r5 (raw file):
higher. If you find problems with Gramine on other Linux distributions, contact us with a |~| detailed `bug report <https://github.com/gramineproject/gramine/issues/new>`__.
Just noticed: The link should actually be https://github.com/gramineproject/gramine/issues/new/choose
(the template picker).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 10 files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @donporter, @kailun-qin, and @mkow)
Documentation/manifest-syntax.rst
line 98 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto for the whole PR
Done (for our whole documentation). But why do you prefer this way of writing?
Documentation/manifest-syntax.rst
line 138 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Is this change correct? You can only specify one of these options, not multiple of them.
Done. This was suggested by Don (iirc), who is a native speaker. But I would be ok with any style here, I think both native and non-native speakers will understand the general idea of this sentence.
Documentation/sgx-intro.rst
line 13 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I don't understand this whole change here. This documentation page is an introduction to SGX, not to Gramine-on-SGX.
Also, half of the new text is basically "what is Gramine".
I'd revert this whole hunk.
Done. Kept only the introductory name of Intel SGX as the change.
Documentation/devel/building.rst
line 22 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Just noticed: The link should actually be
https://github.com/gramineproject/gramine/issues/new/choose
(the template picker).
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r6, 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @donporter)
Documentation/manifest-syntax.rst
line 98 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done (for our whole documentation). But why do you prefer this way of writing?
Seems more natural to me, but this request is mostly because we use "filesystem" almost everywhere else in Gramine codebase.
Documentation/manifest-syntax.rst
line 138 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done. This was suggested by Don (iirc), who is a native speaker. But I would be ok with any style here, I think both native and non-native speakers will understand the general idea of this sentence.
I see. Resolving, but out of curiosity would like to see what @donporter thinks and what was the exact reason for this change (is only one of these forms formally correct? or maybe both, but they have slightly different meanings?)
Documentation/sgx-intro.rst
line 7 at r6 (raw file):
The Gramine project uses the :term:`Intel SGX <SGX>` (Software Guard Extensions) technology to securely run software. SGX is a |~| complicated topic, which may
I really dislike such statements, because security is always relative to some attacker model. The freestanding "running software securely" is meaningless here (e.g. Gramine nor SGX doesn't make apps harder to exploit remotely, it actually makes that easier).
I'd rather say "to protect software running on untrusted hosts".
Code quote:
to securely run software.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 9 of 10 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @donporter and @mkow)
Documentation/sgx-intro.rst
line 7 at r6 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I really dislike such statements, because security is always relative to some attacker model. The freestanding "running software securely" is meaningless here (e.g. Gramine nor SGX doesn't make apps harder to exploit remotely, it actually makes that easier).
I'd rather say "to protect software running on untrusted hosts".
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @donporter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r5, 5 of 6 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @donporter)
Co-authored-by: Deb Taylor <deb.taylor@intel.com> Co-authored-by: Paul Cartee <paulx.alan.cartee@intel.com> Co-authored-by: Benny Fuhry <benny.fuhry@intel.com> Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com> Signed-off-by: Deb Taylor <deb.taylor@intel.com> Signed-off-by: Paul Cartee <paulx.alan.cartee@intel.com> Signed-off-by: Benny Fuhry <benny.fuhry@intel.com>
792252e
to
ad39399
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @donporter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @donporter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @donporter)
a discussion (no related file):
A quick ping for @donporter, since he's the only one blocking on this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r5, 5 of 6 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion
Documentation/manifest-syntax.rst
line 138 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I see. Resolving, but out of curiosity would like to see what @donporter thinks and what was the exact reason for this change (is only one of these forms formally correct? or maybe both, but they have slightly different meanings?)
Is none singular or plural? There are more than one options, but only one can be specified. Probably either is correct, but I might reword. Like "If the manifest does not specify an argument-handling option (from the choices above), the application will get the default..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved
Documentation/manifest-syntax.rst
line 138 at r5 (raw file):
Previously, donporter (Don Porter) wrote…
Is none singular or plural? There are more than one options, but only one can be specified. Probably either is correct, but I might reword. Like "If the manifest does not specify an argument-handling option (from the choices above), the application will get the default..."
Thanks for the input. Since this PR is ready for merge, and you approved (i.e., the proposed change is not blocking), I will merge the PR now, and will add this fix to some other PR.
Description of the changes
Extracted from #1241.
How to test this PR?
Compare:
This change is