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

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

On 6/13/23 06:15, Jan Beulich wrote:
> 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.
Understood, thank you.

> > > > > 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.
That is in fact the source of my hesitance. Fixing xsm sub-op from being 
a flask specific, not well documented thing, to a documented general 
interface is on the list of many improvements needed. I believe handling 
this case could be done as an incremental step towards that by 
introducing a common sub-op structure that is used for the "get current 
module" sub-op since it will be new sub-op for FLASK module as well. As 
you mentioned, the next step will be the bumpy one of trying to convert 
the existing FLASK xsm-op sub-ops over to the new common structure. 
Until I try, I won't know for sure if I can do this as a small step. Let 
me see if I can carve out some time and give it a try.
v/r,
dps