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

[Philippe Mathieu-Daudé]

Hi Eric,

On 01/05/2018 12:29 PM, Eric Blake wrote:
> 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.

Ok.

What about using 'x-qtest-sdbus-command'?

> 
>>
>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>> ---
> 
>> +##
>> +# @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.

Indeed you right.

> 
>> +#
>> +# Since: 2.11
> 
> 2.12

Oops :)

> 
>> +##
>> +{ '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

Yes, you right, I wonder how I ended using hex here (I don't have any in
my .qmp_history ...)

> 
>> +#      }
>> +#    }
>> +# <- { "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.

old habits are hard to change :)

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

Good to know!

> 
>> +    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, ...

Ok.

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

Oh, I didn't know.

Thanks for your detailed review!

Phil.

signature.asc
Description: OpenPGP digital signature