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

[Jan Beulich <jbeulich@xxxxxxxx>]

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