[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/2] node_memory: Do not fail if there is parameter unsupported



> ping
> 
> On 2012年11月22日 11:14, Osier Yang wrote:
> > It makes no sense to fail the whole command if there is parameter
> > unsupported by the kernel. This patch fixes it by ignoring the
> > unsupported parameter for getMemoryParameters, and ignoring the
> > unsupported parameter for setMemoryParameters too if there are
> > more than one parameters to set, otherwise error out.

I'm not sure I agree with these semantics.


> >    *
> > - * Get all node memory parameters.  On input, @nparams gives the
> > size
> > - * of the @params array; on output, @nparams gives how many slots
> > were
> > - * filled with parameter information, which might be less but will
> > - * not exceed the input value.
> > + * Get all node memory parameters (parameter unsupported by OS
> > will be
> > + * ignored).  On input, @nparams gives the size of the @params

s/ignored/omitted/

But is this really right, or shouldn't we at least be able to
fake things?  If the OS doesn't support tuning a variable, then
we should return a hard-coded value for the default setting
of that variable (in relation to newer kernels that DO support
tuning), rather than omitting it altogether.

> > array;
> > + * on output, @nparams gives how many slots were filled with
> > parameter
> > + * information, which might be less but will not exceed the input
> > value.
> >    *
> >    * As a special case, calling with @params as NULL and @nparams
> >    as 0 on
> >    * input will cause @nparams on output to contain the number of
> >    parameters
> > @@ -6811,7 +6811,9 @@ error:
> >    *           value nparams of virDomainGetSchedulerType)
> >    * @flags: extra flags; not used yet, so callers should always
> >    pass 0
> >    *
> > - * Change all or a subset of the node memory tunables.
> > + * Change all or a subset of the node memory tunables. Tunable
> > + * unsupported by OS wll be ignored if @nparams is not 1,

w/wll/will/

I disagree.  I think we should continue to error out on unrecognized
tunables; furthermore, we should error out up front (if the user
passes an array of 3 elements, where only the 2nd element is
unsupported, then we should NOT modify the first element; that is,
we should verify that all the elements are settable before making
any modification).  It is a disservice to the user to ignore their
request for a change.

And if you go with my approach that getting parameters should
return a hard-coded value rather than omitting unsupported
tunables, then the user should be allowed to pass the default
value of that tunable; the only thing they cannot do is
pass a non-default value.



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]