Re: [PATCH v4 2/3] erofs-utils: refactor OCI code for better modularity

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

Hi Chengyu,

On 2025/9/4 13:33, ChengyuZhu6 wrote:
> From: Chengyu Zhu <hudsonzhu@tencent.com>

..

> @@ -96,6 +142,14 @@ static int ocierofs_curl_setup_common_options(struct CURL *curl)
>   	curl_easy_setopt(curl, CURLOPT_TIMEOUT, 120L);
>   	curl_easy_setopt(curl, CURLOPT_NOSIGNAL, 1L);
>   	curl_easy_setopt(curl, CURLOPT_USERAGENT, "ocierofs/" PACKAGE_VERSION);
> +	curl_easy_setopt(curl, CURLOPT_ACCEPT_ENCODING, "");
> +	curl_easy_setopt(curl, CURLOPT_TCP_KEEPALIVE, 1L);
> +#if defined(CURLOPT_TCP_KEEPIDLE)
> +	curl_easy_setopt(curl, CURLOPT_TCP_KEEPIDLE, 30L);
> +#endif
> +#if defined(CURLOPT_TCP_KEEPINTVL)
> +	curl_easy_setopt(curl, CURLOPT_TCP_KEEPINTVL, 15L);
> +#endif
>   	return 0;
>   }
>   
> @@ -114,10 +168,10 @@ static int ocierofs_curl_setup_basic_auth(struct CURL *curl, const char *usernam
>   	return 0;
>   }
>   
> -static int ocierofs_curl_clear_auth(struct CURL *curl)
> +static int ocierofs_curl_clear_auth(struct ocierofs_ctx *ctx)
>   {
> -	curl_easy_setopt(curl, CURLOPT_USERPWD, NULL);
> -	curl_easy_setopt(curl, CURLOPT_HTTPAUTH, CURLAUTH_NONE);
> +	curl_easy_setopt(ctx->curl, CURLOPT_USERPWD, NULL);
> +	curl_easy_setopt(ctx->curl, CURLOPT_HTTPAUTH, CURLAUTH_NONE);
>   	return 0;
>   }
>   
> @@ -176,20 +230,20 @@ static int ocierofs_curl_perform(struct CURL *curl, long *http_code_out)
>   	return 0;
>   }
>   
> -static int ocierofs_request_perform(struct erofs_oci *oci,
> -				    struct erofs_oci_request *req,
> -				    struct erofs_oci_response *resp)
> +static int ocierofs_request_perform(struct ocierofs_ctx *ctx,
> +				   

...

>   
> @@ -204,15 +258,10 @@ static int ocierofs_request_perform(struct erofs_oci *oci,
>    * @realm_out: pointer to store realm value
>    * @service_out: pointer to store service value
>    * @scope_out: pointer to store scope value
> - *
> - * Parse Bearer authentication header and extract realm, service, and scope
> - * parameters for subsequent token requests.
> - *
> - * Return: 0 on success, negative errno on failure

Why do we drop this comment?

>    */
>   static int ocierofs_parse_auth_header(const char *auth_header,
> -				      char **realm_out, char **service_out,
> -				      char **scope_out)
> +			      char **realm_out, char **service_out,
> +			      char **scope_out)

Let's not adjust lines which are unrelated to the commit.

>   {
>   	char *realm = NULL, *service = NULL, *scope = NULL;
>   	static const char * const param_names[] = {"realm=", "service=", "scope="};
> @@ -221,7 +270,6 @@ static int ocierofs_parse_auth_header(const char *auth_header,
>   	const char *p;
>   	int i, ret = 0;
>   
> -	// https://datatracker.ietf.org/doc/html/rfc6750#section-3

Same here.

>   	if (strncmp(auth_header, "Bearer ", strlen("Bearer ")))
>   		return -EINVAL;
>   
> @@ -229,7 +277,6 @@ static int ocierofs_parse_auth_header(const char *auth_header,
>   	if (!header_copy)
>   		return -ENOMEM;
>   
> -	/* Clean up header: replace newlines with spaces and remove double spaces */

Same here.

If they are unrelated to the new feature, let's refine
ocierofs_parse_auth_header() later.

>   	for (char *q = header_copy; *q; q++) {
>   		if (*q == '\n' || *q == '\r')
>   			*q = ' ';
> @@ -277,22 +324,9 @@ out:
>   	return ret;
>   }
>   
> -/**
> - * ocierofs_extract_www_auth_info - Extract WWW-Authenticate header information
> - * @resp_data: HTTP response data containing headers
> - * @realm_out: pointer to store realm value (optional)
> - * @service_out: pointer to store service value (optional)
> - * @scope_out: pointer to store scope value (optional)
> - *
> - * Extract realm, service, and scope from WWW-Authenticate header in HTTP response.
> - * This function handles the common pattern of parsing WWW-Authenticate headers
> - * that appears in multiple places in the OCI authentication flow.
> - *
> - * Return: 0 on success, negative errno on failure
> - */

Same here.

>   static int ocierofs_extract_www_auth_info(const char *resp_data,
> -					  char **realm_out, char **service_out,
> -					  char **scope_out)
> +				  char **realm_out, char **service_out,
> +				  char **scope_out)

Same here.

>   {>   	char *www_auth;
>   	char *line_end;
> @@ -336,29 +370,12 @@ static int ocierofs_extract_www_auth_info(const char *resp_data,
>   	return ret;
>   }
>   
> -/**
> - * ocierofs_get_auth_token_with_url - Get authentication token from auth server
> - * @oci: OCI client structure
> - * @auth_url: authentication server URL
> - * @service: service name for authentication
> - * @repository: repository name
> - * @username: username for basic auth (optional)
> - * @password: password for basic auth (optional)
> - *
> - * Request authentication token from the specified auth server URL using
> - * basic authentication if credentials are provided.
> - *
> - * Return: authentication header string on success, ERR_PTR on failure
> - */
> -static char *ocierofs_get_auth_token_with_url(struct erofs_oci *oci,
> -					      const char *auth_url,
> -					      const char *service,
> -					      const char *repository,
> -					      const char *username,
> -					      const char *password)
> +static char *ocierofs_get_auth_token_with_url(struct ocierofs_ctx *ctx, const char *auth_url,
> +					      const char *service, const char *repository,
> +					      const char *username, const char *password)

Since the arguments are changed, I'm fine with this change.

>   {
> -	struct erofs_oci_request req = {};
> -	struct erofs_oci_response resp = {};
> +	struct ocierofs_request req = {};
> +	struct ocierofs_response resp = {};
>   	json_object *root, *token_obj, *access_token_obj;
>   	const char *token;
>   	char *auth_header = NULL;
> @@ -373,40 +390,36 @@ static char *ocierofs_get_auth_token_with_url(struct erofs_oci *oci,
>   	}
>   
>   	if (username && password && *username) {
> -		ret = ocierofs_curl_setup_basic_auth(oci->curl, username,
> -						     password);
> +		ret = ocierofs_curl_setup_basic_auth(ctx->curl, username,
> +					     password);

Password can be aligned with `(` in the previous line?

>   		if (ret)
>   			goto out_url;
>   	}
>   
> -	ret = ocierofs_request_perform(oci, &req, &resp);
> -	ocierofs_curl_clear_auth(oci->curl);
> +	ret = ocierofs_request_perform(ctx, &req, &resp);
> +	ocierofs_curl_clear_auth(ctx);
>   	if (ret)
>   		goto out_url;
>   
>   	if (!resp.data) {
> -		erofs_err("empty response from auth server");

Why drop this?

>   		ret = -EINVAL;
>   		goto out_url;
>   	}
>   
>   	root = json_tokener_parse(resp.data);
>   	if (!root) {
> -		erofs_err("failed to parse auth response");

Why drop this?

>   		ret = -EINVAL;
> -		goto out_url;
> +		goto out_json;
>   	}
>   
>   	if (!json_object_object_get_ex(root, "token", &token_obj) &&
>   	    !json_object_object_get_ex(root, "access_token", &access_token_obj)) {
> -		erofs_err("no token found in auth response");

Why drop this?

>   		ret = -EINVAL;
>   		goto out_json;
>   	}
>   
>   	token = json_object_get_string(token_obj ? token_obj : access_token_obj);
>   	if (!token) {
> -		erofs_err("invalid token in auth response");

Why drop this?


...

>   		auth_service = discovered_service ? discovered_service : service;
> -		auth_header = ocierofs_get_auth_token_with_url(oci, discovered_auth_url,
> -							       auth_service, repository,
> -							       username, password);
> +		auth_header = ocierofs_get_auth_token_with_url(ctx, discovered_auth_url,
> +				       auth_service, repository,
> +				       username, password);

Same here.

>   		free(discovered_auth_url);
>   		free(discovered_service);
>   		if (!IS_ERR(auth_header))
> @@ -544,9 +555,9 @@ static char *ocierofs_get_auth_token(struct erofs_oci *oci, const char *registry
>   		if (asprintf(&auth_url, auth_patterns[i], registry) < 0)
>   			continue;
>   
> -		auth_header = ocierofs_get_auth_token_with_url(oci, auth_url,
> -							       service, repository,
> -							       username, password);
> +		auth_header = ocierofs_get_auth_token_with_url(ctx, auth_url,
> +				       service, repository,
> +				       username, password);

Same here.

>   		free(auth_url);
>   
>   		if (!IS_ERR(auth_header))
> @@ -557,24 +568,21 @@ static char *ocierofs_get_auth_token(struct erofs_oci *oci, const char *registry
>   	return ERR_PTR(-ENOENT);
>   }
>   
> -static char *ocierofs_get_manifest_digest(struct erofs_oci *oci,
> -					  const char *registry,
> -					  const char *repository, const char *tag,
> -					  const char *platform,
> -					  const char *auth_header)
> +static char *ocierofs_get_manifest_digest(struct ocierofs_ctx *ctx,
> +				  const char *registry,
> +				  const char *repository, const char *tag,
> +				  const char *platform,
> +				  const char *auth_header)
>   {
> -	struct erofs_oci_request req = {};
> -	struct erofs_oci_response resp = {};
> +	struct ocierofs_request req = {};
> +	struct ocierofs_response resp = {};
>   	json_object *root, *manifests, *manifest, *platform_obj, *arch_obj;
>   	json_object *os_obj, *digest_obj, *schema_obj, *media_type_obj;
>   	char *digest = NULL;
>   	const char *api_registry;
>   	int ret = 0, len, i;
>   
> -	if (!registry || !repository || !tag || !platform)
> -		return ERR_PTR(-EINVAL);
> -
> -	api_registry = (!strcmp(registry, DOCKER_REGISTRY)) ? DOCKER_API_REGISTRY : registry;
> +	api_registry = ocierofs_get_api_registry(registry);
>   	if (asprintf(&req.url, "https://%s/v2/%s/manifests/%s",
>   	     api_registry, repository, tag) < 0)
>   		return ERR_PTR(-ENOMEM);
> @@ -584,22 +592,20 @@ 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);
> +	ret = ocierofs_request_perform(ctx, &req, &resp);
>   	if (ret)
>   		goto out;
>   
>   	if (!resp.data) {
> -		erofs_err("empty response from manifest request");

Same here.

>   		ret = -EINVAL;
>   		goto out;
>   	}
>   
>   	root = json_tokener_parse(resp.data);
>   	if (!root) {
> -		erofs_err("failed to parse manifest JSON");

Same here.

>   		ret = -EINVAL;
>   		goto out;
>   	}
> @@ -615,8 +621,7 @@ static char *ocierofs_get_manifest_digest(struct erofs_oci *oci,
>   	if (json_object_object_get_ex(root, "mediaType", &media_type_obj)) {
>   		const char *media_type = json_object_get_string(media_type_obj);
>   
> -		if (!strcmp(media_type, DOCKER_MEDIATYPE_MANIFEST_V2) ||
> -		    !strcmp(media_type, OCI_MEDIATYPE_MANIFEST)) {
> +		if (ocierofs_is_manifest(media_type)) {
>   			digest = strdup(tag);
>   			ret = 0;
>   			goto out_json;
> @@ -624,7 +629,6 @@ static char *ocierofs_get_manifest_digest(struct erofs_oci *oci,
>   	}
>   
>   	if (!json_object_object_get_ex(root, "manifests", &manifests)) {
> -		erofs_err("no manifests found in manifest list");

Same here.

>   		ret = -EINVAL;
>   		goto out_json;
>   	}
> @@ -634,9 +638,9 @@ static char *ocierofs_get_manifest_digest(struct erofs_oci *oci,
>   		manifest = json_object_array_get_idx(manifests, i);
>   
>   		if (json_object_object_get_ex(manifest, "platform",
> -					      &platform_obj) &&
> +				      &platform_obj) &&

No need to change.

>   		    json_object_object_get_ex(platform_obj, "architecture",
> -					      &arch_obj) &&
> +				      &arch_obj) &&

No need to change.


...
> -						}
> +					free(oci_cfg->password);
> +					oci_cfg->password = strdup(p);
> +					if (!oci_cfg->password)
> +						return -ENOMEM;
>   					} else {
> -						erofs_err("invalid --oci value %s", opt);
> +						erofs_err("mkfs: invalid --oci value %s", opt);

Unneeded `mkfs:`.


Thanks,
Gao Xiang