Add support in mojo to check if a MojoHandle identifies a message pipe.

Add support in mojo to check if a MojoHandle identifies a message pipe.

Updated mojo spy to use the newly added function MojoIsMessagePipe to check if the handle
coming back from the ReadMessageRaw call is a message pipe. If yes we go ahead and intercept it.

Removed the hacks added in the previous patch which assumed that the first handle coming back from
the vendor is a message pipe.

BUG=360188
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283680

[viettrungluu, 2014-07-15 22:09:44]

Not LGTM. This is not an appropriately designed API.

[ananta, 2014-07-15 22:13:56]

On 2014/07/15 22:09:44, viettrungluu wrote:
> Not LGTM. This is not an appropriately designed API.

Please elaborate. Are you opposed to adding something like this in general or
just the API.

Thanks
Ananta

[viettrungluu, 2014-07-15 22:33:13]

On 2014/07/15 22:13:56, ananta wrote:
> On 2014/07/15 22:09:44, viettrungluu wrote:
> > Not LGTM. This is not an appropriately designed API.
> 
> Please elaborate. Are you opposed to adding something like this in general or
> just the API.
> 
> Thanks
> Ananta

This API is too specific.

Moreover, I don't think we should plan on adding this sort of thing anytime
soon.

If you really need to tell that a handle is a message pipe handle right now, you
can do so by (ab)using MojoReadMessage (with *num_bytes = 0 and *handles = 0).

[darin (slow to review), 2014-07-15 22:41:42]

What is the reason for avoiding APIs like this. I can imagine some things,
but curious what you have in mind. Are there some programming anti-patterns
you are trying to avoid?


On Tue, Jul 15, 2014 at 3:33 PM, <viettrungluu@chromium.org> wrote:

> On 2014/07/15 22:13:56, ananta wrote:
>
>> On 2014/07/15 22:09:44, viettrungluu wrote:
>> > Not LGTM. This is not an appropriately designed API.
>>
>
>  Please elaborate. Are you opposed to adding something like this in
>> general or
>> just the API.
>>
>
>  Thanks
>> Ananta
>>
>
> This API is too specific.
>
> Moreover, I don't think we should plan on adding this sort of thing anytime
> soon.
>
> If you really need to tell that a handle is a message pipe handle right
> now, you
> can do so by (ab)using MojoReadMessage (with *num_bytes = 0 and *handles =
> 0).
>
> https://codereview.chromium.org/391233002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ananta, 2014-07-15 23:40:02]

On 2014/07/15 22:41:42, darin wrote:
> What is the reason for avoiding APIs like this. I can imagine some things,
> but curious what you have in mind. Are there some programming anti-patterns
> you are trying to avoid?
> 
> 
> On Tue, Jul 15, 2014 at 3:33 PM, <mailto:viettrungluu@chromium.org> wrote:
> 
> > On 2014/07/15 22:13:56, ananta wrote:
> >
> >> On 2014/07/15 22:09:44, viettrungluu wrote:
> >> > Not LGTM. This is not an appropriately designed API.
> >>
> >
> >  Please elaborate. Are you opposed to adding something like this in
> >> general or
> >> just the API.
> >>
> >
> >  Thanks
> >> Ananta
> >>
> >
> > This API is too specific.
> >
> > Moreover, I don't think we should plan on adding this sort of thing anytime
> > soon.
> >
> > If you really need to tell that a handle is a message pipe handle right
> > now, you
> > can do so by (ab)using MojoReadMessage (with *num_bytes = 0 and *handles =
> > 0).
> >
> > https://codereview.chromium.org/391233002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Updated the patch to use MojoReadMessage to identify a message pipe. 

An API which given a handle returns the type would be useful. If this is
acceptable
I can look into it for a future patch.

[viettrungluu, 2014-07-16 02:39:30]

On 2014/07/15 22:41:42, darin wrote:
> What is the reason for avoiding APIs like this. I can imagine some things,
> but curious what you have in mind. Are there some programming anti-patterns
> you are trying to avoid?

As a general principle, an application should know what it has (or is supposed
to have). It's very difficult to do anything reasonable/safe without that prior
knowledge. It's not unreasonable to ask for an API to query information about a
handle, but as such it's not an especially high priority either. (Notice that
most OSes don't have much in the way of APIs to describe handles/FDs either.)

A second important question is what is a message pipe (handle)? This requires
careful thinking. As far as an application should be concerned, any handle that
looks and acts like one obtained from MojoCreateMessagePipe "is" a message pipe
handle. At a minimum, this means that the internal type constant
kTypeMessagePipe is not sufficient to answer this question (though this is an
implementation error, not the only one). It's not clear that you want to have a
complete enumeration of handle types.[*]

