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

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

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?
> > > > 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.
Erg, you are right, not sure why it didn't click. Though I guess what 
should be used will be moot if the decision is to add an xsm-op subop to 
dummy to support reporting the current XSM module.
> > > 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.
With that said, if someone wants to make the case for sysctl, I am open 
to considering it.
v/r,
dps