Make a NaClDesc ctor for creating descs from NaClFileInfo.

Make a NaClDesc ctor for creating descs from NaClFileInfo.

Could make ownership transfer more direct without accidentally dropping
the file tokens:
https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trusted/plugin/plugin.cc#newcode499

Moves NaClFileInfo into src/public/ instead of being under validator,
so that src/trusted/desc doesn't import headers from validator
since src/trusted/validator already imports from src/trusted/desc.

I'm leaving a redirector under the old location to avoid
making the Chrome DEPS roll complicated.
Once it rolls into Chrome and the references are
fixed, we can delete the old copy/redirector.

Put the token validity checks into the ctor, so that the
caller can just say:

NaClDescIoFromFileToken...(info.Release(), RD_ONLY)

instead of checking validity before the call to Release().

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3421
R=mseaborn@chromium.org

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=13198

[Nick Bray (chromium), 2014-05-02 19:44:35]

https://codereview.chromium.org/261683002/diff/40001/src/trusted/desc/nacl_de...
File src/trusted/desc/nacl_desc_wrapper.cc (right):

https://codereview.chromium.org/261683002/diff/40001/src/trusted/desc/nacl_de...
src/trusted/desc/nacl_desc_wrapper.cc:325: if (info.file_token.lo != 0 ||
info.file_token.hi != 0) {
See previous CLs for comments on this check.

https://codereview.chromium.org/261683002/diff/40001/src/trusted/desc/nacl_de...
src/trusted/desc/nacl_desc_wrapper.cc:326: error = (*NACL_VTBL(NaClDesc, desc)->
Why no NaClDescSetFileToken?

[jvoung (off chromium), 2014-05-02 20:38:22]

https://codereview.chromium.org/261683002/diff/40001/src/trusted/desc/nacl_de...
File src/trusted/desc/nacl_desc_wrapper.cc (right):

https://codereview.chromium.org/261683002/diff/40001/src/trusted/desc/nacl_de...
src/trusted/desc/nacl_desc_wrapper.cc:325: if (info.file_token.lo != 0 ||
info.file_token.hi != 0) {
On 2014/05/02 19:44:35, Nick Bray (chromium) wrote:
> See previous CLs for comments on this check.

Done.

https://codereview.chromium.org/261683002/diff/40001/src/trusted/desc/nacl_de...
src/trusted/desc/nacl_desc_wrapper.cc:326: error = (*NACL_VTBL(NaClDesc, desc)->
On 2014/05/02 19:44:35, Nick Bray (chromium) wrote:
> Why no NaClDescSetFileToken?

That's part of desc_cacheabiilty/ and desc_cacheability/ depends on desc/, so I
didn't want to make desc/ depend on desc_cacheability too.

I've moved those helpers over to desc/ and reused them.

[Mark Seaborn, 2014-05-02 20:50:40]

https://codereview.chromium.org/261683002/diff/100001/src/trusted/desc/nacl_d...
File src/trusted/desc/nacl_desc_wrapper.h (right):

https://codereview.chromium.org/261683002/diff/100001/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_wrapper.h:34: DescWrapper*
MakeFileDescMetadata(struct NaClFileInfo file_info,
Is it your intention to use this new method from the Chromium side?

The reason I ask is that DescWrapper is quite deprecated, so it would be
preferable to add a C constructor that returns a NaClDesc* (and if necessary add
a NaClDesc -> DescWrapper constructor).

https://codereview.chromium.org/261683002/diff/100001/src/trusted/validator/n...
File src/trusted/validator/nacl_file_info.h (right):

https://codereview.chromium.org/261683002/diff/100001/src/trusted/validator/n...
src/trusted/validator/nacl_file_info.h:15: * This copy is left here to avoid a
complicated NaCl -> Chrome
You shouldn't leave this as a duplicate.  Instead, do:
#include "native_client/src/public/nacl_file_info.h"

[jvoung (off chromium), 2014-05-14 23:42:46]

https://codereview.chromium.org/261683002/diff/100001/src/trusted/desc/nacl_d...
File src/trusted/desc/nacl_desc_wrapper.h (right):

https://codereview.chromium.org/261683002/diff/100001/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_wrapper.h:34: DescWrapper*
MakeFileDescMetadata(struct NaClFileInfo file_info,
On 2014/05/02 20:50:40, Mark Seaborn wrote:
> Is it your intention to use this new method from the Chromium side?

Yes, this was to make the desc and token ownership transfer happen in one
function call, instead of having separate function calls:

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...



> The reason I ask is that DescWrapper is quite deprecated, so it would be
> preferable to add a C constructor that returns a NaClDesc* (and if necessary
add
> a NaClDesc -> DescWrapper constructor).

Okay, I didn't know that DescWrapper was deprecated.  The current plugin code
still uses that, so it seemed to match better.

I've add a C constructor (well mostly just calls the nacl_desc_io one) and use
that here to make the DescWrapper.

https://codereview.chromium.org/261683002/diff/100001/src/trusted/validator/n...
File src/trusted/validator/nacl_file_info.h (right):

https://codereview.chromium.org/261683002/diff/100001/src/trusted/validator/n...
src/trusted/validator/nacl_file_info.h:15: * This copy is left here to avoid a
complicated NaCl -> Chrome
On 2014/05/02 20:50:40, Mark Seaborn wrote:
> You shouldn't leave this as a duplicate.  Instead, do:
> #include "native_client/src/public/nacl_file_info.h"

Done.

[Mark Seaborn, 2014-05-15 00:36:10]

https://codereview.chromium.org/261683002/diff/200002/src/public/nacl_file_in...
File src/public/nacl_file_info.h (right):

https://codereview.chromium.org/261683002/diff/200002/src/public/nacl_file_in...
src/public/nacl_file_info.h:16: * NaClFileToken is a single-use nonce that the
NaCl process can use to query
Maybe also comment that this is metadata that is used for implementing
file-identity-based validation caching?  That becomes less obvious when this is
moved out of the 'validator' directory.

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
File src/trusted/desc/nacl_desc_file_info.c (right):

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_file_info.c:21: error = (*NACL_VTBL(NaClDesc, desc)->
Nit: Combine with "int error"

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_file_info.c:61: struct NaClDesc* desc =
NaClDescIoDescFromHandleAllocCtor(
Nit: use " *" spacing

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
File src/trusted/desc/nacl_desc_file_info.h (right):

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_file_info.h:40: struct NaClDesc
*NaClDescIoFromFileInfo(struct NaClFileInfo info,
This is intended to be called from the Chromium side, right?  If so, can you
declare it in src/public?  Maybe in src/public/nacl_file_info.h?

I'd like to avoid adding to the set of interfaces that Chromium uses that are
declared outside of src/public.

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
File src/trusted/desc/nacl_desc_wrapper.cc (right):

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_wrapper.cc:319: DescWrapper*
DescWrapperFactory::MakeFileDescFileInfo(struct NaClFileInfo info,
Since DescWrapper is deprecated, can you avoid adding more interface surface to
it, please?  Can you have the Chromium-side code call NaClDescIoFromFileInfo() +
MakeGenericCleanup() instead of adding this method?

[jvoung (off chromium), 2014-05-16 18:02:05]

https://codereview.chromium.org/261683002/diff/200002/src/public/nacl_file_in...
File src/public/nacl_file_info.h (right):

https://codereview.chromium.org/261683002/diff/200002/src/public/nacl_file_in...
src/public/nacl_file_info.h:16: * NaClFileToken is a single-use nonce that the
NaCl process can use to query
On 2014/05/15 00:36:10, Mark Seaborn wrote:
> Maybe also comment that this is metadata that is used for implementing
> file-identity-based validation caching?  That becomes less obvious when this
is
> moved out of the 'validator' directory.

Done.

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
File src/trusted/desc/nacl_desc_file_info.c (right):

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_file_info.c:21: error = (*NACL_VTBL(NaClDesc, desc)->
On 2014/05/15 00:36:10, Mark Seaborn wrote:
> Nit: Combine with "int error"

Done.

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_file_info.c:61: struct NaClDesc* desc =
NaClDescIoDescFromHandleAllocCtor(
On 2014/05/15 00:36:10, Mark Seaborn wrote:
> Nit: use " *" spacing

Done.

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
File src/trusted/desc/nacl_desc_file_info.h (right):

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_file_info.h:40: struct NaClDesc
*NaClDescIoFromFileInfo(struct NaClFileInfo info,
On 2014/05/15 00:36:10, Mark Seaborn wrote:
> This is intended to be called from the Chromium side, right?  If so, can you
> declare it in src/public?  Maybe in src/public/nacl_file_info.h?
> 
> I'd like to avoid adding to the set of interfaces that Chromium uses that are
> declared outside of src/public.

Done.

Yes it is intended to be called from Chrome. The implementation I've left in
desc/.

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
File src/trusted/desc/nacl_desc_wrapper.cc (right):

https://codereview.chromium.org/261683002/diff/200002/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_wrapper.cc:319: DescWrapper*
DescWrapperFactory::MakeFileDescFileInfo(struct NaClFileInfo info,
On 2014/05/15 00:36:10, Mark Seaborn wrote:
> Since DescWrapper is deprecated, can you avoid adding more interface surface
to
> it, please?  Can you have the Chromium-side code call NaClDescIoFromFileInfo()
+
> MakeGenericCleanup() instead of adding this method?

Okay. The intention was to have a single call that hands off the info's desc and
metadata, and that should still work with NaClDescIoFromFileInfo.

https://codereview.chromium.org/261683002/diff/250001/src/public/nacl_file_in...
File src/public/nacl_file_info.h (right):

https://codereview.chromium.org/261683002/diff/250001/src/public/nacl_file_in...
src/public/nacl_file_info.h:36: int32_t desc;
btw, Nick do you remember what the original intention of the "or" is?

(a) Is the "or" in the sense that this should be a NaClHandle (if so, then
should this be changed to NaClHandle which is also declared under src/public/)?

(b) Or is the "or" in the sense that this could be a union. Either a posix fd
(so on windows would need to convert w/ _open_osfhandle) or a NaClHandle?  The
code in chrome's trusted plugin seems to assume it's a posix fd... So, if we
want to be more strict and make this a NaClHandle, then the trusted plugin code
will need to be changed. With the factory method below NaClDescIoFromFileInfo,
I've been assuming case (a).

[Mark Seaborn, 2014-05-16 18:23:29]

To answer your question...

https://codereview.chromium.org/261683002/diff/250001/src/public/nacl_file_in...
File src/public/nacl_file_info.h (right):

https://codereview.chromium.org/261683002/diff/250001/src/public/nacl_file_in...
src/public/nacl_file_info.h:36: int32_t desc;
On 2014/05/16 18:02:05, jvoung wrote:
> btw, Nick do you remember what the original intention of the "or" is?
> 
> (a) Is the "or" in the sense that this should be a NaClHandle (if so, then
> should this be changed to NaClHandle which is also declared under
src/public/)?

If this field contains a Windows handle, I think it's fine to leave this as
int32_t on Windows, for two reasons:

While Windows' HANDLE is a typedef for void*, Windows does use an int type to
represent handles in some cases, in particular _open_osfhandle().  Handles are
really just ints.  It's simpler for NaCl to just use int32_t here.

While NaClHandle is declared in src/public/imc_types.h, this is an IMCism.  IMC
is deprecated and will eventually go away, so it's good for the
validator-caching code to not depend on its typedefs.

[Mark Seaborn, 2014-05-16 18:30:23]

"I'm leaving a copy under the old location" -- maybe s/copy/redirector/?

LGTM, thanks

https://codereview.chromium.org/261683002/diff/250001/src/trusted/desc/nacl_d...
File src/trusted/desc/nacl_desc_wrapper.cc (right):

https://codereview.chromium.org/261683002/diff/250001/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_wrapper.cc:19: #include
"native_client/src/trusted/desc/nacl_desc_file_info.h"
Not needed any more?

https://codereview.chromium.org/261683002/diff/250001/src/trusted/desc/nacl_d...
File src/trusted/desc/nacl_desc_wrapper.h (right):

https://codereview.chromium.org/261683002/diff/250001/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_wrapper.h:9: #include
"native_client/src/public/nacl_file_info.h"
Not needed any more?

[jvoung (off chromium), 2014-05-16 20:58:42]

https://codereview.chromium.org/261683002/diff/250001/src/public/nacl_file_in...
File src/public/nacl_file_info.h (right):

https://codereview.chromium.org/261683002/diff/250001/src/public/nacl_file_in...
src/public/nacl_file_info.h:36: int32_t desc;
On 2014/05/16 18:23:29, Mark Seaborn wrote:
> On 2014/05/16 18:02:05, jvoung wrote:
> > btw, Nick do you remember what the original intention of the "or" is?
> > 
> > (a) Is the "or" in the sense that this should be a NaClHandle (if so, then
> > should this be changed to NaClHandle which is also declared under
> src/public/)?
> 
> If this field contains a Windows handle, I think it's fine to leave this as
> int32_t on Windows, for two reasons:
> 
> While Windows' HANDLE is a typedef for void*, Windows does use an int type to
> represent handles in some cases, in particular _open_osfhandle().  Handles are
> really just ints.  It's simpler for NaCl to just use int32_t here.
> 
> While NaClHandle is declared in src/public/imc_types.h, this is an IMCism. 
IMC
> is deprecated and will eventually go away, so it's good for the
> validator-caching code to not depend on its typedefs.


Okay, on further inspection, it looks like all the users in NaCl and Chrome have
treated this as a posix file descriptor. It was only the validation_cache.h's
ResolveFileToken() method that expected a file desc or file handle.

I've changed the comment here to reflect that it's a posix file descriptor, and
changed the ctor.

https://codereview.chromium.org/261683002/diff/250001/src/trusted/desc/nacl_d...
File src/trusted/desc/nacl_desc_wrapper.cc (right):

https://codereview.chromium.org/261683002/diff/250001/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_wrapper.cc:19: #include
"native_client/src/trusted/desc/nacl_desc_file_info.h"
On 2014/05/16 18:30:23, Mark Seaborn wrote:
> Not needed any more?

Oops  -- Done.

https://codereview.chromium.org/261683002/diff/250001/src/trusted/desc/nacl_d...
File src/trusted/desc/nacl_desc_wrapper.h (right):

https://codereview.chromium.org/261683002/diff/250001/src/trusted/desc/nacl_d...
src/trusted/desc/nacl_desc_wrapper.h:9: #include
"native_client/src/public/nacl_file_info.h"
On 2014/05/16 18:30:23, Mark Seaborn wrote:
> Not needed any more?

Done.

[jvoung (off chromium), 2014-05-16 23:10:25]

Committed patchset #14 manually as r13198 (presubmit successful).