Re: [PATCH 3/6] chmem: add support for dynamic (de)configuration of hotplug memory

On Thu, Oct 16, 2025 at 12:16:50PM +0200, Sumanth Korikkar wrote:
> +
> +static int chmem_set_memmap_on_memory(struct chmem_desc *desc, char *name)
> +{
> +	char str[BUFSIZ];
> +	int rc, index;
> +
> +	index = strtou64_or_err(name + 6, _("Failed to parse index"));
> +	idxtostr(desc, index, str, sizeof(str));
> +	rc = ul_path_writef_u64(desc->sysmemconfig, desc->memmap_on_memory,
> +				"%s/memmap_on_memory", name);
> +	if (rc)
> +		warn(_("%s memmap-on-memory failed"), str);
> +	return rc;

It seems that str[BUFSIZ] and idxtostr() are necessary only in case of
an error. How about:

        index = strtou64_or_err(name + 6, _("Failed to parse index"));
        rc = ul_path_writef_u64(desc->sysmemconfig, desc->memmap_on_memory,
            "%s/memmap_on_memory", name);

        if (rc) {
            char str[BUFSIZ];
            idxtostr(desc, index, str, sizeof(str));
            warn(_("%s memmap-on-memory failed"), str);
        }

Note that BUFSIZ seems excessive.

> +static int chmem_config(struct chmem_desc *desc, char *name, int configure)
> +{
> +	int mblock_configured, memmap, rc, index;
> +	char str[BUFSIZ], state[BUFSIZ];
> +
> +	index = strtou64_or_err(name + 6, _("Failed to parse index"));
> +	idxtostr(desc, index, str, sizeof(str));
> +	rc = ul_path_readf_s32(desc->sysmemconfig, &mblock_configured, "%s/config", name);
> +	if (rc)
> +		goto out;
> +	rc = ul_path_readf_s32(desc->sysmemconfig, &memmap, "%s/memmap_on_memory", name);
> +	if (rc)
> +		goto out;
> +	if (mblock_configured) {
> +		if (configure) {
> +			if (chmem_memmap_on_memory_option_enabled(desc) &&

The name is quite long. How about renaming the function to
chmem_memmap_enabled()?

> +					memmap != desc->memmap_on_memory) {
> +				if (!desc->is_size || desc->verbose)
> +					fprintf(stdout,
> +						_("%s must be deconfigured before using -m option\n"), str);
> +				rc = -1;
> +			} else if (desc->is_size) {
> +				/*
> +				 * Allow chmem_onoff_size() to proceed with
> +				 * configuring different memory blocks when the
> +				 * current block is already configured.
> +				 */
> +				rc = -1;
> +			} else if (desc->verbose) {
> +				fprintf(stdout, _("%s already configured\n"), str);
> +			}
> +			goto out;
> +		} else if (ul_path_readf_buffer(desc->sysmem, state,
> +						sizeof(state), "%s/state", name) > 0 &&
> +			   strncmp("online", state, 6) == 0) {
> +			if (!desc->is_size || desc->verbose)
> +				fprintf(stdout, "%s must be offline before deconfiguration\n", str);

Here, the _( ) is missing for the message.

> +		if (!desc->is_size) {
> +			if (configure)
> +				warn(_("%s configure failed\n"), str);
> +			else
> +				warn(_("%s deconfigure failed\n"), str);

It's a detail, but I would suggest a different code pattern:

   warn(configure ? _("%s configure failed") :
                    _("%s deconfigure failed"), str);

 Note that warn() (err(), ...) appends \n to the output, so do not use
 it in the string.

    Karel

-- 
 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com