On 05.12.2025 21:36, Milan Djokic wrote:
> Handling of unsupported sysctl commands currently diverges: some commands
> return -EOPNOTSUPP, while others fall back to generic -ENOSYS.
>
> Unify the behavior so that unsupported commands consistently return the
> appropriate error code, allowing the control domain to correctly identify
> unsupported operations.
>
> Using IS_ENABLED() macro to identify disabled commands, allowing
> dead code to be removed at compile time.
>
> Signed-off-by: Milan Djokic <[email protected]>
Overall quite okay imo, yet there are a number of small issues.
> @@ -170,19 +173,22 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t)
> u_sysctl)
> op->u.availheap.avail_bytes <<= PAGE_SHIFT;
> break;
>
> -#ifdef CONFIG_PM_STATS
> case XEN_SYSCTL_get_pmstat:
> - ret = do_get_pm_info(&op->u.get_pmstat);
> + if ( !IS_ENABLED(CONFIG_PM_STATS) )
> + ret = -EOPNOTSUPP;
> + else
> + ret = do_get_pm_info(&op->u.get_pmstat);
> break;
> -#endif
>
> -#ifdef CONFIG_PM_OP
> case XEN_SYSCTL_pm_op:
> - ret = do_pm_op(&op->u.pm_op);
> - if ( ret == -EAGAIN )
> - copyback = 1;
> + if ( !IS_ENABLED(CONFIG_PM_OP) )
> + ret = -EOPNOTSUPP;
> + else {
Misplaced figure brace.
> --- a/xen/include/xen/perfc.h
> +++ b/xen/include/xen/perfc.h
> @@ -92,7 +92,6 @@ DECLARE_PER_CPU(perfc_t[NUM_PERFCOUNTERS], perfcounters);
> #endif
>
> struct xen_sysctl_perfc_op;
> -int perfc_control(struct xen_sysctl_perfc_op *pc);
>
> extern void cf_check perfc_printall(unsigned char key);
> extern void cf_check perfc_reset(unsigned char key);
> @@ -114,4 +113,7 @@ extern void cf_check perfc_reset(unsigned char key);
>
> #endif /* CONFIG_PERF_COUNTERS */
>
> +struct xen_sysctl_perfc_op;
> +extern int perfc_control(struct xen_sysctl_perfc_op *pc);
Why would you move the function decl, but duplicate the struct forward decl?
> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -7,6 +7,7 @@
>
> #include <asm/system.h>
> #include <asm/spinlock.h>
> +#include <public/sysctl.h>
Why would this be needed? It means effectively the whole codebase gains a
dependency on this header even when DEBUG_LOCK_PROFILE=n. Yes, you may
need ...
> @@ -40,8 +41,6 @@ union lock_debug { };
>
> #ifdef CONFIG_DEBUG_LOCK_PROFILE
>
> -#include <public/sysctl.h>
> -
> /*
> lock profiling on:
>
> @@ -164,7 +163,6 @@ void _lock_profile_deregister_struct(int32_t type,
> #define lock_profile_deregister_struct(type, ptr)
> \
> _lock_profile_deregister_struct(type, &((ptr)->profile_head))
>
> -extern int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc);
> extern void cf_check spinlock_profile_printall(unsigned char key);
> extern void cf_check spinlock_profile_reset(unsigned char key);
>
> @@ -360,4 +358,6 @@ static always_inline void nrspin_lock_irq(rspinlock_t *l)
> #define nrspin_unlock_irqrestore(l, f) _nrspin_unlock_irqrestore(l, f)
> #define nrspin_unlock_irq(l) _nrspin_unlock_irq(l)
>
> +extern int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc);
> +
> #endif /* __SPINLOCK_H__ */
... a forward decl of struct xen_sysctl_lockprof_op; I don't see what's
wrong with doing just that.
Jan
