include/uk/event: Make handler and raise return values type-safe #1687

michpappas · 2025-08-08T16:40:38Z

uk_event handlers often omit using one of the designated macro values, and return an integer instead. This is particularly problematic when returning zero to signify that the event was handled, as event.h assigns zero to UK_EVENT_NOT_HANDLED.

Convert uk_event handler return values to enum to catch such errors at compile-time. Introduce a UK_EVENT_ERROR state and an additional parameter in the handler prototype that handlers must populate before returning UK_EVENT_ERROR.

Prerequisite checklist

Read the contribution guidelines regarding submitting new changes to the project;
Tested your changes against relevant architectures and platforms;
Ran the checkpatch.uk on your commit series before opening this PR;
Updated relevant documentation.

Base target

Architecture(s): N/A
Platform(s): N/A
Application(s): N/A

Additional configuration

Description of changes

mogasergiu

LGTM, just a couple of missed type updated I think

include/uk/event.h

lib/ukdebug/arch/arm64/gdbsup.c

lib/ukdebug/arch/x86_64/gdbsup.c

lib/uknofault/excpttab.h

uk_event handlers often omit using one of the designated macro values, and return an integer instead. This is particularly problematic when returning zero to signify that the event was handled, as event.h assigns zero to UK_EVENT_NOT_HANDLED. Convert uk_event handler return values to enum to catch such errors at compile-time. Introduce a UK_EVENT_ERROR state and an additional parameter in the handler prototype that handlers must populate before returning UK_EVENT_ERROR. Checkpatch-ignore: MACRO_ARG_REUSE Signed-off-by: Michalis Pappas <[email protected]>

Update event handlers and event raising calls to use the new enum return type and error parameter. Signed-off-by: Michalis Pappas <[email protected]>

Update event handlers and event raising calls to use the new enum return type and error parameter. Checkpatch-Ignore: LONG_LINE Signed-off-by: Michalis Pappas <[email protected]>

Update event handlers and event raising calls to use the new enum return type and error parameter. Signed-off-by: Michalis Pappas <[email protected]>

Update event handlers and event raising calls to use the new enum return type and error parameter. Checkpatch-ignore: MACRO_ARG_REUSE Checkpatch-ignore: MACRO_ARG_PRECEDENCE Checkpatch-ignore: UNNECESSARY_PARENTHESES Signed-off-by: Michalis Pappas <[email protected]>

mogasergiu

For me it feels viscerally wrong whenever I end up having to add functions with lateral effect and can't help but wonder if there is a better way. But, yeah, for now I can't see a better way than this that would fit all requirements either...
So I guess we can stick with this or... maybe leave it for later and brainstorm some more. I would approve either way.

mogasergiu · 2025-08-14T07:38:00Z

include/uk/event.h

- *       else if (!rc)
- *               uk_pr_err("myevent not handled!\n");
+ *       rc = uk_raise_event(myevent, &myevent_data, &error);
+ *       if (unlikely(rc < UK_EVENT_ERROR))


I would drop the unlikely from the documentation and leave as it was. Also the type of rc might need to be updated.

mogasergiu · 2025-08-14T07:38:42Z

include/uk/event.h

 {
 	const uk_event_handler_t *itr;
-	int rc;
 	int ret = UK_EVENT_NOT_HANDLED;


I think ret should have been enum?

mogasergiu · 2025-08-14T07:39:48Z

include/uk/event.h

 			return UK_EVENT_HANDLED;

-		if (rc == UK_EVENT_HANDLED_CONT)
+		if (ret == UK_EVENT_HANDLED_CONT)


I think this if statement is no longer necessary

mogasergiu · 2025-08-14T07:44:15Z

include/uk/event.h

+#define uk_raise_event(event, data, error)				\
 	({	_UK_EVT_IMPORT_EVENT(event);				\
-		uk_raise_event_ptr(UK_EVENT_PTR(event), data);	})
+		uk_raise_event_ptr(UK_EVENT_PTR(event), data, error);	})


Maybe we should enclose data and error between paranthesis? Also don't forget to now add the error argument documentation to uk_raise_event as well.

mogasergiu · 2025-08-14T07:46:33Z

lib/posix-process/execve.c

-int pprocess_raise_execve_event(struct posix_process_execve_event_data *data)
+enum uk_event_status
+pprocess_raise_execve_event(struct posix_process_execve_event_data *event_data,
+			    int event_error)


I think you meant int *event_error? (You do have it in the header definition :D)

mogasergiu · 2025-08-14T07:49:30Z

lib/ukboot/shutdown_req.c

 #if __INTERRUPTSAFE__ && CONFIG_LIBUKBOOT_SHUTDOWNREQ_HANDLER
-static int shutdown_req_handler(void *data)
+static
+enum uk_event_status shutdown_req_handler(void *data, int *error __unused)


I think you can update error to return value of rc.

mogasergiu · 2025-08-14T08:04:21Z

lib/ukvmem/arch/arm/pagefault64.c

+#include <uk/print.h>

-static int vmem_arch_pagefault(void *data)
+static enum uk_event_status vmem_arch_pagefault(void *data, int *error __unused)


Ditto (for x86 too)

github-project-automation bot added this to Unikraft Core Roadmap Aug 8, 2025

github-project-automation bot moved this to 🧊 Icebox in Unikraft Core Roadmap Aug 8, 2025

