Re: [PATCH 2/5] libxl: drop dead assignments to "ret" from libxl__domain_config_setdefault()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.06.2023 11:57, Daniel P. Smith wrote:
> On 6/13/23 05:40, Jan Beulich wrote:
>> On 13.06.2023 11:21, Daniel P. Smith wrote:
>>> On 6/13/23 02:33, Jan Beulich wrote:
>>>> On 12.06.2023 22:21, Daniel P. Smith wrote:
>>>>> On 6/12/23 15:44, Daniel P. Smith wrote:
>>>>>> On 6/12/23 07:46, Jan Beulich wrote:
>>>>>>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls
>>>>>>> if the 1st yielded ENOSYS?
>>>>>>
>>>>>> Would you be okay with the calls staying if instead on the first
>>>>>> invocation of any libxl_flask_* method, flask status was checked and
>>>>>> stored in a variable that would then be checked by any subsequent calls
>>>>>> and immediately returned if flask was not enabled?
>>>>
>>>> I'm wary of global variables in shared libraries.
>>>>
>>>
>>> I agree with that sentiment, but I would distinguish global state from a
>>> global variable. I would take the approach of,
>>>
>>> static boot is_flask_enabled(void)
>>> {
>>>       /* 0 unchecked, 1 checked but disabled, 2 enabled */
>>>       static int state = 0;
>>>
>>>       if ( state == 0 )
>>>     state = check_flask_state(); /* pseudo call for real logic */
>>>
>>>       return (state < 2 ? false : true);
>>> }
>>>
>>> Then all the libxl_flask_* methods would is_flask_enabled(). This would
>>> not expose a global variable that could be manipulated by users of the
>>> library.
>>
>> Well, I certainly did assume the variable wouldn't be widely exposed.
>> And the library also clearly is far from free of r/w data. But still.
>>
>> That aspect aside - wouldn't such a basic state determination better
>> belong in libxc then? (There's far less contents in .data / .bss
>> there.)
> 
> Not opposed at all, so a series with a patch to libxc paired and a new 
> sub-op/sysctl as discussed below would be acceptable?

I can only say yes here for the hypervisor side; I'm not a maintainer
of any significant part of the tool stack.

>>>> Since in the specific case here it looks like the intention really is
>>>> to carry out Flask-specific operations, a means to check for Flask
>>>> specifically might indeed be appropriate. If not a new sub-op of
>>>> xsm_op, a new sysctl might be another option.
>>>
>>> I am actually split on whether this should be an sub-op of xsm op or if
>>> this should be exposed via hyperfs. I did not consider a sysctl, though
>>> I guess an argument could be constructed for it.
>>
>> Please don't forget that hypfs, unlike sysctl, is an optional thing,
>> so you can't use it for decision taking (but only for informational
>> purposes).
> 
> Good point regarding hypfs, the check mentioned above is expected to 
> always work, thus an optional feature probably is not best.
> 
> The next question is, should it be sysctl or xsm-op. I am leaning 
> towards xsm-op because as much as I believe XSM should be consider core 
> to Xen, the XSM logic should remain contained. Adding it to sysctl would 
> mean having to expose XSM state outside of XSM code and would make 
> sysctl logic have to be aware of XSM state query functions.

Well, you seemed hesitant towards an xsm-sub-op, so I suggested sysctl
as a possible alternative. One possible issue with an xsm-op is that at
the public interface level (public headers), besides __HYPERVISOR_xsm_op
there's no notion of xsm-op. And it was pointed out before that by kind
of blindly wiring xsm_op through to flask_op, adding XSM-module-
independent ops and/or ops for some future non-Flask module is going to
be a little, well, bumpy.

Jan