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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2016-03-04 at 09:48 -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@xxxxxxxx>
> 
> --- 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;
>
As per libxl coding style, this wants to be 'r'.

> +    GC_INIT(ctx);
>
I don't seem to find it in CODING_STYLE, but I'd say there should be an
empty line here.

> +    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;
>
Libxl wants only one error/cleanup path out of the function, and
recommends using a variable called rc for hosting the libxl error code
to be returned, and goto, if necessary.

> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c

> +int main_loglvl(int argc, char **argv)
> +{
> +    static const struct option opts[] = {
> +        {"guest", 0, 0, 'g'},
> +        {"set", 0, 0, 's'},
> +        COMMON_LONG_OPTS
> +    };
> +    int opt, lower_thresh = -1, upper_thresh = -1;
> +    bool guest = false, set = false;
> +
> +    SWITCH_FOREACH_OPT(opt, "gs:", opts, "loglvl", 0) {
> +    case 'g':
> +        guest = true;
> +        break;
> +    case 's':
> +        if (*optarg != '/')
> +            lower_thresh = parse_loglvl(&optarg);
> +        if (*optarg == '/') {
> +            ++optarg;
> +            upper_thresh = parse_loglvl(&optarg);
> +        }
> +        set = true;
> +        break;
> +    }
> +
> +    if (libxl_log_level(ctx, set, guest, &lower_thresh,
> &upper_thresh)) {
> +        fprintf(stderr, "cannot %s %s log level\n",
> +                set ? "set" : "get", guest ? "guest" : "host");
> +        return 1;
>
This is indeed super-inconsistent in xl. But we're trying to improve it
 (it's half done and there are patches) and using EXIT_FAILURE and
EXIT_SUCCESS for program exit codes, and this return can be classified
as such.

> +    }
> +
> +    if (!set)
> +        printf("%s log levels: %s/%s\n", guest ? "guest" : "host",
> +               format_loglvl(lower_thresh),
> format_loglvl(upper_thresh));
> +
> +    return 0;
>
And this as well, of course. :-)

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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