libcontainer: support set stdios for container

[abel-von]

currently the container can only inherit stdios from it's parent process, the one who create container with libcontainer can not set stdios to a different file. this restrict the use of the libcontainer that a process can only create one container by itself, because we can't imagine that every container this process creates share the same stdin/stdout/stderr.

[YJDoc2]

May I ask you to take a look at discussion done in #2749 , as it was opened with similar intentions.

[abel-von]

@YJDoc2 Sorry, I also noticed that there is already a related PR after I submit this PR, but it seems it is a breaking change there, but in this PR, we just add three with_std* functions in the builder, it seems no breaking change here.

[jprendes]

This is great, thanks @abel-von.
We are currently doing the same dup2 trick in runwasi here (although we try to keep windows support there) and it works great so far.
I do prefer this over #2749 as it has a more limited scope.

[jprendes]

I think it would be better for this to be OwnedFd.
With the current design as used in the doc example, this is leaking FDs.

[abel-von]

I also hesitated between OwnedFd and RawFd, At last I think maybe RawFd is better because with RawFd the caller has its control over when to close them, otherwise the fd is closed when the it go out of some scope, maybe it is not our expected.

[jprendes]

With RawFd there's a risk the caller closes the file before we use it.
If they dont want their file closed, and want to retain ownership, the caller can always call try_clone() to obtain a new OwnedFd.

[abel-von]

I think you are right, OwnedFd is good, Now The stdin/stdout/stderr in ContainerBuilder is OwnedFd, but in ContainerArgs it is still RawFd because it should implement Clone but OwnedFd does not implement Clone. So I changed it to RawFd with as_raw_fd() when new ContainerArgs, I'm not sure if that is appropriate, please take a look. Thanks

[YJDoc2]

Hey @abel-von , before checking further, couple of comments -

1. CI is failing, there are some typos and some doc comment code is not running properly , can you take a look and fix?
2. Would it be possible to mark the builder method for setting these fds as unsafe ? Seeing your reasoning for using RawFds, maybe it might be better to set these methods to unsafe if possible, and make sure user knows what they're doing, would that make sense?

[abel-von]

Hi @YJDoc2, The CI failure is fixed, could you please trigger the CI again. Thank you.

[YJDoc2]

Hey, I think the tests are passing, but the lint and fmt CI is failing again. We use the just commands in CI to validate, so you can also run them locally and check before pushing.

Also I'm wondering, would it be possible to have e2e or unit tests for this feature?

[abel-von]

Hi @YJDoc2, just lint and just test-all has been run and codes are force pushed again, CI must be ok now. I tried to add a e2e test but it seems there is no e2e tests for libcontainer only, I can only add a unit test in builder.rs to make sure the with_stdin/with_stdout/with_stderr is correctly set.

[YJDoc2]

I tried to add a e2e test but it seems there is no e2e tests for libcontainer only, I can only add a unit test in builder.rs to make sure the with_stdin/with_stdout/with_stderr is correctly set.

Ok, I'll think on this. Btw, I have run the CI again, but there are conflicts with main branch due to another PR that was merged. Can you take a look and resolve them? Thanks.

[abel-von]

@YJDoc2 confliction is resolved. some of my just command can not succeed because my own environment. please check again. Thanks