Re: [PATCH v2 3/3] xl: new "loglvl" command

[Wei Liu <wei.liu2@citrix.com>]

On Mon, Mar 14, 2016 at 09:23:35AM -0600, Jan Beulich wrote:
> >>> On 08.03.16 at 17:20, <wei.liu2@citrix.com> wrote:
> > On Fri, Mar 04, 2016 at 09:48:16AM -0700, Jan Beulich wrote:
> >> This is pretty simplistic for now, but I'd rather have someone better
> >> friends with the tools improve it (if desired).
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> 
> >> --- a/tools/libxl/libxl.c
> >> +++ b/tools/libxl/libxl.c
> >> @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx
> >>      return 0;
> >>  }
> >>  
> >> +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest,
> >> +                    int *lower_thresh, int *upper_thresh)
> >> +{
> >> +    int ret;
> >> +    GC_INIT(ctx);
> >> +    if (set) {
> >> +        ret = xc_set_log_level(ctx->xch, guest, *lower_thresh, *upper_thresh);
> >> +    } else {
> >> +        ret = xc_get_log_level(ctx->xch, guest, lower_thresh, upper_thresh);
> >> +    }
> >> +    if ( ret < 0 ) {
> >> +        LOGE(ERROR, "%s %slog level",
> >> +             set ? "setting" : "getting", guest ? "guest " : "");
> >> +        GC_FREE;
> >> +        return ERROR_FAIL;
> >> +    }
> >> +    GC_FREE;
> >> +    return 0;
> >> +}
> >> +
> > 
> > As Dario said, libxl tends to have getter and setter pair.
> 
> "Tends to have" still leaves room for me to get away without
> adjustments. Could you please clearly state whether splitting
> this is required for acceptance?
> 

Yes, please make a pair of getter and setter.

> >> --- a/tools/libxl/xl_cmdimpl.c
> >> +++ b/tools/libxl/xl_cmdimpl.c
> >> @@ -6469,6 +6469,84 @@ int main_debug_keys(int argc, char **arg
> >>      return 0;
> >>  }
> >>  
> >> +static const struct {
> >> +    int level;
> >> +    char string[8];
> >> +} loglvls[] = {
> >> +    { 0, "none" },
> >> +    { 1, "error" },
> >> +    { 2, "warning" },
> >> +    { 3, "info" },
> >> +    { 4, "all" },
> >> +    { 4, "debug" },
> > 
> > The semantics of the numbers should go into libxl_types.idl. Maybe
> > something like
> > 
> > # Keep in sync with Xen log level.
> > libxl_xen_log_level = Enumeration (...);
> > 
> > Then in here
> > 
> >     static const struct {
> >         int level;
> >         char string[8];
> >     } loglvls[] = {
> >         { LIBXL_XEN_LOG_LEVEL_NONE, "none" },
> >         { ..., "error" },
> >         { ..., "warning" },
> >         { ..., "info" },
> >         { ..., "all" },
> >         { ..., "debug" },
> > 
> > Please also note that after the introduction of this API, Xen log level
> > will become part of the stable API and subject to some compatibility
> > constraints. Is this what you wanted?
> 
> No, and I actually I'm having trouble following your request: This
> lives in xl_cmdimpl.c, which I didn't think is subject to any stability
> requirements in the mapping of strings (from the xl command line)
> to numbers. It is _specifically_ that I do not want to fix those
> mappings.
> 

The new API libxl_log_level relies on the semantics of these mappings.
To make the new API useful to all clients, the semantics of log levels
needs to go into libxl_types.idl (as mentioned a few lines above), hence
it becomes part of the stable API. Otherwise you end up with an API
accepting arbitrary magic numbers.

Wei.

> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel