[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1] xen: move getdomaininfo() to domain.c
On 25.07.2025 03:21, Stefano Stabellini wrote: > On Thu, 24 Jul 2025, Jan Beulich wrote: >> On 23.07.2025 22:30, Stefano Stabellini wrote: >>> On Wed, 23 Jul 2025, Jan Beulich wrote: >>>> On 23.07.2025 02:46, Stefano Stabellini wrote: >>>>> On Tue, 22 Jul 2025, Jan Beulich wrote: >>>>>> On 22.07.2025 07:04, Penny Zheng wrote: >>>>>>> Function getdomaininfo() is not only invoked by domctl-op, but also >>>>>>> sysctl-op, >>>>>>> so it shall better live in domain.c, rather than domctl.c. Which is also >>>>>>> applied for arch_get_domain_info(). Style corrections shall be applied >>>>>>> at >>>>>>> the same time while moving these functions, such as converting u64 to >>>>>>> uint64_t. >>>>>>> >>>>>>> The movement could also fix CI error of a randconfig picking both >>>>>>> SYSCTL=y >>>>>>> and PV_SHIM_EXCLUSIVE=y results in sysctl.c being built, but domctl.c >>>>>>> not >>>>>>> being built, which leaves getdomaininfo() undefined, causing linking to >>>>>>> fail. >>>>>>> >>>>>>> Fixes: 34317c508294 ("xen/sysctl: wrap around sysctl hypercall") >>>>>>> Reported-by: Jan Beulich <jbeulich@xxxxxxxx> >>>>>>> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx> >>>>>> >>>>>> I'm not convinced of this approach. In the longer run this would mean >>>>>> wrapping >>>>>> everything you move in "#if defined(CONFIG_SYSCTL) || >>>>>> defined(CONFIG_DOMCTL)", >>>>>> which I consider undesirable. Without DOMCTL, the usefulness of >>>>>> XEN_SYSCTL_getdomaininfolist is at least questionable. Therefore adding >>>>>> more >>>>>> isolated "#ifdef CONFIG_DOMCTL" just there may be an option. Similarly, >>>>>> as >>>>>> mentioned on the other thread, having SYSCTL depend on DOMCTL is an >>>>>> approach >>>>>> which imo wants at least considering. And there surely are further >>>>>> options. >>>>>> >>>>>> As indicated elsewhere, my preference goes towards reverting the final >>>>>> one or >>>>>> two patches of that series. They can be re-applied once the dependencies >>>>>> were >>>>>> properly sorted, which may (as per above) involve properly introducing a >>>>>> DOMCTL Kconfig setting first. >>>>> >>>>> I don't think this is a good idea. >>>> >>>> And implicitly you say that what I put under question in the first >>>> paragraph >>>> is a good way forward? >>> >>> I think it is OK. >>> >>> I also think "having SYSCTL depend on DOMCTL" is certainly worth >>> thinking about. In terms of privilege and potential for interference >>> with other domains sysctl and domctl don't seem different so it is >>> unlikely one would want to disable one but not the other. >>> >>> Another idea is to have a single kconfig for both SYSCTL and DOMCTL: we >>> don't necessarily need to offer individual kconfig for every feature. >>> From a safety point of view, we want to disable them both. >> >> Then again (and going against the thought of making SYSCTL depend on DOMCTL) >> there may be a desire to query / alter certain properties of the system as >> a whole, without also having that need for individual domains. But yes, >> covering both with a single control also is an option to consider. > > If making SYSCTL depend on DOMCTL and/or a single kconfig for both > SYSCTL and DOMCTL are both way forward, then we can take this patch as > is? In both of the named cases this patch simply wouldn't be needed. Once the conversion work was done, that is. And to be frank, I'm not happy to see the function move out and then back in. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |