Proposed API for querying information about (e.g. shared) buffers.

Proposed API for querying information about (e.g. shared) buffers.

This contains not a whit of implementation.

R=jamesr@chromium.org
BUG=#501

Committed: https://chromium.googlesource.com/external/mojo/+/79568dbabb976de125ad91ee78ecc17fb6f26d17

[viettrungluu, 2016-03-01 20:35:32]

Description was changed from

==========
Proposed API for querying information about (e.g. shared) buffers.

R=jamesr@chromium.org
==========

to

==========
Proposed API for querying information about (e.g. shared) buffers.

This contains not a whit of implementation.

R=jamesr@chromium.org
BUG=#501
==========

[viettrungluu, 2016-03-01 20:36:20]

https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer.h
File mojo/public/c/system/buffer.h (right):

https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer...
mojo/public/c/system/buffer.h:59: struct MOJO_ALIGNAS(8)
MojoDuplicateBufferHandleOptions {
This is an ABI-breaking change, but luckily, no one actually creates/uses this
struct currently. Phew.

[jamesr, 2016-03-07 19:26:36]

https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer.h
File mojo/public/c/system/buffer.h (right):

https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer...
mojo/public/c/system/buffer.h:161: //       |info_num_bytes| is too small).
hm, buffer too small is a MOJO_RESULT_RESOURCE_EXHAUSTED return code for
MojoReadMessage. if the implementation has a larger definition of the buffer in
mind than the caller how does the caller get access to the fields they
understand? seems impossible with this setup unless the caller guesses larger
and larger buffer sizes to allocate for MojoBufferInformation until they get
MOJO_RESULT_OK, only then to read out the fields they know

what if the caller had to populate |info->struct_size| before calling the
function and the callee populated the fields up to that size, or if the
implementation always filled up to min(sizeof(implementation's
MojoBufferInformation), |info_num_bytes|) into the passed-in struct?

https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer...
mojo/public/c/system/buffer.h:164: uint32_t info_num_bytes);            // In.
inparams before outparams?

[viettrungluu, 2016-03-07 20:44:23]

https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer.h
File mojo/public/c/system/buffer.h (right):

https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer...
mojo/public/c/system/buffer.h:161: //       |info_num_bytes| is too small).
On 2016/03/07 19:26:36, jamesr wrote:
> hm, buffer too small is a MOJO_RESULT_RESOURCE_EXHAUSTED return code for
> MojoReadMessage. if the implementation has a larger definition of the buffer
in
> mind than the caller how does the caller get access to the fields they
> understand? seems impossible with this setup unless the caller guesses larger
> and larger buffer sizes to allocate for MojoBufferInformation until they get
> MOJO_RESULT_OK, only then to read out the fields they know

See the parenthetical comment in the first paragraph of the comment (this will
accept and populate according to old valid sizes).

> what if the caller had to populate |info->struct_size| before calling the
> function and the callee populated the fields up to that size,

I thought about that, but opted to not do it, since in a kernel impl it'd mean
having to read in from userspace just to get a uint32_t.

> or if the
> implementation always filled up to min(sizeof(implementation's
> MojoBufferInformation), |info_num_bytes|) into the passed-in struct?

We could do that. I just wonder if, e.g., setting |info_num_bytes| to 0 (or
worse, 1; or maybe more plausibly bad, 5) is valid. Or what "filled up to"
means. (The easiest implementation would be to partially memcpy a "full" struct,
and set info->struct_size to that amount copied, but that seems a little
sketchy.)

https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer...
mojo/public/c/system/buffer.h:164: uint32_t info_num_bytes);            // In.
On 2016/03/07 19:26:36, jamesr wrote:
> inparams before outparams?

I'm following existing (arguably-bad[*]) precedent (e.g., in MojoReadMessage).

[*] Except that it'd get violated anyway, since MojoReadMessage has multiple
buffers, and having the size next to the buffer seems more desirable than
following the in-then-out order. (Though MojoReadMessage's size params are
in/out, not just in.)

[jamesr, 2016-03-07 21:42:36]

https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer.h
File mojo/public/c/system/buffer.h (right):

https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer...
mojo/public/c/system/buffer.h:161: //       |info_num_bytes| is too small).
On 2016/03/07 at 20:44:22, viettrungluu wrote:
> On 2016/03/07 19:26:36, jamesr wrote:
> > hm, buffer too small is a MOJO_RESULT_RESOURCE_EXHAUSTED return code for
> > MojoReadMessage. if the implementation has a larger definition of the buffer
in
> > mind than the caller how does the caller get access to the fields they
> > understand? seems impossible with this setup unless the caller guesses
larger
> > and larger buffer sizes to allocate for MojoBufferInformation until they get
> > MOJO_RESULT_OK, only then to read out the fields they know
> 
> See the parenthetical comment in the first paragraph of the comment (this will
accept and populate according to old valid sizes).

Do we have any definition for what "old valid sizes" are?

> 
> > what if the caller had to populate |info->struct_size| before calling the
> > function and the callee populated the fields up to that size,
> 
> I thought about that, but opted to not do it, since in a kernel impl it'd mean
having to read in from userspace just to get a uint32_t.
> 
> > or if the
> > implementation always filled up to min(sizeof(implementation's
> > MojoBufferInformation), |info_num_bytes|) into the passed-in struct?
> 
> We could do that. I just wonder if, e.g., setting |info_num_bytes| to 0 (or
worse, 1; or maybe more plausibly bad, 5) is valid. Or what "filled up to"
means. (The easiest implementation would be to partially memcpy a "full" struct,
and set info->struct_size to that amount copied, but that seems a little
sketchy.)

Aside: with this scheme it seems impossible to add fields to the struct unless
they grow the size, which means if we want to add a 4-byte value (like another
flags field or something) we need to grow the struct by 8 bytes or we wouldn't
be able to add another 4-byte value later on

Maybe an explicit version field along with documented values for the size of
each version would have been better, but it's kinda late for that now.

How about we just document what values of |info_num_bytes| we support and
populate, i.e.

16: struct_size, flags, num_bytes

then sometime in the future

24: struct_size, flags, num_bytes, alignment (or whatever) the next field is

and then say that if info_num_bytes is >= any value the implementation
understands it fills in the largest value it understands and sets that in
struct_size, otherwise if info_num_bytes is not equal to one the values the
implementation understands it returns INVALID_ARGUMENT, otherwise it fills in
that exact version.

https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer...
mojo/public/c/system/buffer.h:164: uint32_t info_num_bytes);            // In.
On 2016/03/07 at 20:44:23, viettrungluu wrote:
> On 2016/03/07 19:26:36, jamesr wrote:
> > inparams before outparams?
> 
> I'm following existing (arguably-bad[*]) precedent (e.g., in MojoReadMessage).
> 
> [*] Except that it'd get violated anyway, since MojoReadMessage has multiple
buffers, and having the size next to the buffer seems more desirable than
following the in-then-out order. (Though MojoReadMessage's size params are
in/out, not just in.)

Read's params are in/out except for flags which comes at the end. blah!

the other crappy thing about this ordering is that it's 32 bit, 64 bit, then 32
bit. if it was 32/32/64 it could potentially pack better

[viettrungluu, 2016-03-07 23:30:40]

On 2016/03/07 21:42:36, jamesr wrote:
>
https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer.h
> File mojo/public/c/system/buffer.h (right):
> 
>
https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer...
> mojo/public/c/system/buffer.h:161: //       |info_num_bytes| is too small).
> On 2016/03/07 at 20:44:22, viettrungluu wrote:
> > On 2016/03/07 19:26:36, jamesr wrote:
> > > hm, buffer too small is a MOJO_RESULT_RESOURCE_EXHAUSTED return code for
> > > MojoReadMessage. if the implementation has a larger definition of the
buffer
> in
> > > mind than the caller how does the caller get access to the fields they
> > > understand? seems impossible with this setup unless the caller guesses
> larger
> > > and larger buffer sizes to allocate for MojoBufferInformation until they
get
> > > MOJO_RESULT_OK, only then to read out the fields they know
> > 
> > See the parenthetical comment in the first paragraph of the comment (this
will
> accept and populate according to old valid sizes).
> 
> Do we have any definition for what "old valid sizes" are?



> 
> > 
> > > what if the caller had to populate |info->struct_size| before calling the
> > > function and the callee populated the fields up to that size,
> > 
> > I thought about that, but opted to not do it, since in a kernel impl it'd
mean
> having to read in from userspace just to get a uint32_t.
> > 
> > > or if the
> > > implementation always filled up to min(sizeof(implementation's
> > > MojoBufferInformation), |info_num_bytes|) into the passed-in struct?
> > 
> > We could do that. I just wonder if, e.g., setting |info_num_bytes| to 0 (or
> worse, 1; or maybe more plausibly bad, 5) is valid. Or what "filled up to"
> means. (The easiest implementation would be to partially memcpy a "full"
struct,
> and set info->struct_size to that amount copied, but that seems a little
> sketchy.)
> 
> Aside: with this scheme it seems impossible to add fields to the struct unless
> they grow the size, which means if we want to add a 4-byte value (like another
> flags field or something) we need to grow the struct by 8 bytes or we wouldn't
> be able to add another 4-byte value later on

Yeah. I suppose for Mojo -> user information getters, it's not actually a
problem, but it's a potential problem for parameter structs.

> 
> Maybe an explicit version field along with documented values for the size of
> each version would have been better, but it's kinda late for that now.
> 
> How about we just document what values of |info_num_bytes| we support and
> populate, i.e.
> 
> 16: struct_size, flags, num_bytes
> 
> then sometime in the future
> 
> 24: struct_size, flags, num_bytes, alignment (or whatever) the next field is
> 
> and then say that if info_num_bytes is >= any value the implementation
> understands it fills in the largest value it understands and sets that in
> struct_size,

By "any value", you mean "all values" (equivalently "the largest value")?
Otherwise, you seem to be saying >= the smallest (i.e., first) value the
implementation understands.

> otherwise if info_num_bytes is not equal to one the values the
> implementation understands it returns INVALID_ARGUMENT, otherwise it fills in
> that exact version.

But if you meant ">= the largest value", then this seems to lead to backwards
incompatibility. E.g., I pass 50 now, and it works (filling it with the current
struct size). But later if the struct is extended to something larger than 50
(with 50 never being *the* size for any version), then it'll stop working.

How about just requiring that info_num_bytes be >= the first/smallest size, and
that as much information (among known versions) is filled in as possible.

E.g., if the valid sizes are 16, 24, and 48, and you pass in 32, then it'll fill
in/set 24.

> 
>
https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer...
> mojo/public/c/system/buffer.h:164: uint32_t info_num_bytes);            // In.
> On 2016/03/07 at 20:44:23, viettrungluu wrote:
> > On 2016/03/07 19:26:36, jamesr wrote:
> > > inparams before outparams?
> > 
> > I'm following existing (arguably-bad[*]) precedent (e.g., in
MojoReadMessage).
> > 
> > [*] Except that it'd get violated anyway, since MojoReadMessage has multiple
> buffers, and having the size next to the buffer seems more desirable than
> following the in-then-out order. (Though MojoReadMessage's size params are
> in/out, not just in.)
> 
> Read's params are in/out except for flags which comes at the end. blah!
> 
> the other crappy thing about this ordering is that it's 32 bit, 64 bit, then
32
> bit. if it was 32/32/64 it could potentially pack better

Does this make a difference at the C calling convention/ABI level? (On what
platforms/ABIs? Will things really pack two 32-bit values into a 64-bit register
in calling a function? I'm not sure that'd actually be an advantage, since it
may still be better to have the 32-bit values in separate 64-bit registers.)

[jamesr, 2016-03-07 23:34:16]

On 2016/03/07 at 23:30:40, viettrungluu wrote:
> On 2016/03/07 21:42:36, jamesr wrote:
> > > > what if the caller had to populate |info->struct_size| before calling
the
> > > > function and the callee populated the fields up to that size,
> > > 
> > > I thought about that, but opted to not do it, since in a kernel impl it'd
mean
> > having to read in from userspace just to get a uint32_t.
> > > 
> > > > or if the
> > > > implementation always filled up to min(sizeof(implementation's
> > > > MojoBufferInformation), |info_num_bytes|) into the passed-in struct?
> > > 
> > > We could do that. I just wonder if, e.g., setting |info_num_bytes| to 0
(or
> > worse, 1; or maybe more plausibly bad, 5) is valid. Or what "filled up to"
> > means. (The easiest implementation would be to partially memcpy a "full"
struct,
> > and set info->struct_size to that amount copied, but that seems a little
> > sketchy.)
> > 
> > Aside: with this scheme it seems impossible to add fields to the struct
unless
> > they grow the size, which means if we want to add a 4-byte value (like
another
> > flags field or something) we need to grow the struct by 8 bytes or we
wouldn't
> > be able to add another 4-byte value later on
> 
> Yeah. I suppose for Mojo -> user information getters, it's not actually a
problem, but it's a potential problem for parameter structs.

