Re: [Qemu-devel] [RFC PATCH v2 2/4] libqos: add a sdbus API

[Stefan Hajnoczi]

On Wed, Jan 03, 2018 at 06:49:23PM -0300, Philippe Mathieu-Daudé wrote:
> +typedef struct SDBusAdapter SDBusAdapter;
> +struct SDBusAdapter {
> +
> +    ssize_t (*do_command)(SDBusAdapter *adapter, enum NCmd cmd, uint32_t arg,
> +                          uint8_t **response);
> +    void (*write_byte)(SDBusAdapter *adapter, uint8_t value);
> +    uint8_t (*read_byte)(SDBusAdapter *adapter);
> +};

Is it necessary to expose the struct definition?  qmp_sdbus_create()
allocates this struct so the caller cannot embed it anyway and does not
need to know sizeof.

> +
> +ssize_t sdbus_do_cmd(SDBusAdapter *adapter, enum NCmd cmd, uint32_t arg,
> +                     uint8_t **response);
> +ssize_t sdbus_do_acmd(SDBusAdapter *adapter, enum ACmd acmd, uint32_t arg,
> +                      uint16_t address, uint8_t **response);
> +void sdbus_write_byte(SDBusAdapter *adapter, uint8_t value);
> +uint8_t sdbus_read_byte(SDBusAdapter *adapter);
> +
> +SDBusAdapter *qmp_sdbus_create(const char *bus_name);

Does this function belong in another patch?

> +static bool verbose;
> +#define DPRINTF(fmt, ...)                           \
> +    do {                                            \
> +        if (verbose) {                              \

I suggest using if (getenv("V")) directly and removing the verbose
global.  The verbose global is error-prone because it requires that the
caller first initializes it.

> +ssize_t sdbus_do_acmd(SDBusAdapter *adapter, enum ACmd acmd, uint32_t arg,
> +                      uint16_t address, uint8_t **response)
> +{
> +    do_cmd(adapter, 55, address << 16, NULL, false);
> +    // TODO check rv?

Even if there is no actual error handling, g_assert_...() could be used
here.

signature.asc
Description: PGP signature