Another problem with the proposed API is that it's too specific. At the very
minimum, you'd want something more like "GetHandleType()". But as soon as you do
that, it becomes apparent that it's still far from sufficient to create an
"equivalent" handle. So you'd like something like "GetHandleInformation()" which
also returns, e.g., creation options. This has to be able to provide information
in a suitably extensible way.

(As a general comment, exposing any system API is quite costly, since it has to
be exposed for every language binding and then maintained. So general APIs are
greatly preferred.)

But even with all this information, creating an "equivalent" handle remains an
elusive problem. In the sense that a handle is a capability, in general you
can't create an equivalent handle (which is why OSes prefer to provide
DuplicateHandle()/dup()).

[*] The system may, under whatever circumstance, (say) provide you with a handle
which you can both read/write messages and read/write data to/from. There's
nothing that says that it can't do this, and it shouldn't be forced to invent a
type for every special type of object/handle. (Perhaps this just means that
there'd be a "special" type for such things.)

> 
> 
> On Tue, Jul 15, 2014 at 3:33 PM, <mailto:viettrungluu@chromium.org> wrote:
> 
> > On 2014/07/15 22:13:56, ananta wrote:
> >
> >> On 2014/07/15 22:09:44, viettrungluu wrote:
> >> > Not LGTM. This is not an appropriately designed API.
> >>
> >
> >  Please elaborate. Are you opposed to adding something like this in
> >> general or
> >> just the API.
> >>
> >
> >  Thanks
> >> Ananta
> >>
> >
> > This API is too specific.
> >
> > Moreover, I don't think we should plan on adding this sort of thing anytime
> > soon.
> >
> > If you really need to tell that a handle is a message pipe handle right
> > now, you
> > can do so by (ab)using MojoReadMessage (with *num_bytes = 0 and *handles =
> > 0).
> >
> > https://codereview.chromium.org/391233002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[viettrungluu, 2014-07-16 02:40:48]

Please write tests.

https://codereview.chromium.org/391233002/diff/20001/mojo/spy/spy.cc
File mojo/spy/spy.cc (right):

https://codereview.chromium.org/391233002/diff/20001/mojo/spy/spy.cc#newcode106
mojo/spy/spy.cc:106: MOJO_RESULT_RESOURCE_EXHAUSTED) {
I believe it'll return MOJO_RESULT_SHOULD_WAIT if no messages are queued.

[ananta, 2014-07-16 21:06:06]

https://codereview.chromium.org/391233002/diff/20001/mojo/spy/spy.cc
File mojo/spy/spy.cc (right):

https://codereview.chromium.org/391233002/diff/20001/mojo/spy/spy.cc#newcode106
mojo/spy/spy.cc:106: MOJO_RESULT_RESOURCE_EXHAUSTED) {
On 2014/07/16 02:40:48, viettrungluu wrote:
> I believe it'll return MOJO_RESULT_SHOULD_WAIT if no messages are queued.

Checked that. It returns MOJO_RESULT_SHOULD_WAIT or
MOJO_RESULT_FAILED_PRECONDITION.

I changed the code to check for MOJO_RESULT_INVALID_ARGUMENT which is what the
default dispatcher returns.

Will work on tests when I get back from vacation the week after next.

[cpu_(ooo_6.6-7.5), 2014-07-16 23:56:13]

lgtm

[ananta, 2014-07-16 23:56:42]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-07-16 23:59:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ananta@chromium.org/391233002/40001

[commit-bot: I haz the power, 2014-07-16 23:59:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-16 23:59:44]

A disapproval has been posted by following reviewers: viettrungluu@chromium.org.
Please make sure to get positive review before another CQ attempt.

[viettrungluu, 2014-07-17 01:11:20]

On 2014/07/16 21:06:06, ananta wrote:
> https://codereview.chromium.org/391233002/diff/20001/mojo/spy/spy.cc
> File mojo/spy/spy.cc (right):
> 
>
https://codereview.chromium.org/391233002/diff/20001/mojo/spy/spy.cc#newcode106
> mojo/spy/spy.cc:106: MOJO_RESULT_RESOURCE_EXHAUSTED) {
> On 2014/07/16 02:40:48, viettrungluu wrote:
> > I believe it'll return MOJO_RESULT_SHOULD_WAIT if no messages are queued.
> 
> Checked that. It returns MOJO_RESULT_SHOULD_WAIT or
> MOJO_RESULT_FAILED_PRECONDITION.
> 
> I changed the code to check for MOJO_RESULT_INVALID_ARGUMENT which is what the
> default dispatcher returns.
> 
> Will work on tests when I get back from vacation the week after next.

OK. LGTM.

[viettrungluu, 2014-07-17 01:11:27]

The CQ bit was checked by viettrungluu@chromium.org

[commit-bot: I haz the power, 2014-07-17 01:14:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ananta@chromium.org/391233002/40001

[commit-bot: I haz the power, 2014-07-17 05:52:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[commit-bot: I haz the power, 2014-07-17 07:07:38]

Change committed as 283680