Implement containerization from scratch

[vadorovsky]

Drop the dependency on container engines like Docker or Podman by using clone syscall to implement rootless containers, oci-spec crate to read OCI image specifications, reqwest and tar-rs to download the images.

That solves the major problem we had with Docker - file ownership in bind mounted volumes - where new files created inside container in a volume with source code were owned by root and therefore inaccessible for a regular user calling icedragon.

After this change, regular container engines are still used for building the images.

Fixes #7

---

This change is 

[vadorovsky]

Of course hitting an issue reproducible only on Github Actions 😭

[ERROR libcontainer::namespaces] failed to unshare namespace err=Nix(EPERM) namespace=LinuxNamespace { typ: Pid, path: None }
[ERROR libcontainer::process::container_main_process] failed to run intermediate process EPERM: Operation not permitted
[ERROR libcontainer::container::builder_impl] failed to run container process intermediate process error EPERM: Operation not permitted
[WARN  libcgroups::common] cannot configure rootless cgroup using the cgroupfs manager

thread 'tokio-runtime-worker' panicked at src/main.rs:627:18:
called `Result::unwrap()` on an `Err` value: CreateContainerError(CreateContainerError(MainProcess(Channel(OtherError("EPERM: Operation not permitted"))), None))

Rootless podman works fine, so either:

• Podman just skips the namespace-related errors (if that's the case, we should too).
• nix does something weird when unsharing the pidns (unlikely, it looks like pure unshare(pid))

[vadorovsky]

OK, it's AppArmor.

sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0

is a sufficient workaround. Will think of something better, but I might stick to it if I'm out of options.

[Copilot]

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[tamird]

Reviewed 1 of 1 files at r2.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @copilot-pull-request-reviewer[bot])

---

Cargo.toml line 33 at r2 (raw file):

oci-client = { version = "0.14", default-features = false, features = ["rustls-tls"] }
# libcontainer fails to build with newer versions. Not a direct dependency of
# icedragon.

link to a bug?

---

Cargo.toml line 56 at r2 (raw file):

# https://github.com/youki-dev/youki/issues/3144
# https://github.com/youki-dev/youki/pull/3146
libcontainer = { git = "https://github.com/vadorovsky/youki", branch = "cgroupfs-rootless-warning" }

this should probably wait until these are resolved in some way?

[vadorovsky]

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @copilot-pull-request-reviewer[bot] and @tamird)

---

Cargo.toml line 33 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

link to a bug?

Good call, I didn't make one before:

checkpoint-restore/rust-criu#25

---

Cargo.toml line 56 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this should probably wait until these are resolved in some way?

Might take very long, given no response.

Actually, given that working with youki crates turns out to be a bit annoying, I might try going even deeper and creating a "container" by issuing bunch of unshare syscalls myself, avoiding to use these deps. I will give it a though today and if I get convinced that's not too crazy, I will do that.

To be precise, the annoying things are:

• Hard dependency on systemd.
• The protobuf issue.
• Lack of possibility to pass a Spec without saving it on disk.

[vadorovsky]

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @copilot-pull-request-reviewer[bot] and @tamird)

---

Cargo.toml line 56 at r2 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

Might take very long, given no response.

Actually, given that working with youki crates turns out to be a bit annoying, I might try going even deeper and creating a "container" by issuing bunch of unshare syscalls myself, avoiding to use these deps. I will give it a though today and if I get convinced that's not too crazy, I will do that.

To be precise, the annoying things are:

• Hard dependency on systemd.
• The protobuf issue.
• Lack of possibility to pass a Spec without saving it on disk.

I managed to get rid of youki/libcontainer and do everything with mount and unshare through nix! \o/

Going zzz now. My last push will be most likely red, I will polish it tomorrow.

[vadorovsky]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @copilot-pull-request-reviewer[bot] and @tamird)

---

Cargo.toml line 56 at r2 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

I managed to get rid of youki/libcontainer and do everything with mount and unshare through nix! \o/

Going zzz now. My last push will be most likely red, I will polish it tomorrow.

Should become green after last push. Feel free to re-review.

In the meantime, I will try to get the bpf-linker PR green

[tamird]

Reviewed 1 of 4 files at r1, 2 of 3 files at r4, 1 of 2 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 43 unresolved discussions (waiting on @vadorovsky)

---

src/main.rs line 38 at r6 (raw file):

/// Content of the dockerfile.
const DOCKERFILE: &[u8] = include_bytes!("../containers/Dockerfile");

every one of these constants is used in exactly one function. move them into those functions?

---

src/main.rs line 47 at r6 (raw file):

/// A wrapper over [`VecDeque`] making it easy to gather platform-native
/// strings.
#[derive(Clone, Debug, Default)]

is Clone used?

---

src/main.rs line 59 at r6 (raw file):

        for arg in iter {
            cmd.arg(arg);
        }

cmd.args(iter) might work?

Code quote:

        for arg in iter {
            cmd.arg(arg);
        }

---

src/main.rs line 104 at r6 (raw file):

    /// Directory where the internal state is stored.
    #[arg(global = true, long, default_value = "~/.icedragon")]
    state_dir: OsString,

why not PathBuf?

---

src/main.rs line 173 at r6 (raw file):

