Messages in this thread | | | Subject | Re: [PATCH v1 4/5] perf mem: Support record for hybrid platform | From | "Jin, Yao" <> | Date | Wed, 26 May 2021 09:51:34 +0800 |
| |
Hi Jiri,
On 5/25/2021 3:39 PM, Jin, Yao wrote: > Hi Jiri, > >>>> rec_argv = calloc(rec_argc + 1, sizeof(char *)); >>>> if (!rec_argv) >>>> return -1; >>>> + /* >>>> + * Save the allocated event name strings. >>>> + */ >>>> + rec_tmp = calloc(rec_argc + 1, sizeof(char *)); >>>> + if (!rec_tmp) { >>>> + free(rec_argv); >>>> + return -1; >>>> + } >>> >>> why not do strdup on all of them and always call free instead? >>> that would get rid of the rec_tmp and tmp_nr >>> >> >> That is also one method. Let me try it. >> > > If we do strdup on all of them, such as, > > if (e->record) > rec_argv[i++] = strdup("-W"); > > rec_argv[i++] = strdup("-d"); > > if (mem->phys_addr) > rec_argv[i++] = strdup("--phys-data"); > .... > > That looks too much strdup used here. So I choose to use a rec_tmp[] to record the allocated string > and free them before exit the function. > > Or we record the start index and end index in rec_argv[] for the allocated event string, use strdup > on them and call free before exit the function. >
This method looks also not OK.
The rec_argv[] is changed in cmd_record() for some complex command lines.
For example,
./perf mem record -- ./memtest -R0d -b2000 -d64 -n100
Before cmd_record(), rec_argv[3] = "-e". After cmd_record(), rec_argv[3] = "-d64"
Even we do strdup on all of rec_argv[], but the entries are probably changed in cmd_record(), so we can't free the entries at the end of __cmd_record().
Maybe we have to use the original way which just records the allocated event string to a temporary array and free the whole array at the end of __cmd_record().
What do you think?
Thanks Jin Yao
> What do you think? > > Thanks > Jin Yao
| |