Skip to content
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

[LibOS] chroot fs: remove redundant arg in chroot_temp_open() #1816

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

dimakuv
Copy link

@dimakuv dimakuv commented Mar 14, 2024

Description of the changes

As in title. Tiny change.

Extracted from #1812.

How to test this PR?

N/A.


This change is Reviewable

@dimakuv dimakuv marked this pull request as ready for review March 14, 2024 13:31
Copy link
Contributor

@kailun-qin kailun-qin left a 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 r1, 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) (waiting on @dimakuv)

a discussion (no related file):
What about

static int shm_do_open(struct libos_handle* hdl, struct libos_dentry* dent, mode_t type,
?


Copy link
Author

@dimakuv dimakuv left a 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, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin)

a discussion (no related file):

Previously, kailun-qin (Kailun Qin) wrote…

What about

static int shm_do_open(struct libos_handle* hdl, struct libos_dentry* dent, mode_t type,
?

That's different. In the _do_open() functions we can't remove the type argument.

Please note that we have a similar function here, chroot_do_open(), where I also do not remove the type argument.


Copy link
Author

@dimakuv dimakuv left a 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, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

That's different. In the _do_open() functions we can't remove the type argument.

Please note that we have a similar function here, chroot_do_open(), where I also do not remove the type argument.

An additional explanation that may help understand the difference:

  • chroot_temp_open() is used to open a definitely-existing file, just to get a handle on this file. This is needed because many PAL functions operate only on opened handles, not on filenames.
  • chroot_do_open() is used to open or create a file, getting the handle on this file as a by-effect. Since the file may not exist (thus there is no inode to extract the info from), we need to know the type explicitly.

shm_do_open() is related to the second kind of helper functions. So not applicable to this PR.


Copy link
Contributor

@kailun-qin kailun-qin left a 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, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: Intel)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

An additional explanation that may help understand the difference:

  • chroot_temp_open() is used to open a definitely-existing file, just to get a handle on this file. This is needed because many PAL functions operate only on opened handles, not on filenames.
  • chroot_do_open() is used to open or create a file, getting the handle on this file as a by-effect. Since the file may not exist (thus there is no inode to extract the info from), we need to know the type explicitly.

shm_do_open() is related to the second kind of helper functions. So not applicable to this PR.

Indeed -- I misread it. Thanks for the great explanation!


Copy link
Member

@mkow mkow left a 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 r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mkow mkow force-pushed the dimakuv/chroot-temp-open-no-type branch from 869b694 to 1d9d9b3 Compare April 12, 2024 13:07
Copy link
Member

@mkow mkow left a 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: :shipit: complete! all files reviewed, all discussions resolved

@dimakuv
Copy link
Author

dimakuv commented Apr 12, 2024

Jenkins, retest this please (unrelated Jenkins maintenance issues)

Copy link
Contributor

@kailun-qin kailun-qin left a 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: :shipit: complete! all files reviewed, all discussions resolved

@dimakuv
Copy link
Author

dimakuv commented Apr 12, 2024

Jenkins, retest this please (unrelated Jenkins maintenance issues)

@dimakuv dimakuv merged commit 1d9d9b3 into master Apr 12, 2024
18 of 20 checks passed
@dimakuv dimakuv deleted the dimakuv/chroot-temp-open-no-type branch April 12, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants