Hi Adrian,
On Mon, Jun 5, 2017 at 8:13 PM, Adrian Hunter <adrian.hun...@intel.com> wrote:
> On 03/06/17 05:00, Namhyung Kim wrote:
>> If a kernel modules is compressed, it should be decompressed before
>> running objdump to parse binary data correctly. This fixes a failure of
>> object code reading test for me.
>
> Makes sense to me although I have a few small comments below.
Thanks!
>
>>
>> Signed-off-by: Namhyung Kim <namhy...@kernel.org>
>> ---
>> tools/perf/tests/code-reading.c | 26 +++++++++++++++++++++++++-
>> 1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/tests/code-reading.c
>> b/tools/perf/tests/code-reading.c
>> index 1f14e7612cbb..fe6bd34d68b1 100644
>> --- a/tools/perf/tests/code-reading.c
>> +++ b/tools/perf/tests/code-reading.c
>> @@ -229,6 +229,7 @@ static int read_object_code(u64 addr, size_t len, u8
>> cpumode,
>> unsigned char buf2[BUFSZ];
>> size_t ret_len;
>> u64 objdump_addr;
>> + const char *objdump_name;
>> int ret;
>>
>> pr_debug("Reading object code for memory address: %#"PRIx64"\n", addr);
>> @@ -289,9 +290,32 @@ static int read_object_code(u64 addr, size_t len, u8
>> cpumode,
>> state->done[state->done_cnt++] = al.map->start;
>> }
>>
>> + objdump_name = al.map->dso->long_name;
>> + if (dso__needs_decompress(al.map->dso)) {
>> + char buf[32]; /* for "/tmp/perf-kmod-XXXXXX" */
>
> We should be able to set the temporary file name buffer size without
> reference to the format of the name i.e. #define it or use PATH_MAX
OK, will define KMOD_DECOMP_LEN then..
>
>> +
>> + if (dso__decompress_kmodule_path(al.map->dso, objdump_name,
>> + buf, sizeof(buf)) < 0) {
>
> Do we care that it is kmodule i.e. why isn't this function called just
> dso__decompress_path().
Well, I'm not sure loading a compressed DSO is making sense.
Decompressing is needed for kernel modules which supports loading
compressed modules.
>
>> + pr_debug("decompression failed\n");
>> + return -1;
>> + }
>> +
>> + objdump_name = strdup(buf);
>
> Why not just put the buf in the outer scope instead of using strdup().
Will change.
Thanks
Namhyung
>
>> + if (objdump_name == NULL) {
>> + pr_debug("memory allocation failed\n");
>> + return -1;
>> + }
>> + }
>> +
>> /* Read the object code using objdump */
>> objdump_addr = map__rip_2objdump(al.map, al.addr);
>> - ret = read_via_objdump(al.map->dso->long_name, objdump_addr, buf2,
>> len);
>> + ret = read_via_objdump(objdump_name, objdump_addr, buf2, len);
>> +
>> + if (objdump_name != al.map->dso->long_name) {
>> + unlink(objdump_name);
>> + free((void *)objdump_name);
>> + }
>> +
>> if (ret > 0) {
>> /*
>> * The kernel maps are inaccurate - assume objdump is right in
>>
>
