Re: [PATCH v1-changed] erofs-utils: refactor OCI code for better modularity

[Gao Xiang <hsiangkao@linux.alibaba.com>]

On 2025/9/1 15:01, ChengyuZhu6 wrote:
> From: Chengyu Zhu <hudsonzhu@tencent.com>
> 
> Refactor OCI code to improve code organization and maintainability:
> 
> - Add `struct ocierofs_layer_info` to encapsulate layer metadata
> - Extract authentication logic into `ocierofs_prepare_auth()`
> - Split layer processing into `ocierofs_prepare_layers()`
> - Move OCI parsing functions from `mkfs/main.c` to `lib/remotes/oci.c`
> - Add `ocierofs_process_tar_stream()` for separate tar processing
> - Improve error handling with `ocierofs_free_layers_info()`
> - Refactor `ocierofs_extract_layer()` to return file descriptor
> 
> Signed-off-by: Chengyu Zhu <hudsonzhu@tencent.com>
> ---
>   lib/liberofs_oci.h | 100 +++++++++
>   lib/remotes/oci.c  | 540 +++++++++++++++++++++++++++++++++------------
>   mkfs/main.c        | 200 +----------------
>   3 files changed, 506 insertions(+), 334 deletions(-)
> 
> diff --git a/lib/liberofs_oci.h b/lib/liberofs_oci.h
> index 3a8108b..698fe07 100644
> --- a/lib/liberofs_oci.h
> +++ b/lib/liberofs_oci.h
> @@ -19,6 +19,23 @@ struct erofs_inode;
>   struct CURL;
>   struct erofs_importer;
>   
> +/**
> + * struct ocierofs_layer_info
> + * @digest: OCI content-addressable digest (e.g. "sha256:...")
> + * @media_type: mediaType string from the manifest
> + * @size: layer size in bytes from the manifest (0 if not available)
> + *
> + * This structure is exposed to callers so they can enumerate image layers,
> + * decide which ones to fetch, and pass the digest back to download APIs.
> + * Fields are heap-allocated NUL-terminated strings owned by the caller
> + * once returned from public APIs; the caller must free them.
> + */
> +struct ocierofs_layer_info {
> +	char *digest;
> +	char *media_type;
> +	u64 size;
> +};
> +
>   /**
>    * struct erofs_oci_params - OCI configuration parameters
>    * @registry: registry hostname (e.g., "registry-1.docker.io")
> @@ -88,6 +105,89 @@ int erofs_oci_params_set_string(char **field, const char *value);
>    */
>   int ocierofs_build_trees(struct erofs_importer *importer, struct erofs_oci *oci);
>   
> +/*
> + * ocierofs_parse_options - Parse comma-separated OCI options string
> + * @oci: OCI client structure to update
> + * @options_str: comma-separated options string
> + *
> + * Parse OCI options string containing comma-separated key=value pairs.
> + * Supported options include platform, layer, username, and password.
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +int ocierofs_parse_options(struct erofs_oci *oci, char *options_str);

Can we leave this functionality in `mkfs/main.c`.

liberofs is not the place to keep option parser.

> +
> +/*
> + * ocierofs_parse_ref - Parse OCI image reference string
> + * @oci: OCI client structure to update
> + * @ref_str: OCI image reference in various formats
> + *
> + * Parse OCI image reference which can be in formats:
> + * - registry.example.com/namespace/repo:tag
> + * - namespace/repo:tag (uses default registry)
> + * - repo:tag (adds library/ prefix for Docker Hub)

Is there some reference for this rule?

> + * - repo (uses default tag "latest")
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +int ocierofs_parse_ref(struct erofs_oci *oci, const char *ref_str);
> +
> +/*
> + * ocierofs_prepare_layers - Prepare OCI layers for processing
> + * @oci: OCI client structure with configured parameters
> + * @auth_header: Pointer to store authentication header
> + * @using_basic: Pointer to store basic auth flag
> + * @manifest_digest: Pointer to store manifest digest
> + * @layers: Pointer to store layers information
> + * @layer_count: Pointer to store number of layers
> + * @start_index: Pointer to store starting layer index
> + *
> + * Prepare authentication, get manifest digest and layers information
> + * for OCI image processing. This function handles all the preparation
> + * work needed before processing OCI layers.
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +int ocierofs_prepare_layers(struct erofs_oci *oci, char **auth_header,
> +			   bool *using_basic, char **manifest_digest,
> +			   struct ocierofs_layer_info ***layers,
> +			   int *layer_count, int *start_index);

Could we have a way to wrap these arguments into
a structure too?

> +
> +/**
> + * ocierofs_free_layers_info - Free layer information array
> + * @layers: array of layer information structures
> + * @count: number of layers in the array
> + *
> + * Free all layer information structures and the array itself.
> + * This function handles NULL pointers safely.
> + */
> +void ocierofs_free_layers_info(struct ocierofs_layer_info **layers, int count);
> +
> +/**
> + * ocierofs_prepare_auth - Prepare authentication for OCI requests
> + * @oci: OCI client structure
> + * @auth_header_out: pointer to store authentication header
> + * @using_basic_auth: pointer to store basic auth flag
> + *
> + * Prepare authentication header for OCI registry requests.
> + * This function handles both token-based and basic authentication.
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +int ocierofs_prepare_auth(struct erofs_oci *oci, char **auth_header_out,
> +			  bool *using_basic_auth);
> +
> +/**
> + * ocierofs_curl_clear_auth - Clear basic authentication from CURL handle
> + * @curl: CURL handle to clear authentication from
> + *
> + * Clear basic authentication credentials from a CURL handle.
> + * This should be called after using basic authentication to clean up.
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +int ocierofs_curl_clear_auth(struct CURL *curl);

pass in `erofs_oci` instead?

> +
>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/lib/remotes/oci.c b/lib/remotes/oci.c
> index 0fb8c1f..9774d8d 100644
> --- a/lib/remotes/oci.c
> +++ b/lib/remotes/oci.c
> @@ -42,7 +42,6 @@ struct erofs_oci_response {
>   };
>   
>   struct erofs_oci_stream {
> -	struct erofs_tarfile tarfile;
>   	const char *digest;
>   	int blobfd;
>   };
> @@ -111,7 +110,7 @@ static int ocierofs_curl_setup_basic_auth(struct CURL *curl, const char *usernam
>   	return 0;
>   }
>   
> -static int ocierofs_curl_clear_auth(struct CURL *curl)
> +int ocierofs_curl_clear_auth(struct CURL *curl)
>   {
>   	curl_easy_setopt(curl, CURLOPT_USERPWD, NULL);
>   	curl_easy_setopt(curl, CURLOPT_HTTPAUTH, CURLAUTH_NONE);
> @@ -181,7 +180,7 @@ static int ocierofs_request_perform(struct erofs_oci *oci,
>   
>   	ret = ocierofs_curl_setup_rq(oci->curl, req->url,
>   				     OCIEROFS_HTTP_GET, req->headers,
> -			             ocierofs_write_callback, resp,
> +				     ocierofs_write_callback, resp,
>   				     NULL, NULL);
>   	if (ret)
>   		return ret;
> @@ -568,7 +567,7 @@ static char *ocierofs_get_manifest_digest(struct erofs_oci *oci,
>   	const char *api_registry;
>   	int ret = 0, len, i;
>   
> -	if (!registry || !repository || !tag || !platform)
> +	if (!registry || !repository || !tag)
>   		return ERR_PTR(-EINVAL);
>   
>   	api_registry = (!strcmp(registry, DOCKER_REGISTRY)) ? DOCKER_API_REGISTRY : registry;
> @@ -581,8 +580,8 @@ static char *ocierofs_get_manifest_digest(struct erofs_oci *oci,
>   
>   	req.headers = curl_slist_append(req.headers,
>   		"Accept: " DOCKER_MEDIATYPE_MANIFEST_LIST ","
> -		OCI_MEDIATYPE_INDEX "," DOCKER_MEDIATYPE_MANIFEST_V1 ","
> -		DOCKER_MEDIATYPE_MANIFEST_V2);
> +		OCI_MEDIATYPE_INDEX "," OCI_MEDIATYPE_MANIFEST ","
> +		DOCKER_MEDIATYPE_MANIFEST_V1 "," DOCKER_MEDIATYPE_MANIFEST_V2);
>   
>   	ret = ocierofs_request_perform(oci, &req, &resp);
>   	if (ret)
> @@ -663,7 +662,24 @@ out:
>   	return ret ? ERR_PTR(ret) : digest;
>   }
>   
> -static char **ocierofs_get_layers_info(struct erofs_oci *oci,
> +void ocierofs_free_layers_info(struct ocierofs_layer_info **layers, int count)
> +{
> +	int i;
> +
> +	if (!layers)
> +		return;
> +
> +	for (i = 0; i < count; i++) {
> +		if (layers[i]) {
> +			free(layers[i]->digest);
> +			free(layers[i]->media_type);
> +			free(layers[i]);
> +		}
> +	}
> +	free(layers);
> +}
> +
> +static struct ocierofs_layer_info **ocierofs_fetch_layers_info(struct erofs_oci *oci,
>   				       const char *registry,
>   				       const char *repository,
>   				       const char *digest,
> @@ -672,10 +688,10 @@ static char **ocierofs_get_layers_info(struct erofs_oci *oci,
>   {
>   	struct erofs_oci_request req = {};
>   	struct erofs_oci_response resp = {};
> -	json_object *root, *layers, *layer, *digest_obj;
> -	char **layers_info = NULL;
> +	json_object *root, *layers, *layer, *digest_obj, *media_type_obj, *size_obj;
> +	struct ocierofs_layer_info **layers_info = NULL;
>   	const char *api_registry;
> -	int ret, len, i, j;
> +	int ret, len, i;
>   
>   	if (!registry || !repository || !digest || !layer_count)
>   		return ERR_PTR(-EINVAL);
> @@ -725,7 +741,7 @@ static char **ocierofs_get_layers_info(struct erofs_oci *oci,
>   		goto out_json;
>   	}
>   
> -	layers_info = calloc(len, sizeof(char *));
> +	layers_info = calloc(len, sizeof(*layers_info));
>   	if (!layers_info) {
>   		ret = -ENOMEM;
>   		goto out_json;
> @@ -740,11 +756,25 @@ static char **ocierofs_get_layers_info(struct erofs_oci *oci,
>   			goto out_free;
>   		}
>   
> -		layers_info[i] = strdup(json_object_get_string(digest_obj));
> +		layers_info[i] = calloc(1, sizeof(**layers_info));
>   		if (!layers_info[i]) {
>   			ret = -ENOMEM;
>   			goto out_free;
>   		}
> +		layers_info[i]->digest = strdup(json_object_get_string(digest_obj));
> +		if (!layers_info[i]->digest) {
> +			ret = -ENOMEM;
> +			goto out_free;
> +		}
> +		if (json_object_object_get_ex(layer, "mediaType", &media_type_obj))
> +			layers_info[i]->media_type = strdup(json_object_get_string(media_type_obj));
> +		else
> +			layers_info[i]->media_type = NULL;
> +
> +		if (json_object_object_get_ex(layer, "size", &size_obj))
> +			layers_info[i]->size = json_object_get_int64(size_obj);
> +		else
> +			layers_info[i]->size = 0;
>   	}
>   
>   	*layer_count = len;
> @@ -756,11 +786,7 @@ static char **ocierofs_get_layers_info(struct erofs_oci *oci,
>   	return layers_info;
>   
>   out_free:
> -	if (layers_info) {
> -		for (j = 0; j < i; j++)
> -			free(layers_info[j]);
> -	}
> -	free(layers_info);
> +	ocierofs_free_layers_info(layers_info, i);
>   out_json:
>   	json_object_put(root);
>   out:
> @@ -771,8 +797,93 @@ out:
>   	return ERR_PTR(ret);
>   }
>   
> -static int ocierofs_extract_layer(struct erofs_oci *oci, struct erofs_importer *importer,
> -				  const char *digest, const char *auth_header)
> +/**
> + * ocierofs_process_tar_stream - Process tar stream from file descriptor
> + * @importer: EROFS importer structure
> + * @fd: File descriptor containing tar data
> + *
> + * Initialize tar stream, parse all entries, and clean up resources.
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +static int ocierofs_process_tar_stream(struct erofs_importer *importer, int fd)
> +{
> +	struct erofs_tarfile tarfile = {};
> +	int ret;
> +
> +	memset(&tarfile, 0, sizeof(tarfile));

	struct erofs_tarfile tarfile = {};

already indicates
	memset(&tarfile, 0, sizeof(tarfile));

Thanks,
Gao Xiang