Messages in this thread | | | Date | Tue, 15 Mar 2022 16:17:40 +0530 | Subject | RE: [PATCH v2] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled | From | 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[unhandled content-type:application/octet-stream] | |