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

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

On 11/25/2016 11:18 AM, Jan Beulich wrote:
> > > > 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.
done
> > +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).
changed __reserver0 to reserved
> > +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.
C99 says (6.2.5.20):
" An array type describes a contiguously allocated nonempty set of 
objects with a particular member object type,"
will change to *gref[1] /* Variable length */* as it is done in fsif.h
> > +struct xensnd_close_req {
> > +    /* place holder, remove if changing the structure (C89 concern) */
> > +    uint8_t __placeholder;
> > +};
changed __placeholder to 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).
you are right, C99 says ( 6.2.5.20):
" A structure type describes a sequentially allocated *nonempty* set of 
member objects"
C11 doesn't say anything that it is allowed
So, I will change in the comments *C89 concern* to *(C99 6.2.5.20)*
> > +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.
done
> > +union xensnd_resp {
> > +    struct {
> > +        uint16_t id;
> > +        uint8_t operation;
> > +        uint8_t stream_idx;
> > +        int8_t status;
> > +    } data;
> This lacks explicit tail padding.
done, added *uint8_t padding[3];*
> Jan
Thank you,
Oleksandr

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