[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.