Some comments inline.
On Tue May 20, 2025 at 2:42 PM CEST, Filip Schauer wrote:
> [..]
> diff --git a/proxmox-oci/src/lib.rs b/proxmox-oci/src/lib.rs
> new file mode 100644
> index 00000000..57bd48b4
> --- /dev/null
> +++ b/proxmox-oci/src/lib.rs
> @@ -0,0 +1,165 @@
> [..]
> +/// Build a mapping from uncompressed layer digests (as found in the image
> config's `rootfs.diff_ids`)
> +/// to their corresponding compressed-layer digests (i.e. the filenames
> under `blobs/<algorithm>/<digest>`)
> +fn build_layer_map<R: Read + Seek>(
> + oci_tar_image: &mut OciTarImage<R>,
> + image_manifest: &ImageManifest,
> +) -> HashMap<oci_spec::image::Digest, oci_spec::image::Descriptor> {
> + let mut layer_mapping = HashMap::new();
> +
> + for layer in image_manifest.layers() {
> + match layer.media_type() {
> + MediaType::ImageLayer | MediaType::ImageLayerNonDistributable =>
> {
> + layer_mapping.insert(layer.digest().clone(), layer.clone());
> + }
> + MediaType::ImageLayerGzip
> + | MediaType::ImageLayerNonDistributableGzip
> + | MediaType::ImageLayerZstd
> + | MediaType::ImageLayerNonDistributableZstd => {
> + let compressed_blob =
> oci_tar_image.open_blob(layer.digest()).unwrap();
> + let mut decoder: Box<dyn Read> = match layer.media_type() {
> + MediaType::ImageLayerGzip |
> MediaType::ImageLayerNonDistributableGzip => {
> + Box::new(GzDecoder::new(compressed_blob))
> + }
> + MediaType::ImageLayerZstd |
> MediaType::ImageLayerNonDistributableZstd => {
> +
> Box::new(zstd::Decoder::new(compressed_blob).unwrap())
> + }
> + _ => unreachable!(),
> + };
> + let mut hasher = Sha256::new();
> + let mut buf = proxmox_io::boxed::zeroed(4096);
> +
> + loop {
> + let bytes_read = decoder.read(&mut buf).unwrap();
> + if bytes_read == 0 {
> + break;
> + }
> +
> + hasher.update(&buf[..bytes_read]);
> + }
> +
> + let uncompressed_digest =
> + oci_spec::image::Sha256Digest::from_str(&format!("{:x}",
> hasher.finalize()))
> + .unwrap();
For this variant, the hashing part could be extracted into a separate
function and then the Gzip/Zstd can be proper independent match arms.
E.g. each having the decoder construction, calling the layer hashing
function and then `.insert()` mapping below.
Has the nice side-effect of avoiding things like the `unreachable!()`
above, too.
> +
> + layer_mapping.insert(uncompressed_digest.into(),
> layer.clone());
> + }
> + _ => (),
> + }
> + }
> +
> + layer_mapping
> +}
> +
> +pub enum ProxmoxOciError {
> + NotAnOciImage,
> + OciSpecError(OciSpecError),
> +}
Since this is only used by `parse_oci_image()`, rename it to something
like ParseError and/or ExtractError, depending on the below.
> +
> +impl From<OciSpecError> for ProxmoxOciError {
> + fn from(oci_spec_err: OciSpecError) -> Self {
> + Self::OciSpecError(oci_spec_err)
> + }
> +}
> +
> +impl From<std::io::Error> for ProxmoxOciError {
> + fn from(io_err: std::io::Error) -> Self {
> + Self::OciSpecError(io_err.into())
> + }
> +}
> +
> +pub fn parse_oci_image(
Seeing as this function not only parses, but also extract the contents
at the same time, this should be named something like
`parse_and_extract_image()`?
(The `_oci` is implied through the crate name already anyway.)
> + oci_tar_path: &str,
> + rootfs_path: &str,
> +) -> Result<Option<Config>, ProxmoxOciError> {
> + let oci_tar_file = File::open(oci_tar_path)?;
> + let mut oci_tar_image = match OciTarImage::new(oci_tar_file) {
> + Ok(oci_tar_image) => oci_tar_image,
> + Err(_) => return Err(ProxmoxOciError::NotAnOciImage),
I'd also separate the `OciSpecError::Io(_)` variant here, possible into
a separate `ProxmoxOciError::IoError` variant. Callers might want to
handle I/O errors other than e.g. actual parsing errors.
> + };
> + let image_manifest = oci_tar_image.image_manifest(&Arch::Amd64)?;
> +
> + let image_config_descriptor = image_manifest.config();
> +
> + if image_config_descriptor.media_type() != &MediaType::ImageConfig {
> + return Err(ProxmoxOciError::OciSpecError(OciSpecError::Other(
> + "ImageConfig has wrong media type".into(),
> + )));
Can simply be separate variant, e.g. `ProxmoxOciError::WrongMediaType`
> + }
> +
> + let image_config_file =
> oci_tar_image.open_blob(image_config_descriptor.digest())?;
> + let image_config = ImageConfiguration::from_reader(image_config_file)?;
> +
> + extract_oci_image_rootfs(
> + &mut oci_tar_image,
> + &image_manifest,
> + &image_config,
> + rootfs_path.into(),
> + )?;
> +
> + Ok(image_config.config().clone())
> +}
> +
> +fn extract_oci_image_rootfs<R: Read + Seek>(
> + oci_tar_image: &mut OciTarImage<R>,
> + image_manifest: &ImageManifest,
> + image_config: &ImageConfiguration,
> + target_path: PathBuf,
> +) -> oci_spec::Result<()> {
> + if !target_path.exists() {
> + return Err(OciSpecError::Other(
> + "Rootfs destination path not found".into(),
> + ));
> + }
> +
> + let layer_map = build_layer_map(oci_tar_image, image_manifest);
> +
> + for layer in image_config.rootfs().diff_ids() {
> + let layer_digest = oci_spec::image::Digest::from_str(layer)?;
> +
> + let layer_descriptor = match layer_map.get(&layer_digest) {
> + Some(descriptor) => descriptor,
> + None => {
> + return Err(OciSpecError::Other(
> + "Unknown layer digest found in rootfs.diff_ids".into(),
> + ));
> + }
> + };
> +
> + let layer_file = oci_tar_image.open_blob(layer_descriptor.digest())?;
> +
> + let tar_file: Box<dyn Read> = match layer_descriptor.media_type() {
> + MediaType::ImageLayer | MediaType::ImageLayerNonDistributable =>
> Box::new(layer_file),
> + MediaType::ImageLayerGzip |
> MediaType::ImageLayerNonDistributableGzip => {
> + Box::new(GzDecoder::new(layer_file))
> + }
> + MediaType::ImageLayerZstd |
> MediaType::ImageLayerNonDistributableZstd => {
> + Box::new(zstd::Decoder::new(layer_file)?)
> + }
> + _ => {
> + return Err(OciSpecError::Other(
> + "Encountered invalid media type for rootfs layer".into(),
> + ));
Again I'd just introduce a separate error variant or even re-use
`ProxmoxOciError::WrongMediaType`, seems appropriate too.
> + }
> + };
> +
> + let mut archive = Archive::new(tar_file);
> + archive.set_preserve_ownerships(true);
> + archive.unpack(&target_path)?;
> + }
> +
> + Ok(())
> +}
> diff --git a/proxmox-oci/src/oci_tar_image.rs
> b/proxmox-oci/src/oci_tar_image.rs
> new file mode 100644
> index 00000000..02199c75
> --- /dev/null
> +++ b/proxmox-oci/src/oci_tar_image.rs
> @@ -0,0 +1,173 @@
> [..]
> +pub struct OciTarImage<R: Read + Seek> {
> + archive: TarArchive<R>,
> + image_index: ImageIndex,
> +}
> +
> +impl<R: Read + Seek> OciTarImage<R> {
> [..]
> + pub fn image_manifest(&mut self, architecture: &Arch) ->
> oci_spec::Result<ImageManifest> {
> + let image_manifest_descriptor = match
> self.image_index.manifests().iter().find(|&x| {
> + x.media_type() == &MediaType::ImageManifest
> + && x.platform()
> + .as_ref()
> + .is_none_or(|platform| platform.architecture() ==
> architecture)
> + }) {
> + Some(descriptor) => descriptor,
> + None => {
> + return Err(OciSpecError::Io(std::io::Error::new(
> + std::io::ErrorKind::NotFound,
> + "Image manifest not found for architecture",
> + )));
Doesn't really seem like an I/O error?
> + }
> + };
> +
> + let image_manifest_file =
> self.open_blob(&image_manifest_descriptor.digest().clone())?;
> +
> + ImageManifest::from_reader(image_manifest_file)
> + }
> +}
> +
> +fn get_blob_path(digest: &Digest) -> PathBuf {
> + let algorithm = digest.algorithm().as_ref();
> + let digest = digest.digest();
> + PathBuf::from(format!("blobs/{algorithm}/{digest}"))
The path can also be constructed using the primivites from `PathBuf`
itself, e.g.
let mut path = PathBuf::new();
path.push(digest.algorithm().as_ref());
..
> +}
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
