Re: [Xen-devel] [PATCH v2] libxl: add "xl qemu-monitor-command"

[Juergen Gross <jgross@xxxxxxxx>]

On 06/09/16 10:31, Dario Faggioli wrote:
> On Mon, 2016-09-05 at 16:11 +0200, Juergen Gross wrote:
>> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
>> @@ -1103,14 +1103,48 @@ int libxl__qmp_x_blockdev_change(libxl__gc
>> *gc, int domid, const char *parent,
>>      return qmp_run_command(gc, domid, "x-blockdev-change", args,
>> NULL, NULL);
>>  }
>>  
>> -int libxl__qmp_hmp(libxl__gc *gc, int domid, const char
>> *command_line)
>> +static int hmp_callback(libxl__qmp_handler *qmp,
>> +                        const libxl__json_object *response,
>> +                        void *opaque)
>> +{
>> +    char **output = opaque;
>> +    GC_INIT(qmp->ctx);
>> +    int rc = 0;
>> +
> I'm probably being a bit picky, but libxl's CODING_STYLE says rc should
> not be initialized.

Okay.

> 
>> +    if (!output)
>> +        goto out;
>> +
>> +    *output = NULL;
>> +
>> +    if (libxl__json_object_is_string(response)) {
>> +        *output = libxl__strdup(gc,
>> libxl__json_object_get_string(response));
>> +        goto out;
>> +    }
>> +
>> +    LOG(ERROR, "Response has unexpected format");
>> +    rc = ERROR_FAIL;
>> +
>> +out:
>> +    return rc;
>> +}
>> +
> Also, aren't we missing calling GC_FREE on the out path?

Yes. And libxl__strdup should use NOGC.

> 
>> +int libxl__qmp_hmp(libxl__gc *gc, int domid, const char
>> *command_line,
>> +                   char **output)
>>  {
>>      libxl__json_object *args = NULL;
>>  
>>      qmp_parameters_add_string(gc, &args, "command-line",
>> command_line);
>>  
>>      return qmp_run_command(gc, domid, "human-monitor-command", args,
>> -                           NULL, NULL);
>> +                           hmp_callback, output);
>> +}
>> +
>> +int libxl_qemu_monitor_command(libxl_ctx *ctx, uint32_t domid,
>> +                               const char *command_line, char
>> **output)
>> +{
>> +    GC_INIT(ctx);
>> +
>> +    return libxl__qmp_hmp(gc, domid, command_line, output);
>>
>  GC_FREE; again ?

Yep.

> 
>>  }
>>
> 
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> @@ -9536,6 +9536,33 @@ int main_psr_hwinfo(int argc, char **argv)
>>  
>>  #endif
>>  
>> +int main_qemu_monitor_command(int argc, char **argv)
>> +{
>> +    int opt;
>> +    uint32_t domid;
>> +    char *cmd;
>> +    char *output;
>> +    int ret;
>> +
>> +    SWITCH_FOREACH_OPT(opt, "", NULL, "qemu-monitor-command", 2) {
>> +        /* No options */
>> +    }
>> +
>> +    domid = find_domain(argv[optind]);
>> +    cmd = argv[optind + 1];
>> +
>> +    if (argc - optind > 2) {
>> +        fprintf(stderr, "Invalid arguments.\n");
>> +        return 1;
>> +    }
>> +
>> +    ret = libxl_qemu_monitor_command(ctx, domid, cmd, &output);
>> +    if (!ret && output)
>> +        printf("%s\n", output);
>> +
>> +    return ret;
>>
> Things are not fully consistent yet, but there's an ongoing effort for
> having xl terminate with either EXIT_SUCCESS or EXIT_FAILURE. Can you
> use them when exit()-ing or, like in this case, returning from a
> main_foo() function?

Sure.


Thanks,

Juergen

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