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

On Thu, Oct 16, 2025 at 01:39:51PM +0200, Karel Zak wrote:
> 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.

Noted. I will use the above convention.

> > +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()?

Agree.

> > +					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.

Will add.

> > +		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.

Sure. I will integrate it in next version. Thanks.