Hi Mahesh,
On Tue, 9 Aug 2011 23:12:30 +0530
Mahesh J Salgaonkar <mahesh at linux.vnet.ibm.com> wrote:
> > > >
> > > > If adding the searching method to the blow position and removing the code
> > > > from init_dwarf_info(), I guess it makes the code simple.
> > > >
> > > > @ process_config_file()
> > > > 9402 if (!set_dwarf_debuginfo(config->module_name,
> > > > 9403 NULL, -1)) {
> > > > 9404 ERRMSG("Skipping to next Module section\n");
> > > > 9405 skip_section = 1;
> > > > 9406 free_config(config);
> > > > 9407 continue;
> > > > 9408 }
> > > > 9409 << HERE >>
> > >
> > > This may not be the correct place to call search method. We may end up
> > > calling search method multiple times for same kernel module. I think
> > > moving the search method inside set_dwarf_debuginfo() routine at below
> > > position is a better place:
> > >
> > > @set_dwarf_debuginfo()
> > > ......
> > > ......
> > > if (!strcmp(dwarf_info.module_name, "vmlinux") ||
> > > !strcmp(dwarf_info.module_name, "xen-syms"))
> > > return TRUE;
> > > + << HERE >>
> > > - /* check to see whether module debuginfo is available */
> > > - if (!init_dwarf_info())
> > > - return FALSE;
> > > - else
> > > - clean_dwfl_info();
> > > return TRUE;
> > > }
> > >
> > > And then we can remove search routine from init_dwarf_info(). What do
> > > you think?
> >
> > I think the above change will be good, thanks in advance.
>
> Please find the inline patch below that separates the search method from
> init_dwarf_info() function. Please review it and let know your comments.
Thank you for the patch.
I think this patch is good, and the patch has been magerd into
filter-out-devel branch.
Thanks
Ken'ichi Ohmichi
> makedumpfile: Move debuginfo search to set_dwarf_debuginfo() routine.
>
> From: Mahesh Salgaonkar <mahesh at linux.vnet.ibm.com>
>
> Move the debuginfo search logic out of init_dwarf_info() function to make
> this function more simpler. Now we call search method in set_dwarf_debuginfo
> when switch to different module section.
>
> This patch also fixes a BUG where dwarf_info.module_name becomes a stale
> pointer after free_config() invocation. This patch now uses strdup() to get
> duplicate string and assigns it to dwarf_info.module_name.
>
> Signed-off-by: Mahesh Salgaonkar <mahesh at linux.vnet.ibm.com>
> ---
> makedumpfile.c | 126 ++++++++++++++++++++++++++++++++++++++------------------
> 1 files changed, 86 insertions(+), 40 deletions(-)
>
> diff --git a/makedumpfile.c b/makedumpfile.c
> index 35cbed9..fe8e8b0 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -1186,7 +1186,60 @@ clean_dwfl_info(void)
> }
>
> /*
> - * Intitialize the dwarf info.
> + * Search module debuginfo.
> + * This function searches for module debuginfo in default debuginfo path for
> + * a given module in dwarf_info.module_name.
> + *
> + * On success, dwarf_info.name_debuginfo is set to absolute path of
> + * module debuginfo.
> + */
> +static int
> +search_module_debuginfo(void)
> +{
> + Dwfl *dwfl = NULL;
> + static char *debuginfo_path = DEFAULT_DEBUGINFO_PATH;
> + static const Dwfl_Callbacks callbacks = {
> + .section_address = dwfl_offline_section_address,
> + .find_debuginfo = dwfl_standard_find_debuginfo,
> + .debuginfo_path = &debuginfo_path,
> + };
> +
> + /*
> + * Check if We already have debuginfo file name with us. If yes,
> + * then we don't need to proceed with search method.
> + */
> + if (dwarf_info.name_debuginfo)
> + return TRUE;
> +
> + if ((dwfl = dwfl_begin(&callbacks)) == NULL) {
> + ERRMSG("Can't create a handle for a new dwfl session.\n");
> + return FALSE;
> + }
> +
> + /* Search for module debuginfo file. */
> + if (dwfl_linux_kernel_report_offline(dwfl,
> + info->system_utsname.release,
> + &dwfl_report_module_p)) {
> + ERRMSG("Can't get Module debuginfo for module '%s'\n",
> + dwarf_info.module_name);
> + dwfl_end(dwfl);
> + return FALSE;
> + }
> + dwfl_report_end(dwfl, NULL, NULL);
> + dwfl_getmodules(dwfl, &process_module, NULL, 0);
> +
> + dwfl_end(dwfl);
> + clean_dwfl_info();
> +
> + /* Return success if module debuginfo is found. */
> + if (dwarf_info.name_debuginfo)
> + return TRUE;
> +
> + return FALSE;
> +}
> +
> +/*
> + * Initialize the dwarf info.
> * Linux kernel module debuginfo are of ET_REL (relocatable) type.
> * This function uses dwfl API's to apply relocation before reading the
> * dwarf information from module debuginfo.
> @@ -1198,51 +1251,45 @@ init_dwarf_info(void)
> {
> Dwfl *dwfl = NULL;
> int dwfl_fd = -1;
> - static char *debuginfo_path = DEFAULT_DEBUGINFO_PATH;
> static const Dwfl_Callbacks callbacks = {
> .section_address = dwfl_offline_section_address,
> - .find_debuginfo = dwfl_standard_find_debuginfo,
> - .debuginfo_path = &debuginfo_path,
> };
>
> dwarf_info.elfd = NULL;
> dwarf_info.dwarfd = NULL;
>
> + /*
> + * We already know the absolute path of debuginfo file. Fail if we
> + * still don't have one. Ideally we should never be in this situation.
> + */
> + if (!dwarf_info.name_debuginfo) {
> + ERRMSG("Can't find absolute path to debuginfo file.\n");
> + return FALSE;
> + }
> +
> if ((dwfl = dwfl_begin(&callbacks)) == NULL) {
> ERRMSG("Can't create a handle for a new dwfl session.\n");
> return FALSE;
> }
>
> - if (dwarf_info.name_debuginfo) {
> - /* We have absolute path for debuginfo file, use it directly
> - * instead of searching it through
> - * dwfl_linux_kernel_report_offline() call.
> - *
> - * Open the debuginfo file if it is not already open.
> - */
> - if (dwarf_info.fd_debuginfo < 0)
> - dwarf_info.fd_debuginfo =
> - open(dwarf_info.name_debuginfo, O_RDONLY);
> -
> - dwfl_fd = dup(dwarf_info.fd_debuginfo);
> - if (dwfl_fd < 0) {
> - ERRMSG("Failed to get a duplicate handle for"
> - " debuginfo.\n");
> - goto err_out;
> - }
> - if (dwfl_report_offline(dwfl, dwarf_info.module_name,
> - dwarf_info.name_debuginfo, dwfl_fd) == NULL) {
> - ERRMSG("Failed reading %s: %s\n",
> - dwarf_info.name_debuginfo, dwfl_errmsg (-1));
> - /* dwfl_fd is consumed on success, not on failure */
> - close(dwfl_fd);
> - goto err_out;
> - }
> - } else if (dwfl_linux_kernel_report_offline(dwfl,
> - info->system_utsname.release,
> - &dwfl_report_module_p)) {
> - ERRMSG("Can't get Module debuginfo for module '%s'\n",
> - dwarf_info.module_name);
> + /* Open the debuginfo file if it is not already open. */
> + if (dwarf_info.fd_debuginfo < 0)
> + dwarf_info.fd_debuginfo =
> + open(dwarf_info.name_debuginfo, O_RDONLY);
> +
> + dwfl_fd = dup(dwarf_info.fd_debuginfo);
> + if (dwfl_fd < 0) {
> + ERRMSG("Failed to get a duplicate handle for"
> + " debuginfo.\n");
> + goto err_out;
> + }
> + /* Apply relocations. */
> + if (dwfl_report_offline(dwfl, dwarf_info.module_name,
> + dwarf_info.name_debuginfo, dwfl_fd) == NULL) {
> + ERRMSG("Failed reading %s: %s\n",
> + dwarf_info.name_debuginfo, dwfl_errmsg (-1));
> + /* dwfl_fd is consumed on success, not on failure */
> + close(dwfl_fd);
> goto err_out;
> }
> dwfl_report_end(dwfl, NULL, NULL);
> @@ -2522,20 +2569,19 @@ set_dwarf_debuginfo(char *mod_name, char *name_debuginfo, int fd_debuginfo)
> if (dwarf_info.name_debuginfo)
> free(dwarf_info.name_debuginfo);
> }
> + if (dwarf_info.module_name)
> + free(dwarf_info.module_name);
> +
> dwarf_info.fd_debuginfo = fd_debuginfo;
> dwarf_info.name_debuginfo = name_debuginfo;
> - dwarf_info.module_name = mod_name;
> + dwarf_info.module_name = strdup(mod_name);
>
> if (!strcmp(dwarf_info.module_name, "vmlinux") ||
> !strcmp(dwarf_info.module_name, "xen-syms"))
> return TRUE;
>
> /* check to see whether module debuginfo is available */
> - if (!init_dwarf_info())
> - return FALSE;
> - else
> - clean_dwfl_info();
> - return TRUE;
> + return search_module_debuginfo();
> }
>
> int
>