It's a potential problem for us if we want to grow the struct.  Not a problem
for applications

> 
> > 
> > Maybe an explicit version field along with documented values for the size of
> > each version would have been better, but it's kinda late for that now.
> > 
> > How about we just document what values of |info_num_bytes| we support and
> > populate, i.e.
> > 
> > 16: struct_size, flags, num_bytes
> > 
> > then sometime in the future
> > 
> > 24: struct_size, flags, num_bytes, alignment (or whatever) the next field is
> > 
> > and then say that if info_num_bytes is >= any value the implementation
> > understands it fills in the largest value it understands and sets that in
> > struct_size,
> 
> By "any value", you mean "all values" (equivalently "the largest value")?
Otherwise, you seem to be saying >= the smallest (i.e., first) value the
implementation understands.
> 
> > otherwise if info_num_bytes is not equal to one the values the
> > implementation understands it returns INVALID_ARGUMENT, otherwise it fills
in
> > that exact version.
> 
> But if you meant ">= the largest value", then this seems to lead to backwards
incompatibility. E.g., I pass 50 now, and it works (filling it with the current
struct size). But later if the struct is extended to something larger than 50
(with 50 never being *the* size for any version), then it'll stop working.

Yeah, but if you pass 50 to an implementation that only knows about fields up to
32 how is it supposed to react?

> How about just requiring that info_num_bytes be >= the first/smallest size,
and that as much information (among known versions) is filled in as possible.
> 
> E.g., if the valid sizes are 16, 24, and 48, and you pass in 32, then it'll
fill in/set 24.

That could work.  The caller can look at |struct_size| to see that they got 24
bytes, right?  We should document the known sizes then (for now just 16)

> >
https://codereview.chromium.org/1749353002/diff/1/mojo/public/c/system/buffer...
> > mojo/public/c/system/buffer.h:164: uint32_t info_num_bytes);            //
In.
> > On 2016/03/07 at 20:44:23, viettrungluu wrote:
> > > On 2016/03/07 19:26:36, jamesr wrote:
> > > > inparams before outparams?
> > > 
> > > I'm following existing (arguably-bad[*]) precedent (e.g., in
MojoReadMessage).
> > > 
> > > [*] Except that it'd get violated anyway, since MojoReadMessage has
multiple
> > buffers, and having the size next to the buffer seems more desirable than
> > following the in-then-out order. (Though MojoReadMessage's size params are
> > in/out, not just in.)
> > 
> > Read's params are in/out except for flags which comes at the end. blah!
> > 
> > the other crappy thing about this ordering is that it's 32 bit, 64 bit, then
32
> > bit. if it was 32/32/64 it could potentially pack better
> 
> Does this make a difference at the C calling convention/ABI level? (On what
platforms/ABIs? Will things really pack two 32-bit values into a 64-bit register
in calling a function? I'm not sure that'd actually be an advantage, since it
may still be better to have the 32-bit values in separate 64-bit registers.)

/me did some more reading.  These won't get packed together in any calling
convention I can find, but it could matter if someone wanted to create a struct
with the parameters for forwarding or whatever.  Not a big deal most likely.

[jamesr, 2016-03-07 23:34:29]

In other words, add some comments documenting the interface.  code lgtm

[viettrungluu, 2016-03-08 00:38:21]

Description was changed from

==========
Proposed API for querying information about (e.g. shared) buffers.

This contains not a whit of implementation.

R=jamesr@chromium.org
BUG=#501
==========

to

==========
Proposed API for querying information about (e.g. shared) buffers.

This contains not a whit of implementation.

R=jamesr@chromium.org
BUG=#501

Committed:
https://chromium.googlesource.com/external/mojo/+/79568dbabb976de125ad91ee78e...
==========

[viettrungluu, 2016-03-08 00:38:23]

Committed patchset #2 (id:20001) manually as
79568dbabb976de125ad91ee78ecc17fb6f26d17 (presubmit successful).

[viettrungluu, 2016-03-08 00:39:01]

On 2016/03/07 23:34:29, jamesr wrote:
> In other words, add some comments documenting the interface.  code lgtm

Done, thanks.