Re: [Xen-devel] [PATCH] sndif: add ABI for Para-virtual sound

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.01.15 at 10:02, <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx> wrote:
> Frontend driver registers an virtual sound card
> and sends an PCM streams to the backend driver.

This seems backwards - a frontend driver may attach to a virtual sound
card, but is unlikely to be the entity creating (i.. registering) it.

> Backend driver is an user-space application.

This is irrelevant for the public interface.

> + 
> *****************************************************************************
> + *                            Frontend XenBus Nodes
> + 
> *****************************************************************************
> + *
> + *----------------------- Request Transport Parameters 
> -----------------------
> + *
> + * event-channel
> + *      Values:         <uint32_t>
> + *
> + *      The identifier of the Xen event channel used to signal activity
> + *      in the ring buffer.
> + *
> + * ring-ref
> + *      Values:         <uint32_t>
> + *      Notes:          6

Please don't blindly copy-and-paste from other headers. There's no note
6 anywhere here, and even if it was the numbering should start at 1.

> +/*
> + * PCM FORMATS.
> + */
> +#define SNDIF_PCM_FORMAT_S8             (0)
> +#define SNDIF_PCM_FORMAT_U8             (1)
> +#define SNDIF_PCM_FORMAT_S16_LE         (2)
> +#define SNDIF_PCM_FORMAT_S16_BE         (3)
> +#define SNDIF_PCM_FORMAT_U16_LE         (4)
> +#define SNDIF_PCM_FORMAT_U16_BE         (5)

Pointless parentheses.

> +/* low three bytes */
> +#define SNDIF_PCM_FORMAT_S24_LE         (6)
> +
> +/* low three bytes */
> +#define SNDIF_PCM_FORMAT_S24_BE         (7)
> +
> +/* low three bytes */
> +#define SNDIF_PCM_FORMAT_U24_LE         (8)
> +
> +/* low three bytes */
> +#define SNDIF_PCM_FORMAT_U24_BE         (9)

What are these "low three bytes" comments to tell us? Please if
you add comments, make them meaningful (and as this particular
one appears to apply to three similarly named #define-s, just
one instance of it will suffice). Furthermore the presence of a
comment here implies to me that one is missing from the
_[SU]16_[BL]E ones above as well as a few more below.

> +/*
> + * STATUS RETURN CODES.
> + */
> + /* Operation failed for some unspecified reason (-EIO). */
> +#define SNDIF_RSP_ERROR       -1

Missing parentheses.

> +struct sndif_request {
> +    uint8_t operation;          /* SNDIF_OP_??? */
> +    union {

Looks like you started from a Linux clone of blkif.h. This way of laying
out things is just not suitable without the use of (forbidden) compiler
extensions. Simply make the respective fields of this structure unions,
rather than using this union of structures.

> +        struct sndif_request_common common;
> +        struct sndif_request_open open;
> +        struct sndif_request_rw rw;
> +        struct sndif_request_volume vol;
> +    } u;
> +} __attribute__((__packed__));

And do you btw realize that this together with there not being
padding after "operation" yields all of the other structure fields
unaligned? IOW, besides needing to drop the packing, you
should also make all padding explicit, and that in a way that it
fits both 32- and 64-bit consumers.

> +struct sndif_response {
> +    uint64_t id;                /* copied from request */
> +    uint8_t operation;          /* copied from request */
> +    int16_t status;             /* SNDIF_RSP_???       */
> +};
> +
> +DEFINE_RING_TYPES(sndif, struct sndif_request, struct sndif_response);
> +
> +#endif /* __XEN_PUBLIC_IO_SNDIF_H__ */

Also, overall - despite existing headers under io/ giving bad examples,
please let's not add further public definitions without proper XEN_ or
xen_ prefixes.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel