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

[Jan Beulich <jbeulich@suse.com>]

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.)

>>> Looking closer I realized there is a slight flaw in the logic here. The
>>> first call is accomplished via an xsm_op call and then assumes that
>>> FLASK is the only XSM that has implemented the xsm hook, xsm_op, and
>>> that the result will be an ENOSYS. If someone decides to implement an
>>> xsm_op hook for any of the existing XSM modules or introduces a new XSM
>>> module that has an xsm_op hook, the return likely would not be ENOSYS. I
>>> have often debated if there should be a way to query which XSM module
>>> was loaded for instances just like this. The question is what mechanism
>>> would be best to do so.
>>
>> Well, this is what results from abusing ENOSYS. The default case of a
>> switch() in a handler shouldn't return that value. Only if the entire
>> hypercall is outright unimplemented, returning ENOSYS is appropriate.
>> In fact I wonder whether dummy.h's xsm_do_xsm_op() is validly doing so,
>> when that function also serves as the fallback for XSM modules
>> choosing to not implement any of the op-s (like SILO does).
> 
> I understand your point, but if ENOSYS (Not Implemented) is not correct, 
> what is the appropriate return value for this module does not support 
> this op?

You almost say it by the wording you chose: EOPNOTSUPP.

>> 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).

Jan