C types and utilities to validate and access struct and message headers

C types and utilities to validate and access struct and message headers

This provides simple C types and utilities for validating Mojo struct
and message headers and accessing their data. These utilities are
written in C without any dependencies other than typedefs from the
stdbool.h and stdint.h headers so they should be usable in programs
that use different libcs or no libc at all.

These utilities are general for all struct / messages, there's no
codegen (yet) for particular structures or interfaces. As such it's
difficult to reuse the existing validation conformance tests as they do
not distinguish cleanly between tests for the Mojo archive format and
tests for mojom-generated interfaces. For now tests for these routine
are hand written reusing the validation input format but ideally they
would share data with the validation tests for other languages.

R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/0ba8e9e0eb08e291e6989477201345e043dc3aa8

[jamesr, 2016-02-01 23:59:41]

jamesr@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[jamesr, 2016-02-01 23:59:42]

I want to write some test programs in C with nostdlib and need some utilities to
help deal with Mojo formatting. I don't plan to write (or need) codegen in the
short term as I can hand write code for particular protocols, but once somebody
does get around to codegen they should be able to reuse this stuff.

[jamesr, 2016-02-02 00:03:23]

https://codereview.chromium.org/1654373002/diff/1/mojo/public/c/bindings/stru...
File mojo/public/c/bindings/struct.h (right):

https://codereview.chromium.org/1654373002/diff/1/mojo/public/c/bindings/stru...
mojo/public/c/bindings/struct.h:24: _Static_assert(sizeof(mojo_struct_header_t)
== 8u,
this is kind of annoying. the android NDK toolchain doesn't like this keyword
when compiling as c++, i.e. this file is included from a cc file, which is
possibly correct since this is a C11 keyword.  <assert.h> is supposed to define
a nice macro for static_assert but including <assert.h> from this header will
make my life harder in some users that don't want all of the other dependencies
<assert.h> drags in.

[viettrungluu, 2016-02-02 00:09:34]

https://codereview.chromium.org/1654373002/diff/1/mojo/public/c/bindings/stru...
File mojo/public/c/bindings/struct.h (right):

https://codereview.chromium.org/1654373002/diff/1/mojo/public/c/bindings/stru...
mojo/public/c/bindings/struct.h:24: _Static_assert(sizeof(mojo_struct_header_t)
== 8u,
On 2016/02/02 00:03:23, jamesr wrote:
> this is kind of annoying. the android NDK toolchain doesn't like this keyword
> when compiling as c++, i.e. this file is included from a cc file, which is
> possibly correct since this is a C11 keyword.  <assert.h> is supposed to
define
> a nice macro for static_assert but including <assert.h> from this header will
> make my life harder in some users that don't want all of the other
dependencies
> <assert.h> drags in.

Can you include the assertion in a .c file instead?

[jamesr, 2016-02-02 00:27:35]

I think that works (the trybots will confirm) but it's a little dodgy since the
user of the header file really needs to see the correct version of the struct. 
Maybe it's OK, or I could have #ifdef __cplusplus guards.  Or assume that it'll
work out if the user's toolchain compiles the .c file correct.

[viettrungluu, 2016-02-02 00:48:48]

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/message.c (right):

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:13: #define MESSAGE_EXPECTS_RESPONSE (1 <<
0u)
Probably these should be in the header (probably with more elaborate names).

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:40: return true;
I'd skip the return true here, if you're going to have an "else (if)" (with the
return true at the end).

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:46: // Version 1 has a request id and
should have one of these flags set.
Is this really what we do in the C++ bindings?

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:51: }
What if header->version > 1?

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
File mojo/public/c/bindings/message.h (right):

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/message.h:19: bool
mojo_validate_message_header(mojo_struct_header_t* header, uint32_t size);
const mojo_struct_header_t*?

|size_t| for size?

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/message.h:20: 
Probably you want suitable pack pragmas, no?

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/message.h:25: } mojo_message_header_t;
Note that POSIX sez that _t names are reserved for implementors of systems,
which is fine when we're the implementor, but not so good if people want to use
these headers on other systems.

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
File mojo/public/c/bindings/struct.h (right):

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/struct.h:17: bool mojo_validate_struct_header(void* data,
uint32_t size);
const void*?

Also, should you use a |size_t| for size?

[jamesr, 2016-02-02 00:55:04]

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/message.c (right):

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:13: #define MESSAGE_EXPECTS_RESPONSE (1 <<
0u)
On 2016/02/02 at 00:48:47, viettrungluu wrote:
> Probably these should be in the header (probably with more elaborate names).

Dumping a macro in a header sucks but I guess there isn't a great way around
this in C.  Can do.

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:40: return true;
On 2016/02/02 at 00:48:47, viettrungluu wrote:
> I'd skip the return true here, if you're going to have an "else (if)" (with
the return true at the end).

d'oh, will do

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:46: // Version 1 has a request id and
should have one of these flags set.
On 2016/02/02 at 00:48:47, viettrungluu wrote:
> Is this really what we do in the C++ bindings?

No we don't
(https://github.com/domokit/mojo/blob/master/mojo/public/cpp/bindings/lib/mess...)
but I'm not sure how anyone is supposed to interpret a message with a request ID
that doesn't set one of these flags.  Maybe this should just be accepted?

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:51: }
On 2016/02/02 at 00:48:47, viettrungluu wrote:
> What if header->version > 1?

Those deliberately validate as I don't know what to check for.  See
https://github.com/domokit/mojo/blob/master/mojo/public/cpp/bindings/lib/mess....
 Maybe I should duplicate that comment

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
File mojo/public/c/bindings/message.h (right):

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/message.h:19: bool
mojo_validate_message_header(mojo_struct_header_t* header, uint32_t size);
On 2016/02/02 at 00:48:47, viettrungluu wrote:
> const mojo_struct_header_t*?

