Conversation
| GBINDER_STATUS_OK = 0, | ||
| GBINDER_STATUS_FAILED, | ||
| GBINDER_STATUS_DEAD_OBJECT | ||
| GBINDER_STATUS_UNKNOWN_ERROR = (-2147483647-1), // INT32_MIN value |
There was a problem hiding this comment.
Since this is supposed to be a stable interface, the externally exposed status codes must be a stable enum, and internal errors need to be translated internally from ENOSYS or whatever into the interface enum values.
There was a problem hiding this comment.
Yes that makes sense, though it's more the other way around - we need to be able to return these codes like UNKNOWN_TRANSACTION and PERMISSION_DENIED in order to accurately flag errors in calls to binder services. Currently returning GBINDER_STATUS_FAILED in a GBinderLocalReply causes 'Unknown binder error code. 0x1' when interpreted here. So they're not always being caught right now.
There was a problem hiding this comment.
If that's the value passed to gbinder_driver_reply_status() then what about replacing GBINDER_STATUS_FAILED with INT32_MIN and GBINDER_STATUS_DEAD_OBJECT with -EPIPE. Would that do it?
There was a problem hiding this comment.
Nearly. We should be sure that the actual values will be completely internal, i.e. they're translated back when returned by Android services too.
Can we not add the others to the end of the enum without breaking anything though? We really need GBINDER_STATUS_PERMISSION_DENIED (-EPERM) to get these tests working nicely, but I can see GBINDER_STATUS_UNKNOWN_TRANSACTION (-EBADMSG) should be what everything returns in the default case of all transaction code switches. Really any of them can be returned by Android services.
There was a problem hiding this comment.
If these are going to be completely internal, then they would have to be replaced in requests too. I guess gbinder_driver_txstatus would be the place to do that?
There was a problem hiding this comment.
Yes, a few additional public enum values feels like the way to go and gbinder_driver_txstatus() does seem to be the right place for the conversion, although I'm a bit puzzled by the switch under the "Filter out special cases" comment - it seems to do the conversion wrong way, e.g. replacing GBINDER_STATUS_DEAD_OBJECT with -EFAULT. Now that I'm looking at it, I'd say it should convert e.g. -EPIPE to GBINDER_STATUS_DEAD_OBJECT since that's what's going to be returned as a status by e.g. gbinder_client_transact() and friends (unless I'm missing something)
There was a problem hiding this comment.
Ah I'm glad I wasn't the only one confused by that. I had no idea where that EFAULT had come from. So my plan is to convert FAILED and DEAD_OBJECT in there and reply_status, and add the others with their actual values so that's not necessary. Is that ok, or would you rather they were all converted?
|
|
||
| #define GBINDER_FIRST_CALL_TRANSACTION (0x00000001) | ||
| #define GBINDER_TRANSACTION(c2,c3,c4) GBINDER_FOURCC('_',c2,c3,c4) | ||
| #define GBINDER_PING_TRANSACTION GBINDER_TRANSACTION('P','N','G') |
There was a problem hiding this comment.
While this doesn't break anything, I don't see much of a value in defining internal RPC protocol specific constants in public header files. One of the design goals has been to make the API as independent on the underlying implementation as possible and this looks like a step in the opposite direction.
There was a problem hiding this comment.
We need to implement DUMP to pass CTS tests, so we need to either paste this into other code or use it from here. The binder_dump test pastes it though, so is that what you would prefer?
There was a problem hiding this comment.
I would rather have these transaction codes defined in places where they're being used. I generally don't like duplicating anything, but in this particular case I don't think it's worth the trouble. Adding HIDL_FOURCC macro is OK, though.
There was a problem hiding this comment.
I feel dump is ok to have in the headers though? It's not really an internal transaction, more a service specific one. The others probably can be kept internal.
There was a problem hiding this comment.
Maybe we could reduce this set to only the transactions that are the responsibility of the service to implement? This particular test we're dealing with (ServicePermissionsTest#testDumpProtected) requires that DUMP is implemented for every service, so it seems like a definition that really must be used or defined by every AIDL service.
I can understand keeping things like PING out of the public declaration, as that's completely handled internally and isn't something that services need to care about at all, if I have that right? But they've mixed up internal low-level operations with standard interface definitions here. From what I can see used in Android, most seem to be intended to be handled at a lower level except for maybe SHELL_COMMAND and SYSPROPS.
Maybe in HIDL they are all truly internal, so it's not necessary to make any of them public. I'll take that whole block out.
| #define GBINDER_EXTENSION_TRANSACTION GBINDER_TRANSACTION('E','X','T'), | ||
| #define GBINDER_DEBUG_PID_TRANSACTION GBINDER_TRANSACTION('P','I','D'), | ||
| #define GBINDER_SET_RPC_CLIENT_TRANSACTION GBINDER_TRANSACTION('R','P','C'), | ||
| #undef GBINDER_FOURCC |
There was a problem hiding this comment.
The GBINDER_FOURCC macro has been there since the very beginning, may be considered part of the compile-time API and should remain defined.
There was a problem hiding this comment.
Ah yes sorry, I thought I was moving that over from the private headers. I think I should undef GBINDER_TRANSACTION here though instead?
There was a problem hiding this comment.
If GBINDER_TRANSACTION and HIDL_FOURCC macros are to be defined in the public headers, I suggest to call them e.g. GBINDER_AIDL_TRANSACTION and GBINDER_HIDL_TRANSACTION to avoid compile-time collisions with other macros with HIDL_ prefix. And I still think that defining individual transaction codes would be an overkill.
There was a problem hiding this comment.
Ok. After a bit more digging yesterday I did see that the response to the INTERFACE call just should be interface descriptor string, but that's probably still too high level to handle in gbinder.
There was a problem hiding this comment.
AIDL-style INTERFACE transactions are handled by gbinder_local_object_interface_transaction() and HIDL-style GET_DESCRIPTOR are handled by gbinder_local_object_hidl_get_descriptor_transaction(), are BTW that happens on the looper thread because the object's interface information is immutable and there's nothing to synchronize with the main thread. Are you saying that the response format for those transactions has changed?
985b03c to
3d324a3
Compare
|
Any good now? |
|
sorry, I haven't had time to really look at it past weekend, please give me a bit more time... |
|
No problem, we defined everything in other places for now so we're not blocked or anything. |
| break; | ||
| default: | ||
| mapped_status = status; | ||
|
|
| txstatus = GBINDER_STATUS_UNKNOWN_ERROR; | ||
| break; | ||
| case GBINDER_STATUS_DEAD_OBJECT: | ||
| txstatus = (-EPIPE); |
There was a problem hiding this comment.
Another break is missing, and isn't this switch supposed to convert things like INT32_MIN, -EPIPE etc, into the interface values e.g. GBINDER_STATUS_DEAD_OBJECT?
…es. JB#61891 Useful for implementation of DUMP and other internal transactions (except for the silly ones). Also define proper status codes and translate the existing ones as the current GBINDER_STATUS_FAILED is not recognized.
3 participants
Useful for implementation of DUMP and other internal transactions (except for the silly ones). Also define proper error codes for them as the current GBINDER_STATUS_FAILED is not recognized.