Re: [PATCH ipsec-next v5 7/8] xfrm: add error messages to state migration
From: Sabrina Dubroca <sd@queasysnail.net>
Date: 2026-01-30 12:14:43
Also in:
lkml
2026-01-27, 11:43:42 +0100, Antony Antony wrote:
quoted hunk ↗ jump to hunk
Add descriptive(extack) error messages for all error paths in state migration. This improves diagnostics by providing clear feedback when migration fails. Signed-off-by: Antony Antony <redacted> --- v4->v5: - added this patch --- net/xfrm/xfrm_state.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index 88a362e46972..2e03871ae872 100644 --- a/net/xfrm/xfrm_state.c +++ b/net/xfrm/xfrm_state.c@@ -2129,15 +2129,21 @@ struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x, struct xfrm_state *xc; xc = xfrm_state_clone_and_setup(x, encap, m); - if (!xc) + if (!xc) { + NL_SET_ERR_MSG(extack, "Failed to clone and setup state");
When xfrm_state_clone_and_setup fails it's because some allocation failed and the user won't be able to do much about this, right? I don't feel extack in those situations is super helpful.
return NULL;
+ }
- if (xfrm_init_state(xc) < 0)
+ if (xfrm_init_state(xc) < 0) {
+ NL_SET_ERR_MSG(extack, "Failed to initialize migrated state");xfrm_init_state itself doesn't handle extack, but it's just a wrapper around functions that do. Maybe better to make xfrm_init_state propagate extack?
goto error;
+ }
/* configure the hardware if offload is requested */
- if (xuo && xfrm_dev_state_add(net, xc, xuo, extack))
+ if (xuo && xfrm_dev_state_add(net, xc, xuo, extack)) {
+ NL_SET_ERR_MSG(extack, "Failed to initialize state offload");We already set an extack in xfrm_dev_state_add, this chunk should be dropped to avoid overwriting the more specific info we got.
quoted hunk ↗ jump to hunk
goto error; + } return xc; error:@@ -2161,6 +2167,7 @@ int xfrm_state_migrate_install(const struct xfrm_state *x, xfrm_state_insert(xc); } else { if (xfrm_state_add(xc) < 0) { + NL_SET_ERR_MSG(extack, "Failed to add migrated state");
Not a strong objection, but this case would be the EEXIST situation from xfrm_state_add, and there's not much the user can do about this?
if (xuo) xfrm_dev_state_delete(xc); xc->km.state = XFRM_STATE_DEAD;
-- Sabrina