Re: [PATCH v8 09/12] xen: add runtime parameter access support to hypfs

[Jürgen Groß <jgross@xxxxxxxx>]

On 14.05.20 17:02, Jan Beulich wrote:
> On 14.05.2020 16:56, Jürgen Groß wrote:
> > On 14.05.20 14:10, Jan Beulich wrote:
> > > On 14.05.2020 13:48, Jürgen Groß wrote:
> > > > On 14.05.20 12:20, Jan Beulich wrote:
> > > > > On 08.05.2020 17:34, Juergen Gross wrote:
> > > > > > --- a/xen/common/grant_table.c
> > > > > > +++ b/xen/common/grant_table.c
> > > > > > @@ -85,8 +85,43 @@ struct grant_table {
> > > > > >         struct grant_table_arch arch;
> > > > > >     };
> > > > > >     -static int parse_gnttab_limit(const char *param, const char *arg,
> > > > > > -                              unsigned int *valp)
> > > > > > +unsigned int __read_mostly opt_max_grant_frames = 64;
> > > > > > +static unsigned int __read_mostly opt_max_maptrack_frames = 1024;
> > > > > > +
> > > > > > +#ifdef CONFIG_HYPFS
> > > > > > +#define GRANT_CUSTOM_VAL_SZ  12
> > > > > > +static char __read_mostly opt_max_grant_frames_val[GRANT_CUSTOM_VAL_SZ];
> > > > > > +static char __read_mostly opt_max_maptrack_frames_val[GRANT_CUSTOM_VAL_SZ];
> > > > > > +
> > > > > > +static void update_gnttab_par(struct param_hypfs *par, unsigned int val,
> > > > > > +                              char *parval)
> > > > > > +{
> > > > > > +    snprintf(parval, GRANT_CUSTOM_VAL_SZ, "%u", val);
> > > > > > +    custom_runtime_set_var_sz(par, parval, GRANT_CUSTOM_VAL_SZ);
> > > > > > +}
> > > > > > +
> > > > > > +static void __init gnttab_max_frames_init(struct param_hypfs *par)
> > > > > > +{
> > > > > > +    update_gnttab_par(par, opt_max_grant_frames, opt_max_grant_frames_val);
> > > > > > +}
> > > > > > +
> > > > > > +static void __init max_maptrack_frames_init(struct param_hypfs *par)
> > > > > > +{
> > > > > > +    update_gnttab_par(par, opt_max_maptrack_frames,
> > > > > > +                      opt_max_maptrack_frames_val);
> > > > > > +}
> > > > > > +#else
> > > > > > +#define opt_max_grant_frames_val    NULL
> > > > > > +#define opt_max_maptrack_frames_val NULL
> > > > > This looks latently dangerous to me (in case new uses of these
> > > > > two identifiers appeared), but I guess my alternative suggestion
> > > > > will be at best controversial, too:
> > > > >
> > > > > #define update_gnttab_par(par, val, unused) update_gnttab_par(par, val)
> > > > > #define parse_gnttab_limit(par, arg, valp, unused) parse_gnttab_limit(par, arg, 
> > > > > valp)
> > > > >
> > > > > (placed right here)
> > > > Who else would use those identifiers not related to hypfs?
> > > I can't see an obvious possible use, but people get creative, i.e.
> > > you never know. Passing NULL into a function without it being
> > > blindingly obvious that it won't ever get (de)referenced is an at
> > > least theoretical risk imo.
> > Hmm, what about using a special type for those content variables?
> > Something like:
> >
> > #ifdef CONFIG_HYPFS
> > #define hypfs_string_var            char *
> > #else
> > #define hypfs_string_var            char
> > #define opt_max_grant_frames_val    0
> > #define opt_max_maptrack_frames_val 0
> > #endif
> >
> > And then use that as type for function parameters? This should make
> > dereferencing pretty hard.
> >
> > Other than that I have no really good idea how to avoid this problem.
> IOW (as suspected) you don't like my suggestion? Personally I think
> yours, having more #define-s, is at least not better than mine.
Oh, maybe I misunderstood you here. I thought you weren't happy with
your solution either and suspected others (not me) wouldn't like it.

In case others don't reject it I'd be happy to use your variant.


Juergen