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

Support domainname #1214

Merged
merged 3 commits into from
Sep 27, 2022
Merged

Support domainname #1214

merged 3 commits into from
Sep 27, 2022

Conversation

higuruchi
Copy link
Contributor

@higuruchi higuruchi commented Sep 25, 2022

Add: #1213

  • Implement

  • Add a test for it

  • config.json

    {
      "ociVersion": "1.0.2-dev",
      -- snip --
      "process": {
        -- snip --
        "args": [
          "domainname"
        ],
        -- snip --
      },
      "hostname": "youki",
      "domainname": "youki_domain",
      "annotations": {},
      -- snip --
    }
  • Execution result

     $ sudo ./youki run -b ./tutorial youki_test
     -- snip --
     youki_domain

@utam0k
Copy link
Member

utam0k commented Sep 26, 2022

@higuruchi Can I ask you to put your signature into your commits?

$ git commit --amend --no-edit -s
$ git push -f origin(or upstream?) HEAD

@utam0k
Copy link
Member

utam0k commented Sep 26, 2022

What is the status of this PR? read for review? If not, please convert to draft PR.

@utam0k utam0k self-requested a review September 26, 2022 04:33
Signed-off-by: higuruchi <fumiya2324@gmail.com>
Signed-off-by: higuruchi <fumiya2324@gmail.com>
@higuruchi higuruchi marked this pull request as ready for review September 26, 2022 09:15
Comment on lines 206 to 215
let res = unsafe { setdomainname(ptr, len) };
if res == EFAULT {
bail!("Failed to set {} as domainname. domainname pointed outside of user address space.", domainname)
}
if res == EINVAL {
bail!("Failed to set {} as domainname. domainname len was negative or too large.", domainname)
}
if res == EPERM {
bail!("Failed to set {} as domainname. the caller did not have the CAP_SYS_ADMIN capability.", domainname)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using match instead of if statements?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified to match.
And when setdomainname syscall executed errno handling is invalid so fix it.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Sep 27, 2022

Hey @higuruchi , can you fix the formatting issue that's failing the check? You will just need to run cargo fmt and push new commit / amend and force push

Signed-off-by: higuruchi <fumiya2324@gmail.com>
@utam0k
Copy link
Member

utam0k commented Sep 27, 2022

@higuruchi Thanks for your contribution!

@utam0k utam0k merged commit c7503a4 into youki-dev:main Sep 27, 2022
@YJDoc2 YJDoc2 mentioned this pull request Sep 29, 2022
2 tasks
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