Skip to content

lkl: hijack: move dbg_handler out of liblkl #585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 27, 2025

Conversation

ddiss
Copy link

@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
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 <[email protected]>
dbg_entrance() is only called via the SIGTSTP signal handler setup in
lkl_register_dbg_handler().

Signed-off-by: David Disseldorp <[email protected]>
Copy link
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?

@ddiss
Copy link
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
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
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
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