deploy: Use delete_if_present in can_soft_reboot#3532
deploy: Use delete_if_present in can_soft_reboot#3532cgwalters merged 1 commit intoostreedev:mainfrom
Conversation
|
Hi @ckyrouac. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a crash in ostree_sysroot_deployment_can_soft_reboot by using ostree_kernel_args_delete_if_present. This prevents an assertion failure when the ostree= kernel argument is not present, which can happen in scenarios like a bootc factory reset.
The change is correct and addresses the issue. I've added one suggestion to further improve the robustness of this function by handling other potential failure cases from ostree_kernel_args_delete_if_present more gracefully instead of using g_assert().
| g_assert (ostree_kernel_args_delete_if_present (booted_kargs, "ostree", NULL)); | ||
| g_assert (ostree_kernel_args_delete_if_present (target_kargs, "ostree", NULL)); |
There was a problem hiding this comment.
While ostree_kernel_args_delete_if_present() correctly handles the case where the ostree= argument is missing, it can still fail and return FALSE in other scenarios (e.g., if there are multiple ostree= arguments). Using g_assert() here would cause a crash in such cases.
It would be more robust to check the return value and gracefully return FALSE from ostree_sysroot_deployment_can_soft_reboot(), indicating that a soft reboot is not possible. This avoids a potential crash and makes the function more resilient to unexpected kernel argument configurations. You might also consider adding a g_debug() message to log the failure.
if (!ostree_kernel_args_delete_if_present (booted_kargs, "ostree", NULL))
return FALSE;
if (!ostree_kernel_args_delete_if_present (target_kargs, "ostree", NULL))
return FALSE;|
@ckyrouac can you rebase on main to pickup the latest CI fix ? |
This avoids a dump when trying to delete the ostree= karg if it isn't present. This is an issue with bootc factory reset. Signed-off-by: ckyrouac <ckyrouac@redhat.com>
Head branch was pushed to by a user without write access
62323ad to
7a5214e
Compare
|
rebased |
|
/ok-to-test |
|
fcos-e2e is going to timeout again, but there is almost no logs |
|
the fcos-e2e mostly duplicates the jenkins run |
|
@cgwalters: Overrode contexts on behalf of cgwalters: ci/prow/fcos-e2e DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This avoids a dump when trying to delete the ostree= karg if it isn't present. This is an issue with bootc factory reset.