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

[Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>]

On Tue, Jan 20, 2015 at 12:22 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> 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.
I'll correct the commit message in the next patch-set.

>> Backend driver is an user-space application.
>
> This is irrelevant for the public interface.
I'll remove it in the next patch-set.

>> + 
>> *****************************************************************************
>> + *                            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.
I'll fix it in the next patch-set.

>> +/*
>> + * 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.
I'll remove all pointless parentheses in the next patch-set

>> +/* 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.
I'll remove those comments in the next patch-set.

>> +/*
>> + * STATUS RETURN CODES.
>> + */
>> + /* Operation failed for some unspecified reason (-EIO). */
>> +#define SNDIF_RSP_ERROR       -1
>
> Missing parentheses.
A'll all parentheses in the next patch-set.

>> +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.
I'll try to do this in the next patch-set.

>> +        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.
I'll remove all __attribute__((__packed__)) in the next patch-set
and I'll make all padding explicit in the next patch-set.

>> +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.
I'll try to do this in the next patch-set.

> Jan
>

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