github-actions bot added arch/arm arch/arm64 area/include Part of include/uk area/lib Internal Unikraft Microlibrary area/plat Unikraft Patform lang/c Issues or PRs to do with C/C++ lib/syscall_shim lib/ukboot lib/ukdebug plat/common Common to all platforms labels Aug 8, 2025

michpappas requested review from andreittr and mogasergiu August 8, 2025 16:40

michpappas assigned mogasergiu Aug 8, 2025

mogasergiu requested changes Aug 11, 2025

View reviewed changes

include/uk/event.h Outdated Show resolved Hide resolved

lib/ukdebug/arch/arm64/gdbsup.c Outdated Show resolved Hide resolved

lib/ukdebug/arch/x86_64/gdbsup.c Outdated Show resolved Hide resolved

lib/uknofault/excpttab.h Outdated Show resolved Hide resolved

michpappas force-pushed the michpappas/ukevent_return_enum branch from 472282d to 6f779b3 Compare August 12, 2025 12:23

github-actions bot added arch/x86_64 lib/posix-process Information about system parameters plat/xen Unikraft for Xen labels Aug 12, 2025

michpappas changed the title ~~include/uk/event: Convert handler return values to enum~~ include/uk/event: Make handler and raise return values type-safe Aug 12, 2025

michpappas added 9 commits August 12, 2025 14:29

plat/common: Adapt to strongly typed uk_event_status API

53b812e

Update event handlers and event raising calls to use the new enum return type and error parameter. Signed-off-by: Michalis Pappas <[email protected]>

plat/xen: Adapt to strongly typed uk_event_status API

56d324f

Update event handlers and event raising calls to use the new enum return type and error parameter. Signed-off-by: Michalis Pappas <[email protected]>

lib/ukintctlr: Adapt to strongly typed uk_event_status API

e6fdd86

Update event handlers and event raising calls to use the new enum return type and error parameter. Signed-off-by: Michalis Pappas <[email protected]>

lib/ukdebug: Adapt to strongly typed uk_event_status API

dbf086c

Update event handlers and event raising calls to use the new enum return type and error parameter. Signed-off-by: Michalis Pappas <[email protected]>

lib/ukboot: Adapt to strongly typed uk_event_status API

3ee5188

Update event handlers and event raising calls to use the new enum return type and error parameter. Signed-off-by: Michalis Pappas <[email protected]>

lib/ukvmem: Adapt to strongly typed uk_event_status API

6cf5513

Update event handlers and event raising calls to use the new enum return type and error parameter. Signed-off-by: Michalis Pappas <[email protected]>

lib/uknofault: Adapt to strongly typed uk_event_status API

9a1617a

Update event handlers and event raising calls to use the new enum return type and error parameter. Checkpatch-Ignore: LONG_LINE Signed-off-by: Michalis Pappas <[email protected]>

lib/ukstore: Adapt to strongly typed uk_event API

12671ab

Update event handlers and event raising calls to use the new enum return type and error parameter. Signed-off-by: Michalis Pappas <[email protected]>

michpappas added 5 commits August 12, 2025 14:29

lib/syscall-shim: Adapt to strongly typed uk_event_status API

771ab47

Update event handlers and event raising calls to use the new enum return type and error parameter. Signed-off-by: Michalis Pappas <[email protected]>

drivers/ukps2/kbd: Adapt to strongly typed uk_event_status API

623ba2a

Update event handlers and event raising calls to use the new enum return type and error parameter. Signed-off-by: Michalis Pappas <[email protected]>

lib/posix-process: Adapt to strongly typed uk_event_status API

0145341

Update event handlers and event raising calls to use the new enum return type and error parameter. Signed-off-by: Michalis Pappas <[email protected]>

lib/posix-fdtab: Adapt to strongly typed uk_event_status API

a55ebb9

Update event handlers and event raising calls to use the new enum return type and error parameter. Signed-off-by: Michalis Pappas <[email protected]>

michpappas force-pushed the michpappas/ukevent_return_enum branch from 6f779b3 to b37f342 Compare August 12, 2025 12:32

michpappas requested a review from mogasergiu August 12, 2025 12:42

mogasergiu requested changes Aug 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

include/uk/event: Make handler and raise return values type-safe #1687

include/uk/event: Make handler and raise return values type-safe #1687

Uh oh!

michpappas commented Aug 8, 2025 •

edited

Loading

Uh oh!

mogasergiu left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mogasergiu left a comment

Uh oh!

mogasergiu Aug 14, 2025

Uh oh!

mogasergiu Aug 14, 2025

Uh oh!

mogasergiu Aug 14, 2025

Uh oh!

mogasergiu Aug 14, 2025

Uh oh!

mogasergiu Aug 14, 2025

Uh oh!

mogasergiu Aug 14, 2025

Uh oh!

mogasergiu Aug 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

include/uk/event: Make handler and raise return values type-safe #1687

Are you sure you want to change the base?

include/uk/event: Make handler and raise return values type-safe #1687

Uh oh!

Conversation

michpappas commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisite checklist

Base target

Additional configuration

Description of changes

Uh oh!

mogasergiu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mogasergiu left a comment

Choose a reason for hiding this comment

Uh oh!

mogasergiu Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

mogasergiu Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

mogasergiu Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

mogasergiu Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

mogasergiu Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

mogasergiu Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

mogasergiu Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

michpappas commented Aug 8, 2025 •

edited

Loading