#[derive(Parser)]
struct BuildContainerImageArgs {
    /// Container engine (if not provided, is going to be autodetected).

s/is going to/will/

---

src/main.rs line 204 at r6 (raw file):

    /// Additional volumes to mount to the container.
    #[arg(long = "volume", short)]
    pub volumes: Vec<String>,

shouldn't this be a vec of PathBuf?

---

src/main.rs line 212 at r6 (raw file):

/// Parameters used to launch a container.
#[derive(Clone)]

Clone not used?

---

src/main.rs line 236 at r6 (raw file):

    override_cc_with_cross: bool,
    /// List of user-provided volumes to bind mount to the container.
    volumes: Vec<String>,

PathBuf?

---

src/main.rs line 258 at r6 (raw file):

        Self {
            interactive,
            state_dir: state_dir.to_path_buf(),

let the caller do this.

---

src/main.rs line 281 at r6 (raw file):

    let mut child = cmd.spawn().with_context(|| {
        format!("failed to run command {cmd:?} for pushing the container image")

s/run/spawn/ (be precise)

the word push is already going to be in {cmd:?}. I suggest not putting anything after the cmd's debug representation.

here and below.

---

src/main.rs line 296 at r6 (raw file):

            for line in BufReader::new(stdout).lines() {
                let line = line.unwrap_or_else(|e| {
                    panic!("failed to retrieve the stdout line from command {cmd:?}: {e:?}")

drop "the"

here and below

---

src/main.rs line 312 at r6 (raw file):

    let status = child
        .wait()
        .with_context(|| format!("command {cmd:?} failed to start"))?;

is failed to start accurate? i'd just say failed to wait, matching phrasing above

---

src/main.rs line 339 at r6 (raw file):

        Some(ref container_engine) => container_engine,
        None => &ContainerEngine::autodetect()?,
    };

why the ref dance?

Code quote:

    let container_engine = match container_engine {
        Some(ref container_engine) => container_engine,
        None => &ContainerEngine::autodetect()?,
    };

---

src/main.rs line 357 at r6 (raw file):

    let mut child = cmd.spawn().with_context(|| {
        format!("failed to run command {cmd:?} for bulding the container image")

ditto (nothing after {cmd:?})

---

src/main.rs line 363 at r6 (raw file):

            .stdin
            .take()
            .unwrap_or_else(|| panic!("expected command {cmd:?} to have piped stdin"));

ditto (nothing after {cmd:?})

here and 2 more below

---

src/main.rs line 399 at r6 (raw file):

    let status = child
        .wait()
        .with_context(|| format!("command {cmd:?} failed to start"))?;

ditto (failed to wait)

---

src/main.rs line 446 at r6 (raw file):

///     if the host and target CPU targets are different.
/// * If `override_cc_with_cross` is set, `CC` and `CXX` are set to point to
///   the clang cross wrappers.

isn't this going to be incredibly difficult to keep in sync with the implementation? i'd sooner break this up and sprinkle the details inside the implementation.

Code quote:

/// Returns a vector of environment variables to use in the container spec,
/// consisting of:
///
/// * The current environment variables, except ones like `HOME`, `PATH` etc.,
///   which would break the containerized environment. We filter out variables
///   prefixed by `CARGO` and `RUSTUP` to make sure that the Rust toolchain
///   inside container is isolated, expecially when icedragon is launched with
///   `cargo run`. Variables prefixed by `SSL` are filtered to make sure that
///   ca-certificates from the container are used.
/// * Additional variables defined by us to shape the behavior of compilers and
///   making sure they perform a correct cross build for the given `target`:
///   * `CARGO_BUILD_TARGET`, telling cargo what target to build for.
///   * `CXXFLAGS` and `LDFLAGS`, pointing to LLVM libc++ as a C++ stdlib, LLD
///     as a linker, compiler-rt as a runtime library and LLVM libunwind as
///     unwinder.
///   * `PATH` including the directories of LLVM and Rust toolchains.
///   * `PKG_CONFIG_SYSROOT_DIR`, to point pkg-config to the sysroot.
///   * `RUSTUP_HOME` pointing to the directory with Rust toolchains.
///   * `RUSTFLAGS` telling Rust to:
///     * Use an appropriate clang wrapper as a linker.
///     * Use the cross sysroot.
///   * `CARGO_TARGET_{triple}_RUNNER`, pointing to a QEMU user-space emulator,
///     if the host and target CPU targets are different.
/// * If `override_cc_with_cross` is set, `CC` and `CXX` are set to point to
///   the clang cross wrappers.

---

src/main.rs line 495 at r6 (raw file):

        .map(char::from)
        .collect()
}

nit: can be a lot closer to where it's used (inlined, even)

Code quote:

/// Returns a random string with the given `len`.
fn rand_string(rng: &mut StdRng, len: usize) -> String {
    rng.sample_iter(&Alphanumeric)
        .take(len)
        .map(char::from)
        .collect()
}

---

src/main.rs line 500 at r6 (raw file):

    client: OciClient,
    mpb: MultiProgress,
    reference: Reference,

this can be a reference to avoid cloning in the caller

---

src/main.rs line 514 at r6 (raw file):

    // Why is `oci-spec` storing layer size as `i64`? No idea. ¯\_(ツ)_/¯
    #[allow(clippy::cast_sign_loss)]

just do a checked conversion and panic.

---

src/main.rs line 523 at r6 (raw file):

    while let Some(res) = stream.next().await {
        let chunk = res.unwrap();
        layer_file.write_all(&chunk).await.unwrap();

is it safe to unwrap here?

Code quote:

        let chunk = res.unwrap();
        layer_file.write_all(&chunk).await.unwrap();

---

src/main.rs line 523 at r6 (raw file):

    while let Some(res) = stream.next().await {
        let chunk = res.unwrap();
        layer_file.write_all(&chunk).await.unwrap();

does layer_file want to be wrapped in https://docs.rs/tokio/latest/tokio/io/struct.BufWriter.html?

---

src/main.rs line 524 at r6 (raw file):

        let chunk = res.unwrap();
        layer_file.write_all(&chunk).await.unwrap();
        pb.inc(chunk.len() as u64);

use checked conversion

---

src/main.rs line 581 at r6 (raw file):

        layer_files.push(layer_file.clone());

        let (tx, mut rx) = tokio::sync::mpsc::channel(1);

you can get rid of this if you use combinators, I think.

---

src/main.rs line 582 at r6 (raw file):

        let (tx, mut rx) = tokio::sync::mpsc::channel(1);
        download_tasks.spawn(async move {

why spawn? presumably these are all IO bound, can't we just a futures combinator? spawn is only helpful if you need multithreading, which I think here you don't.

---

src/main.rs line 588 at r6 (raw file):

            });
        });
        rx.recv().await.unwrap()?;

I don't love all this panicking

Code quote:

            tx.send(res).await.unwrap_or_else(|e| {
                panic!("failed to send the result of the download task: {e:?}")
            });
        });
        rx.recv().await.unwrap()?;

---

src/main.rs line 600 at r6 (raw file):

    tokio::fs::create_dir_all(&ctx.rootfs_dir)
        .await
        .with_context(|| format!("failed to create rootfs directory {:?}", &ctx.rootfs_dir))?;

why are we doing this here?

Code quote:

    if ctx.rootfs_dir.exists() {
        tokio::fs::remove_dir_all(&ctx.rootfs_dir)
            .await
            .with_context(|| format!("failed to remove rootfs directory {:?}", &ctx.rootfs_dir))?;
    }
    tokio::fs::create_dir_all(&ctx.rootfs_dir)
        .await
        .with_context(|| format!("failed to create rootfs directory {:?}", &ctx.rootfs_dir))?;

---

src/main.rs line 609 at r6 (raw file):

where
    T: fmt::Debug + AsRef<Path>,
    D: fmt::Debug + AsRef<Path>,

this is wrong. don't print as Debug, use .display() instead.

---

src/main.rs line 619 at r6 (raw file):

        .unpack(&dest)
        .with_context(|| format!("failed to unpack tarball {tarball:?} into {dest:?}"))?;
    Ok(())

you can drop this and the preceding ?

---

src/main.rs line 664 at r6 (raw file):

        }
    });
    rx.recv()??;

isn't this only reading one result?

---

src/main.rs line 678 at r6 (raw file):

        .truncate(true)
        .open(digest_filename)
        .with_context(|| format!("failed to open the local digest file {digest_filename:?}"))?;

here and everywhere: do not use debug for paths

---

src/main.rs line 678 at r6 (raw file):

        .truncate(true)
        .open(digest_filename)
        .with_context(|| format!("failed to open the local digest file {digest_filename:?}"))?;

https://doc.rust-lang.org/stable/std/fs/fn.write.html

Code quote:

    let mut digest_file = OpenOptions::new()
        .write(true)
        .create(true)
        .truncate(true)
        .open(digest_filename)
        .with_context(|| format!("failed to open the local digest file {digest_filename:?}"))?;

---

src/main.rs line 700 at r6 (raw file):

    let digest_file = ctx.state_dir.join("digest");

    if let Some((digest, layer_files)) = tokio::runtime::Builder::new_multi_thread()

i think you don't need multi thread, see other comments

---

src/main.rs line 773 at r6 (raw file):

    debug!("Mounting /dev/mqueue");
    let mqueue_path = rootfs_dir.join("dev/mqueue");
    if !mqueue_path.exists() {

this is a pointless check

---

src/main.rs line 786 at r6 (raw file):

    debug!("Mounting /dev/pts");
    let pts_path = rootfs_dir.join("dev/pts");
    if !pts_path.exists() {

this is a pointless check

---

src/main.rs line 828 at r6 (raw file):

        }
    }
    if !full_dst.exists() {

why do you need this check?

---

src/main.rs line 848 at r6 (raw file):

                .with_context(|| {
                    format!("failed to create the bind mount destination file {full_dst:?}")
                })?;

what is this doing? is it the same as fs::write(full_dst, &[])?

Code quote:

            OpenOptions::new()
                .write(true)
                .create(true)
                .truncate(true)
                .open(&full_dst)
                .with_context(|| {
                    format!("failed to create the bind mount destination file {full_dst:?}")
                })?;

---

src/main.rs line 881 at r6 (raw file):

    // Mount all the user-provided volumes.
    for volume in &ctx.volumes {
        let parts: Vec<&str> = volume.split(':').collect();

pointless allocation

---

src/main.rs line 955 at r6 (raw file):

    };

    // mount_volumes(rootfs_dir, volumes.clone())?;

?

---

src/main.rs line 968 at r6 (raw file):

    if let Some(exit_code) = exit_code {
        process::exit(exit_code);
    }

yuck. return an error instead please

Code quote:

    if let Some(exit_code) = exit_code {
        process::exit(exit_code);
    }

---

src/main.rs line 1167 at r6 (raw file):

/// Expands a tilde with a home directory in the given path.
fn expand_tilde<P: AsRef<Path>>(p: P) -> anyhow::Result<PathBuf> {

nit: this could return a Cow

---

src/main.rs line 1172 at r6 (raw file):

        let mut home = match env::var_os("HOME") {
            Some(home) => PathBuf::from(home),
            None => bail!("the current user has no HOME directory"),

speculating. just say HOME isn't set instead.

---

src/main.rs line 1175 at r6 (raw file):

        };
        if !p.ends_with("~") {
            home.extend(p.components().skip(1));

?

[vadorovsky]

Reviewable status: all files reviewed, 43 unresolved discussions (waiting on @tamird)

---

src/main.rs line 38 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

every one of these constants is used in exactly one function. move them into those functions?

The PROGRESS_BAR constants are used by more functions in one of the next PRs:

#14

But yes, DOCKERFILE and LLVM_VERSION can go in the function bodies.

---

src/main.rs line 236 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

PathBuf?

I will have to handle the UTF-8 error before issuing mount syscalls, but that's probably worth doing - PathBuf gives me path validation on clippy's side for free.

---

src/main.rs line 312 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

is failed to start accurate? i'd just say failed to wait, matching phrasing above

Well, a failure of the wait call usually means that the command is not running / the expected PID doesn't exist. But you're right, being precise that wait failed is better.

---

src/main.rs line 600 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why are we doing this here?

mount syscall doesn't work if the destination doesn't exist. If you bind mount a file, it expects the destination to be a file. If you bind

---

src/main.rs line 828 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why do you need this check?

A mount syscall trying to bind mount into non-existent path If the source is a directory, the destination needs to be a directory too. If the source is a regular file, a destination also needs to be a file. So when the dest doesn't exist, I need to figure out whether it has to be a file or dir.

---

src/main.rs line 848 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

what is this doing? is it the same as fs::write(full_dst, &[])?

Not exactly (the following code is also truncating the content, in case there is any), but I think should work (truncation shouldn't be necessary, but let me double check).

---

src/main.rs line 968 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

yuck. return an error instead please

What I care about here is returning the same exit code as the containerized process. I will think of some cleaner way of doing so, but just returning a regular anyhow error won't be sufficient.

---

src/main.rs line 1175 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

?

What exactly don't you like here? Please be specific.

[tamird]

Reviewable status: all files reviewed, 43 unresolved discussions (waiting on @vadorovsky)

---

src/main.rs line 600 at r6 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

mount syscall doesn't work if the destination doesn't exist. If you bind mount a file, it expects the destination to be a file. If you bind

but the mount syscall isn't in this function. the name of this function is pull_image.

---

src/main.rs line 828 at r6 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

A mount syscall trying to bind mount into non-existent path If the source is a directory, the destination needs to be a directory too. If the source is a regular file, a destination also needs to be a file. So when the dest doesn't exist, I need to figure out whether it has to be a file or dir.

why can't you just make this code unconditional? what would break?

---

src/main.rs line 848 at r6 (raw file):
from the fs::write docs:

This function will create a file if it does not exist, and will entirely replace its contents if it does.

Sounds like truncation to me.

---

src/main.rs line 1175 at r6 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

What exactly don't you like here? Please be specific.

I don't understand this logic; what is this magic skip(1)? on line 824 you check for RootDir, is that what this is? it should use the same style if possible, or have a comment if it means to stay in this magical form.

[vadorovsky]

Reviewable status: 2 of 5 files reviewed, 42 unresolved discussions (waiting on @tamird)

---

src/main.rs line 47 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

is Clone used?

Nope, removed.

---

src/main.rs line 59 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

cmd.args(iter) might work?

Done.

---

src/main.rs line 104 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why not PathBuf?

Done.

---

src/main.rs line 173 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/is going to/will/

Done.

---

src/main.rs line 204 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

shouldn't this be a vec of PathBuf?

My intention is to take a pair of paths, consisting of source and destination, e.g. ~/foo:/root/foo, like Docker or Podman do.

But you're right that keeping that as Vec<String> is a smell, it makes more sense to parse it as Vec<(PathBuf, PathBuf(> with a custom clap parser. Done.

---

src/main.rs line 212 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Clone not used?

Done.

---

src/main.rs line 236 at r6 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

I will have to handle the UTF-8 error before issuing mount syscalls, but that's probably worth doing - PathBuf gives me path validation on clippy's side for free.

nvm, I can do Vec<(PathBuf, PathBuf)> here and parse properly beforehand.

---

src/main.rs line 258 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

let the caller do this.

Done.

---

src/main.rs line 281 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/run/spawn/ (be precise)

the word push is already going to be in {cmd:?}. I suggest not putting anything after the cmd's debug representation.

here and below.

Done.

---

src/main.rs line 296 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

drop "the"

here and below

Done.

---

src/main.rs line 339 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why the ref dance?

Done.

---

src/main.rs line 357 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto (nothing after {cmd:?})

Done.

---

src/main.rs line 363 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto (nothing after {cmd:?})

here and 2 more below

Done.

---

src/main.rs line 399 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto (failed to wait)

Done.

---

src/main.rs line 446 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

isn't this going to be incredibly difficult to keep in sync with the implementation? i'd sooner break this up and sprinkle the details inside the implementation.

Done.

---

src/main.rs line 495 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit: can be a lot closer to where it's used (inlined, even)

Done

---

src/main.rs line 500 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this can be a reference to avoid cloning in the caller

Good call, I also realized I don't have to clone all the other arguments. I still need to pass layer_file as an owned argument though (it's constructed locally in each future).

---

src/main.rs line 514 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

just do a checked conversion and panic.

Done.

---

src/main.rs line 523 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

is it safe to unwrap here?

Nope, my bad. Fixed.

---

src/main.rs line 523 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

does layer_file want to be wrapped in https://docs.rs/tokio/latest/tokio/io/struct.BufWriter.html?

Done.

---

src/main.rs line 524 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

use checked conversion

Done.

---

src/main.rs line 581 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

you can get rid of this if you use combinators, I think.

Nice, yeah, futures::future::try_join_all is all I needed.

---

src/main.rs line 582 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why spawn? presumably these are all IO bound, can't we just a futures combinator? spawn is only helpful if you need multithreading, which I think here you don't.

Got rid of it.

---

src/main.rs line 588 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I don't love all this panicking

Me neither, but try_join_all fixes that.

Unfortunately, I'm keeping a similar panic in threads inside unpack_image now and honestly, I don't know how can I avoid it.

---

src/main.rs line 600 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

but the mount syscall isn't in this function. the name of this function is pull_image.

Ah, sorry, this code removes the rootfs dir and creates it again. After pulling a new image, I want to completely wipe out the old rootfs and replace it with the new images' content. I updated the comment.

---

src/main.rs line 609 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is wrong. don't print as Debug, use .display() instead.

Done.

---

src/main.rs line 619 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

you can drop this and the preceding ?

Done.

---

src/main.rs line 664 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

isn't this only reading one result?

You're right. Should be fixed now.

---

src/main.rs line 678 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

here and everywhere: do not use debug for paths

Done.

---

src/main.rs line 678 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

https://doc.rust-lang.org/stable/std/fs/fn.write.html

Good call, I inlined that in prepare_container.

---

src/main.rs line 700 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

i think you don't need multi thread, see other comments

Done.

---

src/main.rs line 773 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is a pointless check

Done.

---

src/main.rs line 786 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is a pointless check

Done.

---

src/main.rs line 828 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why can't you just make this code unconditional? what would break?

Actually nothing, good point, done.

---

src/main.rs line 848 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

from the fs::write docs:

This function will create a file if it does not exist, and will entirely replace its contents if it does.

Sounds like truncation to me.

Fair, I switched to fs::write.

---

src/main.rs line 881 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

pointless allocation

Removed, I'm doing the parsing with clap now.

---

src/main.rs line 955 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

?

removed

---

src/main.rs line 1167 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit: this could return a Cow

Actually, after resolving some of you other comments (storing state_dir as PathBuf and not doing conversions inside ContaineContext::new(), I think it makes sense to just pass and return an owned PathBuf here.

---

src/main.rs line 1172 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

speculating. just say HOME isn't set instead.

Done.

---

src/main.rs line 1175 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I don't understand this logic; what is this magic skip(1)? on line 824 you check for RootDir, is that what this is? it should use the same style if possible, or have a comment if it means to stay in this magical form.

If your path looks like:

~/foo/bar/lorem/ipsum

then p.components() gives you an iterator with the following elements:

["~", "foo", "bar", "lorem", "ipsum"]

The magic skip(1) skips ~ and extends the HOME path with the following elements. It's pretty much the same trick I'm doing in OsVecDeque::command, where I'm passing all args except the first one.

I reorganized the code a bit and added some comments.

[tamird]

Reviewed 3 of 3 files at r7, 1 of 1 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 32 unresolved discussions (waiting on @vadorovsky)

---

src/main.rs line 38 at r6 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

The PROGRESS_BAR constants are used by more functions in one of the next PRs:

#14

But yes, DOCKERFILE and LLVM_VERSION can go in the function bodies.

not done?

---

src/main.rs line 104 at r6 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

Done.

hmm, does that default value work, given it has ~ in it?

---

src/main.rs line 500 at r6 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

Good call, I also realized I don't have to clone all the other arguments. I still need to pass layer_file as an owned argument though (it's constructed locally in each future).

Hm. Does that mean you have to pass it as owned? i think maybe not.

---

src/main.rs line 600 at r6 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

Ah, sorry, this code removes the rootfs dir and creates it again. After pulling a new image, I want to completely wipe out the old rootfs and replace it with the new images' content. I updated the comment.

but why here? this function doesn't otherwise touch rootfs_dir.

---

src/main.rs line 968 at r6 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

What I care about here is returning the same exit code as the containerized process. I will think of some cleaner way of doing so, but just returning a regular anyhow error won't be sufficient.

Ack. This isn't done, though.

---

src/main.rs line 203 at r9 (raw file):

    U::Err: std::error::Error + Send + Sync + 'static,
{
    let pos = s

i think you want to use slice::split and check that the iterator returns None after 2 next calls.

---

src/main.rs line 303 at r9 (raw file):

        .stderr
        .take()
        .unwrap_or_else(|| panic!("expected piped stderr command {cmd:?}"));

missing "in"

---

src/main.rs line 326 at r9 (raw file):

        .with_context(|| format!("failed to wait for command {cmd:?}"))?;
    if !status.success() {
        return Err(anyhow!("failed to push a container image: {status}"));

consider including {cmd:?} here

---

src/main.rs line 370 at r9 (raw file):

    let mut child = cmd
        .spawn()
        .with_context(|| format!("failed to run command {cmd:?}"))?;

s/run/spawn/

---

src/main.rs line 413 at r9 (raw file):

        .with_context(|| format!("failed to wait for command {cmd:?}"))?;
    if !status.success() {
        return Err(anyhow!("failed to build container image: {status}"));

ditto

---

src/main.rs line 430 at r9 (raw file):

        Ok(())
    } else {
        Err(anyhow!("Failed to push images: {errors:?}"))

nit: capitalization inconsistent with other errors

---

src/main.rs line 434 at r9 (raw file):

}

/// Returns a vector of environment variables to use in containerized processes

nit: drop the word vector, you're repeating the type (what if you changed it to a map? it's unlikely you'd notice the stale comment)

---

src/main.rs line 444 at r9 (raw file):

/// between native and cross builds, it's better to let them set the compiler
/// target.
fn prepare_env(triple: &Triple, override_cc_with_cross: bool) -> Vec<(String, String)> {

this could be a Vec<(Cow<'static, str>, Cow<'static, str>)>

---

src/main.rs line 448 at r9 (raw file):

    // `PATH` etc., which would break the containerized environment. We also
    // filter out variables prefixed by `CARGO` and `RUSTUP` to make sure that
    // the Rust toolchain inside container is isolated, expecially when

inside the container

---

src/main.rs line 467 at r9 (raw file):

        // Tell cargo what target to build for.
        ("CARGO_BUILD_TARGET".to_owned(), format!("{triple}")),
        // Use all LLVM components, including linker, standard C++ library and

the linker

---

src/main.rs line 468 at r9 (raw file):

        ("CARGO_BUILD_TARGET".to_owned(), format!("{triple}")),
        // Use all LLVM components, including linker, standard C++ library and
        // runtime libraries. Without being exlplicit about that, clang might

explicit

---

src/main.rs line 469 at r9 (raw file):

        // Use all LLVM components, including linker, standard C++ library and
        // runtime libraries. Without being exlplicit about that, clang might
        // still try to use GNU equivalents.

the GNU

---

src/main.rs line 472 at r9 (raw file):

        ("CXXFLAGS".to_owned(), format!("{} --stdlib=libc++", env::var("CXXFLAGS").unwrap_or_default())),
        ("LDFLAGS".to_owned(), format!("{} -fuse-ld=lld -rtlib=compiler-rt -unwindlib=libunwind", env::var("LDFLAGS").unwrap_or_default())),
        // Include the directories of LLVM and Rust toolchains in `PATH`.

the LLVM ...

---

src/main.rs line 478 at r9 (raw file):

        // Point to the directory with Rust toolchains.
        ("RUSTUP_HOME".to_owned(), "/root/.rustup".to_owned()),
        // Tell Rust to use an appropriate clang wrapper as a linker and the

as the linker? this phrasing is confusing, I think you want

Tell Rust to use the cross sysroot and to use a clang wrapper as its linker.

---

src/main.rs line 519 at r9 (raw file):

    // Why is `oci-spec` storing layer size as `i64`? No idea. ¯\_(ツ)_/¯
    let content_length = stream.content_length.unwrap_or(

use unwrap_or_else?

---

src/main.rs line 521 at r9 (raw file):

    let content_length = stream.content_length.unwrap_or(
        u64::try_from(layer.size)
            .unwrap_or_else(|e| panic!("invalid layer size: {}: {e:?}", layer.size)),

given you don't panic below, maybe don't panic here either

---

src/main.rs line 530 at r9 (raw file):

    while let Some(res) = stream.next().await {
        let chunk = res?;
        layer_file.write_all(&chunk).await?;

maybe add some .context(..)

Code quote:

        let chunk = res?;
        layer_file.write_all(&chunk).await?;

---

src/main.rs line 536 at r9 (raw file):

        pb.inc(chunk_len);
    }
    layer_file.flush().await?;

context?

---

src/main.rs line 583 at r9 (raw file):

    let futures = manifest.layers.iter().map(|layer| {
        let layer_file = download_dir.join(&layer.digest);
        layer_files.push(layer_file.clone());

quite sure you don't need this clone - you can just do this push after the download_layer call below.

also, why do you need to mutate the outer variable? can't you return this from the future? something like

download_layer(....).map(|()| layer_file)

---

src/main.rs line 608 at r9 (raw file):

    T: AsRef<Path>,
    D: AsRef<Path>,
{

on the first line, just write

let tarball_path = tarball_path.as_ref();
let dest_path = tarball_path.as_ref();

to avoid repeating it below.

---

src/main.rs line 669 at r9 (raw file):

        }
    });
    drop(tx);

nit: instead of this, I think you can make thread::scope(|s| { a move closure

---

src/main.rs line 675 at r9 (raw file):

    fs::remove_dir_all(download_dir)
        .with_context(|| format!("failed to remove download directory {download_dir:?}"))?;

this is a path being logged using debug. i suspect there are many more left over

---

src/main.rs line 677 at r9 (raw file):

        .with_context(|| format!("failed to remove download directory {download_dir:?}"))?;

    Ok(())

nit: can remove this and the preceding ?

---

src/main.rs line 695 at r9 (raw file):

    let download_dir = Path::new("/tmp").join(download_dir);
    fs::create_dir_all(&download_dir)
        .with_context(|| format!("failed to create download directory {download_dir:?}"))?;

e.g. here

---

src/main.rs line 774 at r9 (raw file):

        Some("mode=755,size=65536k"),
    )
    .with_context(|| format!("failed to mount tmpfs into {dev_path:?}"))?;

and here

---

src/main.rs line 1160 at r9 (raw file):

    //
    // Otherwise, just return the original path.
    if let Some(Component::Normal(first)) = components.next() {

I think this can just be if matches!(Some(Component::Normal(first)) if first == "~", components.next()) { ... } else { Ok(p) }

instead of having two different else arms with the same body

---

src/main.rs line 1163 at r9 (raw file):

        if first == "~" {
            // Resolve the home path of the user.
            let mut home = match env::var_os("HOME") {

why not use https://doc.rust-lang.org/nightly/std/env/fn.home_dir.html? it's been un-deprecated

[vadorovsky]

Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @tamird)

---

src/main.rs line 38 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

not done?

Now done, sorry

---

src/main.rs line 104 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

hmm, does that default value work, given it has ~ in it?

Yes, it goes into expand_tilde in line 1186. And a PathBuf starting with ~/ is a valid one (~ is treated as a regular component).

---

src/main.rs line 500 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Hm. Does that mean you have to pass it as owned? i think maybe not.

Yeah, given your comments below, I don't have to. Done.

---

src/main.rs line 600 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

but why here? this function doesn't otherwise touch rootfs_dir.

Good point, move it out to separate function.

---

src/main.rs line 968 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Ack. This isn't done, though.

OK, the only idea I managed to come with is... returning exit the exit code down to main(). I guess still better than an explicit exit.

---

src/main.rs line 203 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

i think you want to use slice::split and check that the iterator returns None after 2 next calls.

Done.

---

src/main.rs line 303 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

missing "in"

Done.

---

src/main.rs line 326 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

consider including {cmd:?} here

Done. Are you fine with keeping the {status} after {cmd}? This is the Display implementation for ExitStatus:

https://github.com/rust-lang/rust/blob/5f145689b1c0a313ee737de296a57d1479c18cb5/library/std/src/sys/process/unix/unix.rs#L1188-L1208

---

src/main.rs line 370 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/run/spawn/

Done.

---

src/main.rs line 413 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto

Done.

---

src/main.rs line 430 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit: capitalization inconsistent with other errors

Done

---

src/main.rs line 434 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit: drop the word vector, you're repeating the type (what if you changed it to a map? it's unlikely you'd notice the stale comment)

Done

---

src/main.rs line 444 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this could be a Vec<(Cow<'static, str>, Cow<'static, str>)>

I tried, Cow<_, str> doesn't satisfy AsRef, so I would have to do an another dumb conversion before calling Command::envs.

---

src/main.rs line 448 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

inside the container

Done.

---

src/main.rs line 467 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

the linker

Done.

---

src/main.rs line 468 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

explicit

Done.

---

src/main.rs line 469 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

the GNU

Done.

---

src/main.rs line 472 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

the LLVM ...

Done.

---

src/main.rs line 478 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

as the linker? this phrasing is confusing, I think you want

Tell Rust to use the cross sysroot and to use a clang wrapper as its linker.

Done.

---

src/main.rs line 519 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

use unwrap_or_else?

Given the comment below and handling the error with ?, I rewrote it as a match statement (closures make it hard to use ?).

---

src/main.rs line 521 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

given you don't panic below, maybe don't panic here either

Done.

---

src/main.rs line 530 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

maybe add some .context(..)

Done.

---

src/main.rs line 536 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

context?

Done.

---

src/main.rs line 583 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

quite sure you don't need this clone - you can just do this push after the download_layer call below.

also, why do you need to mutate the outer variable? can't you return this from the future? something like

download_layer(....).map(|()| layer_file)

I didn't do exactly what you proposed, but I think I found even cleaner way.

---

src/main.rs line 608 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

on the first line, just write

let tarball_path = tarball_path.as_ref();
let dest_path = tarball_path.as_ref();

to avoid repeating it below.

Done.

---

src/main.rs line 669 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit: instead of this, I think you can make thread::scope(|s| { a move closure

Done

---

src/main.rs line 675 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is a path being logged using debug. i suspect there are many more left over

Done.

---

src/main.rs line 677 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit: can remove this and the preceding ?

Done

---

src/main.rs line 695 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

e.g. here

OK, yeah, I found over 10 more, sorry for not spotting it before.

---

src/main.rs line 774 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

and here

Done.

---

src/main.rs line 1160 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I think this can just be if matches!(Some(Component::Normal(first)) if first == "~", components.next()) { ... } else { Ok(p) }

instead of having two different else arms with the same body

Looks like it's impossible to use if inside matches! macro:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=c8dce2a25c3006ccef6cdb383fa5ec05

I can write a match statement though, it will still let me avoid additional nesting.

---

src/main.rs line 1163 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why not use https://doc.rust-lang.org/nightly/std/env/fn.home_dir.html? it's been un-deprecated

It's still deprecated in stable https://doc.rust-lang.org/std/env/fn.home_dir.html
I'll wait until it gets un-deprecated in the next stable release

[tamird]

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @vadorovsky)

---

src/main.rs line 326 at r9 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

Done. Are you fine with keeping the {status} after {cmd}? This is the Display implementation for ExitStatus:

https://github.com/rust-lang/rust/blob/5f145689b1c0a313ee737de296a57d1479c18cb5/library/std/src/sys/process/unix/unix.rs#L1188-L1208

Yeah I think it's ok

---

src/main.rs line 444 at r9 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

I tried, Cow<_, str> doesn't satisfy AsRef, so I would have to do an another dumb conversion before calling Command::envs.

🤷

---

src/main.rs line 1160 at r9 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

Looks like it's impossible to use if inside matches! macro:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=c8dce2a25c3006ccef6cdb383fa5ec05

I can write a match statement though, it will still let me avoid additional nesting.

Just had the syntax wrong: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=5b3390a624b78d10f9715da7cf1f6591

---

src/main.rs line 1163 at r9 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

It's still deprecated in stable https://doc.rust-lang.org/std/env/fn.home_dir.html
I'll wait until it gets un-deprecated in the next stable release

Add a TODO plz

---

src/main.rs line 216 at r11 (raw file):

    if let Some(component) = components.next() {
        return Err(anyhow!("expected only two values separated by `:` in format `[key]:[value]`, got an additional unwanted component `{component}` in `{s}`").into());
    }

I would actually use collect here and then check empty; that would not allocate in the success case.

Code quote:

    if let Some(component) = components.next() {
        return Err(anyhow!("expected only two values separated by `:` in format `[key]:[value]`, got an additional unwanted component `{component}` in `{s}`").into());
    }

---

src/main.rs line 338 at r11 (raw file):

    if !status.success() {
        return Err(anyhow!(
            "failed to push a container image {status} with command {cmd:?}, {status}"

nit: s/,/:/

---

src/main.rs line 430 at r11 (raw file):

    if !status.success() {
        return Err(anyhow!(
            "failed to build container image with command {cmd:?}, {status}"

ditto

---

src/main.rs line 777 at r11 (raw file):

        )
    })?;
    Ok(())

nit: can remove

---

src/main.rs line 810 at r11 (raw file):

    )
    .with_context(|| format!("failed to mount proc filesystem into {}", dest.display()))?;
    Ok(())

nit: can remove

[vadorovsky]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @tamird)

---

src/main.rs line 216 at r11 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I would actually use collect here and then check empty; that would not allocate in the success case.

I'm struggling to see the benefit. How is collect better than checking one element?

But I'm noticing something else, I don't really need to consume the component. Probably something like

    if components.next().is_some() {
        return Err(anyhow!("expected only two values separated by `:` in format `[key]:[value]`, got  `{s}`").into());
    }

would be better.

[tamird]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @vadorovsky)

---

src/main.rs line 216 at r11 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

I'm struggling to see the benefit. How is collect better than checking one element?

But I'm noticing something else, I don't really need to consume the component. Probably something like

    if components.next().is_some() {
        return Err(anyhow!("expected only two values separated by `:` in format `[key]:[value]`, got  `{s}`").into());
    }

would be better.

the benefit is that you can emit all the remaining items, rather than just the very next one. but your suggestion is better (emit the whole string).

[vadorovsky]

Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @tamird)

---

src/main.rs line 1160 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Just had the syntax wrong: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=5b3390a624b78d10f9715da7cf1f6591

Nice, thanks!

---

src/main.rs line 1163 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Add a TODO plz

Done.

---

src/main.rs line 216 at r11 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

the benefit is that you can emit all the remaining items, rather than just the very next one. but your suggestion is better (emit the whole string).

Done.

---

src/main.rs line 430 at r11 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto

Done.

[tamird]

Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vadorovsky)

---

src/main.rs line 795 at r11 (raw file):

    write_mapping(&proc_self.join("gid_map"), Gid::current().as_raw())
        .context("failed to write GID mapping")?;
    Ok(())

would've kept this one for symmetry 🤷

[tamird]

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vadorovsky)

[tamird]

Reviewable status: complete! all files reviewed, all discussions resolved

---

src/main.rs line 444 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

🤷

#17