Re: [Embedded-pv-devel] [PATCH v11] xen: add para-virtual sound interface header file

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.11.16 at 09:03, <andr2000@xxxxxxxxx> wrote:
> +#ifndef __XEN_PUBLIC_IO_XENSND_H__
> +#define __XEN_PUBLIC_IO_XENSND_H__
> +
> +#include <xen/interface/io/ring.h>
> +#include <xen/interface/grant_table.h>

Along with the target tree (and hence path) change, these also want
to become ""-style #include-s.

> +struct xensnd_open_req {
> +    uint32_t pcm_rate; /* in Hz */
> +    uint8_t pcm_format;
> +    uint8_t pcm_channels;
> +    uint16_t __reserved0;

No double underscores please at the beginning of these names;
preferably none at all (as field names may collide with file scope
object-like macros).

> +struct xensnd_page_directory {
> +    grant_ref_t gref_dir_next_page;
> +    uint32_t num_grefs;
> +    grant_ref_t gref[0];

This is not allowed in C89, and I think not even in newer version (i.e.
it's a GNU extension). There are a couple of places where we already
deal with similar constructs, so please take those into consideration
when dealing with the problem here.

> +struct xensnd_close_req {
> +    /* place holder, remove if changing the structure (C89 concern) */
> +    uint8_t __placeholder;
> +};

The comment ought to be correct - did you verify this is permitted
in e.g. C99? I don't think it is, and I've referred to C89 only to give
you an understanding with what standard the header as a whole
is expected to comply (i.e. to avoid you using C99 constructs
elsewhere).

> +union xensnd_req {
> +    struct {
> +        uint16_t id;
> +        uint8_t operation;
> +        uint8_t stream_idx;
> +        uint32_t padding;
> +        union {
> +            struct xensnd_open_req open;
> +                struct xensnd_close_req close;
> +                struct xensnd_write_req write;
> +                struct xensnd_read_req read;
> +                struct xensnd_get_vol_req get_vol;
> +                struct xensnd_set_vol_req set_vol;
> +                struct xensnd_mute_req mute;
> +                struct xensnd_unmute_req unmute;
> +            } op;
> +        } data;
> +    uint8_t raw[64];
> +};

Hmm, you've indeed changed the way you indent, but there are
more style issues here: The inner union's members aren't all
indented equally, and (perhaps as a result) the closing braces
are indented too much.

> +union xensnd_resp {
> +    struct {
> +        uint16_t id;
> +        uint8_t operation;
> +        uint8_t stream_idx;
> +        int8_t status;
> +    } data;

This lacks explicit tail padding.

Jan


_______________________________________________
Embedded-pv-devel mailing list
Embedded-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/embedded-pv-devel