Re: [Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDBus

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5158 bytes --]

On 01/03/2018 03:49 PM, Philippe Mathieu-Daudé wrote:
> Use Base64 to serialize the binary blobs in JSON.
> So far at most 512 bytes will be transfered, which result

s/transfered/transferred/

> in a 684 bytes payload.
> Since this command is intented for qtesting, it is acceptable.

s/intented/intended/

Might be worth mentioning the actual command name,
x-debug-sdbus-command, in the commit message to make future git log
trawling easier.

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

> +##
> +# @SDBusCommandResponse:
> +#
> +# SD Bus command response.
> +#
> +# @base64: the command response encoded as a Base64 string, if any (optional)

s/ (optional)//, now that the documentation engine automatically takes
care of that.

Even if there is no response, isn't the empty string "" more accurate
than omitting the string altogether?  In other words, I'm not sure why
you made the 'base64' member optional.

> +#
> +# Since: 2.11

2.12

> +##
> +{ 'struct': 'SDBusCommandResponse', 'data': {'*base64': 'str'} }
> +
> +##
> +# @x-debug-sdbus-command:
> +#
> +# Execute a command on a SD Bus return the response (if any).
> +#

Maybe mention that this command is only intended for use during unit
testing (that information is already implicit from the x-debug prefix,
but stating it explicitly doesn't hurt).

> +# @qom-path: the SD Bus path
> +# @command: the SD protocol command to execute in the bus
> +# @arg: a 64-bit command argument (optional)
> +# @crc: the command/argument CRC (optional)
> +#
> +# Returns: the response of the command encoded as a Base64 string
> +#
> +# Since: 2.11

2.12

> +#
> +# -> { "execute": "x-debug-sdbus-command",
> +#      "arguments": { "qom-path": "/machine/unattached/device[32]/sd.0",
> +#                     "command": 0x01

That's invalid JSON (which does not understand hex numbers).  You need
"command": 1

> +#      }
> +#    }
> +# <- { "return": {'base64': 'A='} }
> +#
> +##
> +{ 'command': 'x-debug-sdbus-command',
> +  'data': { 'qom-path': 'str',
> +            'command': 'uint8',
> +            '*arg': 'uint64',
> +            '*crc': 'uint16' },
> +  'returns': 'SDBusCommandResponse'
> +}
> diff --git a/hw/sd/sdbus-qmp.c b/hw/sd/sdbus-qmp.c
> new file mode 100644
> index 0000000000..8c4b6f2aee
> --- /dev/null
> +++ b/hw/sd/sdbus-qmp.c
> @@ -0,0 +1,65 @@
> +/*
> + * SD card bus QMP debugging interface (for QTesting).
> + *
> + * Copyright (c) 2017 ?

The question mark in a copyright line is not right.  I don't know what
you meant to use, but unless you were doing the work on behalf of an
employer, your own name is probably correct.  You could include 2018 now
if you wanted.


> +SDBusCommandResponse *qmp_x_debug_sdbus_command(const char *qom_path,
> +                                                uint8_t command,
> +                                                bool has_arg, uint64_t arg,
> +                                                bool has_crc, uint16_t crc,
> +                                                Error **errp)
> +{
> +    uint8_t response[16 + 1];
> +    SDBusCommandResponse *res;
> +    bool ambiguous = false;
> +    Object *obj;
> +    SDBus *sdbus;
> +    int sz;
> +
> +    obj = object_resolve_path(qom_path, &ambiguous);
> +    if (obj == NULL) {

I don't know if the style 'if (!obj) {' is any more prevalent; but it
doesn't really matter.

> +        if (ambiguous) {
> +            error_setg(errp, "Path '%s' is ambiguous", qom_path);
> +        } else {
> +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                      "Device '%s' not found", qom_path);
> +        }
> +        return NULL;
> +    }
> +    sdbus = (SDBus *)object_dynamic_cast(obj, TYPE_SD_BUS);

Is the cast still necessary, or does object_dynamic_cast() return void*
so that you can omit the cast?

> +    if (sdbus == NULL) {
> +        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> +                  "Device '%s' not a sd-bus", qom_path);
> +        return NULL;
> +    }
> +
> +    res = g_new0(SDBusCommandResponse, 1);
> +    sz = sdbus_do_command(sdbus,
> +                          &(SDRequest){ command, arg, has_crc ? crc : -1 },

It's probably safer to use specific initializer assignments, as in:

&(SDRequest){ .cmd = command, .arg = arg, ...


> +++ b/stubs/qmp_sdbus.c
> @@ -0,0 +1,12 @@
> +#include "qemu/osdep.h"
> +#include "qmp-commands.h"
> +#include "hw/sd/sd.h"
> +
> +SDBusCommandResponse *qmp_x_debug_sdbus_command(const char *qom_path,
> +                                                uint8_t command,
> +                                                bool has_arg, uint64_t arg,
> +                                                bool has_crc, uint16_t crc,
> +                                                Error **errp)
> +{
> +    return NULL;

In addition to returning NULL, the stub should set errp.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]