RE: [PATCH v2] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled

[Maninder Singh]

Hi,

> > +int sprint_kallsym_common(char *buffer, unsigned long address, int build_id,
> > +			    int backtrace, int symbol)
> > +{
> > +	if (backtrace)
> > +		return __sprint_symbol(buffer, address, -1, 1, build_id);
> 
> > +	else if (symbol)
> > +		return __sprint_symbol(buffer, address, 0, 1, build_id);
> > +	else
> > +		return __sprint_symbol(buffer, address, 0, 0, 0);
> 
> Redundant 'else' in both cases.
>

Ok, will change it

> > +}
> 
> ...
> 
> > +static int sprint_module_info(char *buf, char *end, unsigned long value,
> > +			     const char *fmt, int modbuildid, int backtrace, int symbol)
> 
> fmt is not used.

Yes, did not notice it.(will remove both end and gmt)

> > +{
> > +	struct module *mod;
> > +	unsigned long offset = 0;
> 
> > +	unsigned long base;
> 
> Can it be the same type as core_layout.base? Why not?
>
> > +	char *modname;
> > +	int len;
> > +	const unsigned char *buildid = NULL;
> > +
> > +	if (is_ksym_addr(value))
> > +		return 0;
> > +
> > +	if (backtrace || symbol)
> > +		offset = 1;
> 
> I would expect here to have
> 
> 	else
> 		offset = 0;
> 
> But see below.
> 
> > +	preempt_disable();
> > +	mod = __module_address(value);
> > +	if (mod) {
> > +		modname = mod->name;
> > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
> > +		if (modbuildid)
> > +			buildid = mod->build_id;
> > +#endif
> 
> > +		if (offset) {
> 
> This seems quite confusing because semantically you use offset as a boolean
> flag and offset. Why not add a boolean variable with a clear name?
>

Ok, will add 2 separate variables.

> > +			base = (unsigned long)mod->core_layout.base;
> > +			offset = value - base;
> > +		}
> > +	}
> 
> > +
> 
> Probably you can drop this blank line to group entire critical section,
> or add a blank line above.
> 
> > +	preempt_enable();
> > +	if (!mod)
> > +		return 0;
> > +
> > +	/* address belongs to module */
> > +	if (offset)
> > +		len = sprintf(buf, "0x%p+0x%lx", (void *)base, offset);
> > +	else
> 
> > +		len = sprintf(buf, "0x%lx", (void *)value);
> 
> What this casting is for? Don't you have a compilation warning?

My Bad, earlier I made patch with hashing this value also (%p), but after that
changed it to %lx to have same original behavior in case of %ps, forgot to update final patch
to remove typecast.

> 
> > +	len += fill_name_build_id(buf, modname, modbuildid, buildid, len);
> > +	return len;
> 
> 	return len + ...;
> 
> ?
> 
> > +}

Will modify patch with all changes.

Thanks,
Maninder Singh

Attachment: rcptInfo.txt
Description: Binary data