Re: [PATCH v3 5/7] xsm: decouple xsm header inclusion selection

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 8/30/21 9:46 AM, Jan Beulich wrote:
> On 30.08.2021 15:41, Daniel P. Smith wrote:
>> On 8/30/21 9:24 AM, Jan Beulich wrote:
>>> On 27.08.2021 16:06, Daniel P. Smith wrote:
>>>> On 8/26/21 4:13 AM, Jan Beulich wrote:
>>>>> On 05.08.2021 16:06, Daniel P. Smith wrote:
>>>>>> --- /dev/null
>>>>>> +++ b/xen/include/xsm/xsm-core.h
>>>>>> @@ -0,0 +1,273 @@
>>>>>> +/*
>>>>>> + *  This file contains the XSM hook definitions for Xen.
>>>>>> + *
>>>>>> + *  This work is based on the LSM implementation in Linux 2.6.13.4.
>>>>>> + *
>>>>>> + *  Author:  George Coker, <gscoker@xxxxxxxxxxxxxx>
>>>>>> + *
>>>>>> + *  Contributors: Michael LeMay, <mdlemay@xxxxxxxxxxxxxx>
>>>>>> + *
>>>>>> + *  This program is free software; you can redistribute it and/or modify
>>>>>> + *  it under the terms of the GNU General Public License version 2,
>>>>>> + *  as published by the Free Software Foundation.
>>>>>> + */
>>>>>> +
>>>>>> +#ifndef __XSM_CORE_H__
>>>>>> +#define __XSM_CORE_H__
>>>>>> +
>>>>>> +#include <xen/sched.h>
>>>>>> +#include <xen/multiboot.h>
>>>>>
>>>>> I was going to ask to invert the order (as we try to arrange #include-s
>>>>> alphabetically), but it looks like multiboot.h isn't fit for this.
>>>>
>>>> So my understanding is to leave this as is.
>>>
>>> Yes, unfortunately.
>>>
>>>>>> +typedef void xsm_op_t;
>>>>>> +DEFINE_XEN_GUEST_HANDLE(xsm_op_t);
>>>>>
>>>>> Just FTR - I consider this dubious. If void is meant, I don't see why
>>>>> a void handle can't be used.
>>>>
>>>> Unless I am misunderstanding what you are calling for, I am afraid this
>>>> will trickle further that what intended to be addressed in this patch
>>>> set. If disagree and would like to provide me a suggest that stays
>>>> bounded, I would gladly incorporate.
>>>
>>> All I'm asking is to remove this pointless typedef and handle definition,
>>> seeing that you're doing a major rework anyway. I'm afraid I don't see
>>> how this would collide with the purpose of the overall series (albeit I
>>> may also have misunderstood your reply, as the 2nd half of the first
>>> sentence makes me struggle some with trying to parse it).
>>
>> If I drop the typedef and start changing everywhere xsm_op_t is
>> referenced to void, this now adds hypercall.h to the files I am now
>> touching.
>>
>> In the end it is not about whether the change is big or small, but that
>> more and more unrelated small changes/clean ups keep getting requested.
>> There has to be a cut-off point to limit the scope of changes down to
>> the purpose of the patch set, which is to unravel and simplify the XSM
>> hooks. And this is being done so, so that the the XSM-Roles work can be
>> introduced to bring a more solid definition to the the default access
>> control system, which itself is needed to bring in hyperlaunch.
> 
> Well, yes, you effectively suffer from XSM not having been actively
> maintained for a number of years. As said in the original reply, I'd
> prefer my ack to cover all the suggested changes, but I didn't mean
> to insist. If this particular one goes too far for your taste, so be
> it.
> 
> Jan
> 

I think we can agree that XSM has not been receiving appropriate care
and maintenance. It can't all be fixed in one go and I am trying to step
up to get it back into shape. Since this is really undoing a masking of
void, I can add this and the risk is very low of breaking something
else. Which this is my biggest concern as I see new files getting
brought in with each cleanup requested.

v/r,
dps