-
Notifications
You must be signed in to change notification settings - Fork 359
linux: fix regression in libcrun_configure_network #1829
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
base: main
Are you sure you want to change the base?
linux: fix regression in libcrun_configure_network #1829
Conversation
Reviewer's GuideThis patch corrects a regression in libcrun_configure_network by replacing manual parsing of netlink error messages with use of the standard nlmsgerr structure and properly distinguishing between error and acknowledgment responses. Sequence diagram for revised netlink error handling in libcrun_configure_networksequenceDiagram
participant libcrun_configure_network
participant kernel
libcrun_configure_network->>kernel: send netlink request
kernel-->>libcrun_configure_network: receive netlink response
alt NLMSG_ERROR
libcrun_configure_network->>libcrun_configure_network: parse nlmsgerr from response
alt err_data->error < 0
libcrun_configure_network->>libcrun_configure_network: call crun_make_error
else err_data->error == 0
libcrun_configure_network->>libcrun_configure_network: treat as success (ack)
end
end
Class diagram for updated error handling in libcrun_configure_networkclassDiagram
class libcrun_configure_network {
+int libcrun_configure_network(libcrun_container_t *container, libcrun_error_t *err)
}
class nlmsghdr {
+int nlmsg_type
}
class nlmsgerr {
+int error
}
libcrun_configure_network --> nlmsghdr : uses
libcrun_configure_network --> nlmsgerr : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @eriksjolund - I've reviewed your changes - here's some feedback:
- Before casting NLMSG_DATA to struct nlmsgerr, verify that the netlink message payload length is at least sizeof(struct nlmsgerr) to avoid potential invalid memory access.
- Make the zero-error acknowledgment path (err_data->error == 0) explicit—either return success immediately or clearly continue—so the intent is obvious and we don’t fall through accidentally.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Before casting NLMSG_DATA to struct nlmsgerr, verify that the netlink message payload length is at least sizeof(struct nlmsgerr) to avoid potential invalid memory access.
- Make the zero-error acknowledgment path (err_data->error == 0) explicit—either return success immediately or clearly continue—so the intent is obvious and we don’t fall through accidentally.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Please take an extra look when doing the review. I haven't used netlink before. |
6d5b033
to
eabc488
Compare
commit ab64a5c introduced the regression. Closes: containers#1828 Signed-off-by: Erik Sjölund <[email protected]>
eabc488
to
d30d151
Compare
commit ab64a5c introduced the regression.
Closes: #1828
Summary by Sourcery
Fix regression in libcrun_configure_network by properly handling netlink NLMSG_ERROR messages using struct nlmsgerr and ignoring success acknowledgements.
Bug Fixes: