Skip to content

lkl: hijack: move dbg_handler out of liblkl#585

Merged
thehajime merged 2 commits intolkl:masterfrom
ddiss:lkl_hijack_dbg
Mar 27, 2025
Merged

lkl: hijack: move dbg_handler out of liblkl#585
thehajime merged 2 commits intolkl:masterfrom
ddiss:lkl_hijack_dbg

Conversation

@ddiss
Copy link
Copy Markdown

@ddiss ddiss commented Mar 24, 2025

dbg_handler exposes a very useful debug shell, but it's currently only used (within tools/lkl at least) by hijack.
Move the functionality into liblkl-hijack, to slightly trim down the liblkl core library.
This may break external lkl_register_dbg_handler() callers.

@ddiss ddiss marked this pull request as draft March 24, 2025 10:57
@ddiss
Copy link
Copy Markdown
Author

ddiss commented Mar 24, 2025

flagging as draft as it's an API breakage... if we want to retain a signal-based debug hook then maybe it'd make sense to add a callback parameter instead, e.g. lkl_register_dbg_handler_fn(<callback function>, <fn_param>).

ddiss added 2 commits March 25, 2025 10:09
dbg_handler exposes a very useful debug shell, but it's currently only
used (within tools/lkl at least) by hijack and zpoline.
Move the functionality into liblkl-hijack, to slightly trim down the
liblkl core library.
This may break external lkl_register_dbg_handler() callers.

Signed-off-by: David Disseldorp <ddiss@suse.de>
dbg_entrance() is only called via the SIGTSTP signal handler setup in
lkl_register_dbg_handler().

Signed-off-by: David Disseldorp <ddiss@suse.de>
Copy link
Copy Markdown
Member

@tavip tavip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I am not too concerned with this API change. @thehajime any thoughts?

Comment thread tools/lkl/lib/hijack/Build
@ddiss
Copy link
Copy Markdown
Author

ddiss commented Mar 25, 2025

v2:

  • fix zpoline linker error
  • move dbg.c source in with dbg_handler.c
    • the unmodified source causes checkpatch errors

@ddiss ddiss marked this pull request as ready for review March 25, 2025 07:56
@thehajime
Copy link
Copy Markdown
Member

@ddiss @tavip
thanks for the patch. overall looks good to me.

I just wish to know the motivation part of this patch; do we have an impact on trimming down the size of core library ?

Move the functionality into liblkl-hijack, to slightly trim down the liblkl core library.

I was wondering what motivates you to prepare this patch, as this introduces the API breakage (as you mentioned), we may not know the user of our library (liblkl.so) who uses this feature.

@ddiss
Copy link
Copy Markdown
Author

ddiss commented Mar 26, 2025

thanks for the patch. overall looks good to me.

Thanks for the review.

I just wish to know the motivation part of this patch; do we have an impact on trimming down the size of core library ?

Move the functionality into liblkl-hijack, to slightly trim down the liblkl core library.

I was wondering what motivates you to prepare this patch, as this introduces the API breakage (as you mentioned), we may not know the user of our library (liblkl.so) who uses this feature.

I don't have a strong motivation for this one. Aside from the small size reduction, I was mostly considering ongoing maintenance and mainlining, where a simpler API should help.

@thehajime
Copy link
Copy Markdown
Member

I don't have a strong motivation for this one. Aside from the small size reduction, I was mostly considering ongoing maintenance and mainlining, where a simpler API should help.

thanks for the description. I'm fine to merge this. We can revert (or refactor) it if somebody tells it.

@thehajime thehajime merged commit cab6e39 into lkl:master Mar 27, 2025
12 of 14 checks passed
@ddiss ddiss deleted the lkl_hijack_dbg branch April 2, 2025 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants