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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.03.16 at 19:45, <dario.faggioli@xxxxxxxxxx> wrote:
> 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'.

This and everything else below look to be valid comments, but
it's rather frustrating that simply cloning an existing function (I
user the debug key ones as basis) doesn't give me valid code,
the more that I did scroll up and down a few pages to see
whether I just happened to pick a particularly bad example.
(This adds to the reasons why I've continue to push out getting
the tool stack side done for a patch the hypervisor side of which
has been done a couple of months back.)

Jan

>> +    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)




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