Sure (although it's meaningless here)

> |size_t| for size?

I'm sticking with uint32_t for sizes as that's what the mojo archive format uses
for struct sizes+offsets and to avoid <stddef.h>.  Wouldn't mind switching if
you think it's strictly better.

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/message.h:20: 
On 2016/02/02 at 00:48:47, viettrungluu wrote:
> Probably you want suitable pack pragmas, no?

I'm relying on the static assertions to tell me if these need to be packed.  So
far the asserts seem to be holding true without any pragmas.

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/message.h:25: } mojo_message_header_t;
On 2016/02/02 at 00:48:47, viettrungluu wrote:
> Note that POSIX sez that _t names are reserved for implementors of systems,
which is fine when we're the implementor, but not so good if people want to use
these headers on other systems.

That's super annoying.  Is there another naming convention I could use or should
I just drop the convenience typedefs? I want to leave the real struct names so
forward decls can work

[jamesr, 2016-02-02 01:20:31]

Unless I'm missing something it looks like zlib, libpng, libxml, freetype,
opengl, egl, libevent, etc all violate the _t rule so I think I'll violate it
here in the name of having nicer types.  So long as we keep things mojo_
prefixed I think there's no real possibility of a collision.

[jamesr, 2016-02-02 01:30:28]

PTAL.  Latest patch puts the flag macros in the header, uses const pointers,
uses size_t for buffer lengths, and is consistent with the other bindings about
checking for flags in version==1 (although I'm not sure what the other bindings
do makes much sense).  I left the _t typedefs and omitted the packing pragma
until we have some evidence that they're needed somewhere.

[viettrungluu, 2016-02-02 18:19:16]

lgtm w/nits

Possibly we should ponder our C style though (c_hacker_style_function_names()?
underscore_t_for_typedefs_t?).

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/message.c (right):

https://codereview.chromium.org/1654373002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:46: // Version 1 has a request id and
should have one of these flags set.
On 2016/02/02 00:55:03, jamesr wrote:
> On 2016/02/02 at 00:48:47, viettrungluu wrote:
> > Is this really what we do in the C++ bindings?
> 
> No we don't
>
(https://github.com/domokit/mojo/blob/master/mojo/public/cpp/bindings/lib/mess...)
> but I'm not sure how anyone is supposed to interpret a message with a request
ID
> that doesn't set one of these flags.  Maybe this should just be accepted?

Yes. The reason is that the "version" is not a "type". One should be able to
always send the latest version of a struct (in this case, the header struct) if
one wishes. Sending a version 0 struct in the
not-expecting-response/non-response case is just an optimization.

https://codereview.chromium.org/1654373002/diff/40001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/message.c (right):

https://codereview.chromium.org/1654373002/diff/40001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:16: size < sizeof(mojo_message_header_t) ||
size > 0xffffffff) {
0xffffffffu (or 0xffffffffU)

Or UINT32_MAX (from <stdint.h>).

https://codereview.chromium.org/1654373002/diff/40001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/struct.c (right):

https://codereview.chromium.org/1654373002/diff/40001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/struct.c:11: if (size < sizeof(mojo_struct_header_t)
|| size > 0xffffffff) {
"

https://codereview.chromium.org/1654373002/diff/40001/mojo/public/c/bindings/...
File mojo/public/c/bindings/tests/bindings_unittest.cc (right):

https://codereview.chromium.org/1654373002/diff/40001/mojo/public/c/bindings/...
mojo/public/c/bindings/tests/bindings_unittest.cc:9: #include
"mojo/public/cpp/bindings/tests/validation_test_input_parser.h"
This dependency is ... interesting. Hrm.

https://codereview.chromium.org/1654373002/diff/40001/mojo/public/c/bindings/...
mojo/public/c/bindings/tests/bindings_unittest.cc:62: std::vector<uint8_t> data;
nit: Should include <vector>. Maybe <stdint.h> too.

[jamesr, 2016-02-03 18:40:48]

Description was changed from

==========
C types and utilities to validate and access struct and message headers

This provides simple C types and utilities for validating Mojo struct
and message headers and accessing their data. These utilities are
written in C without any dependencies other than typedefs from the
stdbool.h and stdint.h headers so they should be usable in programs
that use different libcs or no libc at all.

These utilities are general for all struct / messages, there's no
codegen (yet) for particular structures or interfaces. As such it's
difficult to reuse the existing validation conformance tests as they do
not distinguish cleanly between tests for the Mojo archive format and
tests for mojom-generated interfaces. For now tests for these routine
are hand written reusing the validation input format but ideally they
would share data with the validation tests for other languages.
==========

to

==========
C types and utilities to validate and access struct and message headers

This provides simple C types and utilities for validating Mojo struct
and message headers and accessing their data. These utilities are
written in C without any dependencies other than typedefs from the
stdbool.h and stdint.h headers so they should be usable in programs
that use different libcs or no libc at all.

These utilities are general for all struct / messages, there's no
codegen (yet) for particular structures or interfaces. As such it's
difficult to reuse the existing validation conformance tests as they do
not distinguish cleanly between tests for the Mojo archive format and
tests for mojom-generated interfaces. For now tests for these routine
are hand written reusing the validation input format but ideally they
would share data with the validation tests for other languages.

R=viettrungluu@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/0ba8e9e0eb08e291e6989477201...
==========

[jamesr, 2016-02-03 18:40:50]

Committed patchset #4 (id:60001) manually as
0ba8e9e0eb08e291e6989477201345e043dc3aa8 (presubmit successful).