[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDB
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDBus |
Date: |
Fri, 5 Jan 2018 13:06:40 -0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 |
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