[PATCH v1] binder: convert `BINDER_*` ioctl `#define`s into an `enum`

[PATCH v1] binder: convert `BINDER_*` ioctl `#define`s into an `enum`

[Miguel Ojeda]

bindgen (a tool which generates the "raw" C bindings for Rust) only
works (so far) with "simple" C `#define`s. In order to avoid having
to manually maintain these constants in the (potential) Rust side,
this patch converts them into an `enum`.

There may be support in the future for expanding macros that end up in
a "numeric" one: https://github.com/rust-lang/rust-bindgen/issues/753.

Co-developed-by: Wedson Almeida Filho <wedsonaf@google.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
Two notes:
  - Let me know if you prefer that I base this on top of a binder branch.
  - Wedson should OK the changes for his `Signed-off-by` tag (and probably
    he should be the author of the patch).

 include/uapi/linux/android/binder.h | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index 11157fae8a8e..ed4d11a0bb99 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -236,19 +236,21 @@ struct binder_frozen_status_info {
 	__u32            async_recv;
 };
 
-#define BINDER_WRITE_READ		_IOWR('b', 1, struct binder_write_read)
-#define BINDER_SET_IDLE_TIMEOUT		_IOW('b', 3, __s64)
-#define BINDER_SET_MAX_THREADS		_IOW('b', 5, __u32)
-#define BINDER_SET_IDLE_PRIORITY	_IOW('b', 6, __s32)
-#define BINDER_SET_CONTEXT_MGR		_IOW('b', 7, __s32)
-#define BINDER_THREAD_EXIT		_IOW('b', 8, __s32)
-#define BINDER_VERSION			_IOWR('b', 9, struct binder_version)
-#define BINDER_GET_NODE_DEBUG_INFO	_IOWR('b', 11, struct binder_node_debug_info)
-#define BINDER_GET_NODE_INFO_FOR_REF	_IOWR('b', 12, struct binder_node_info_for_ref)
-#define BINDER_SET_CONTEXT_MGR_EXT	_IOW('b', 13, struct flat_binder_object)
-#define BINDER_FREEZE			_IOW('b', 14, struct binder_freeze_info)
-#define BINDER_GET_FROZEN_INFO		_IOWR('b', 15, struct binder_frozen_status_info)
-#define BINDER_ENABLE_ONEWAY_SPAM_DETECTION	_IOW('b', 16, __u32)
+enum {
+	BINDER_WRITE_READ			= _IOWR('b', 1, struct binder_write_read),
+	BINDER_SET_IDLE_TIMEOUT			= _IOW('b', 3, __s64),
+	BINDER_SET_MAX_THREADS			= _IOW('b', 5, __u32),
+	BINDER_SET_IDLE_PRIORITY		= _IOW('b', 6, __s32),
+	BINDER_SET_CONTEXT_MGR			= _IOW('b', 7, __s32),
+	BINDER_THREAD_EXIT			= _IOW('b', 8, __s32),
+	BINDER_VERSION				= _IOWR('b', 9, struct binder_version),
+	BINDER_GET_NODE_DEBUG_INFO		= _IOWR('b', 11, struct binder_node_debug_info),
+	BINDER_GET_NODE_INFO_FOR_REF		= _IOWR('b', 12, struct binder_node_info_for_ref),
+	BINDER_SET_CONTEXT_MGR_EXT		= _IOW('b', 13, struct flat_binder_object),
+	BINDER_FREEZE				= _IOW('b', 14, struct binder_freeze_info),
+	BINDER_GET_FROZEN_INFO			= _IOWR('b', 15, struct binder_frozen_status_info),
+	BINDER_ENABLE_ONEWAY_SPAM_DETECTION	= _IOW('b', 16, __u32),
+};
 
 /*
  * NOTE: Two special error codes you should check for when calling

base-commit: 42226c989789d8da4af1de0c31070c96726d990c
-- 
2.36.1

Re: [PATCH v1] binder: convert `BINDER_*` ioctl `#define`s into an `enum`

[Greg Kroah-Hartman]

On Mon, May 16, 2022 at 12:04:01PM +0200, Miguel Ojeda wrote:
> bindgen (a tool which generates the "raw" C bindings for Rust) only
> works (so far) with "simple" C `#define`s. In order to avoid having
> to manually maintain these constants in the (potential) Rust side,
> this patch converts them into an `enum`.
> 
> There may be support in the future for expanding macros that end up in
> a "numeric" one: https://github.com/rust-lang/rust-bindgen/issues/753.
> 
> Co-developed-by: Wedson Almeida Filho <wedsonaf@google.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
> Two notes:
>   - Let me know if you prefer that I base this on top of a binder branch.

Yes, this does not apply to my char-misc.git tree in the char-misc-next
branch on git.kernel.org as I think we have added some new binder ioctls
recently.

Or you can make it against linux-next with the rust stuff removed, that
also would work as well.  But as-is, this patch does not work.

thanks,

greg k-h

Re: [PATCH v1] binder: convert `BINDER_*` ioctl `#define`s into an `enum`

[Miguel Ojeda]

On Mon, May 16, 2022 at 6:56 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Yes, this does not apply to my char-misc.git tree in the char-misc-next
> branch on git.kernel.org as I think we have added some new binder ioctls
> recently.

Yeah, exactly, I thought you might want the other base to avoid the
conflicts -- sending it in a bit!

Cheers,
Miguel

Re: [PATCH v1] binder: convert `BINDER_*` ioctl `#define`s into an `enum`

[Arnd Bergmann]

On Mon, May 16, 2022 at 11:04 AM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> -#define BINDER_WRITE_READ              _IOWR('b', 1, struct binder_write_read)
> -#define BINDER_SET_IDLE_TIMEOUT                _IOW('b', 3, __s64)
> -#define BINDER_SET_MAX_THREADS         _IOW('b', 5, __u32)
> -#define BINDER_SET_IDLE_PRIORITY       _IOW('b', 6, __s32)
> -#define BINDER_SET_CONTEXT_MGR         _IOW('b', 7, __s32)
> -#define BINDER_THREAD_EXIT             _IOW('b', 8, __s32)
> -#define BINDER_VERSION                 _IOWR('b', 9, struct binder_version)
> -#define BINDER_GET_NODE_DEBUG_INFO     _IOWR('b', 11, struct binder_node_debug_info)
> -#define BINDER_GET_NODE_INFO_FOR_REF   _IOWR('b', 12, struct binder_node_info_for_ref)
> -#define BINDER_SET_CONTEXT_MGR_EXT     _IOW('b', 13, struct flat_binder_object)
> -#define BINDER_FREEZE                  _IOW('b', 14, struct binder_freeze_info)
> -#define BINDER_GET_FROZEN_INFO         _IOWR('b', 15, struct binder_frozen_status_info)
> -#define BINDER_ENABLE_ONEWAY_SPAM_DETECTION    _IOW('b', 16, __u32)
> +enum {
> +       BINDER_WRITE_READ                       = _IOWR('b', 1, struct binder_write_read),
> +       BINDER_SET_IDLE_TIMEOUT                 = _IOW('b', 3, __s64),
> +       BINDER_SET_MAX_THREADS                  = _IOW('b', 5, __u32),
> +       BINDER_SET_IDLE_PRIORITY                = _IOW('b', 6, __s32),
> +       BINDER_SET_CONTEXT_MGR                  = _IOW('b', 7, __s32),
> +       BINDER_THREAD_EXIT                      = _IOW('b', 8, __s32),
> +       BINDER_VERSION                          = _IOWR('b', 9, struct binder_version),

I wonder if that breaks any tools that extract ioctl command number definitions
from kernel headers. I see one other header using enum, but everything else
has the #define. The main user of ioctl command definitions that comes to
mind is 'strace', but I don't know where they get the numbers from.

It's probably not a big deal as long as it's limited to binder, but it
may become
more of a problem if we do this for more subsystems over time.

         Arnd

Re: [PATCH v1] binder: convert `BINDER_*` ioctl `#define`s into an `enum`

[Miguel Ojeda]

On Wed, May 18, 2022 at 9:52 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> I wonder if that breaks any tools that extract ioctl command number definitions
> from kernel headers. I see one other header using enum, but everything else
> has the #define. The main user of ioctl command definitions that comes to
> mind is 'strace', but I don't know where they get the numbers from.
>
> It's probably not a big deal as long as it's limited to binder, but it
> may become
> more of a problem if we do this for more subsystems over time.

Good point. For public headers, we may just want to do this on the
Rust side, after all. And it should be as automated as possible too --
I will take a look.

In any case, I also took at `strace`. They indeed parse the C files --
with regexes, not an actual C preprocessor/compiler, and for some
files, including `binder.h`, they have custom code where they convert
the `enum`s into `#define`s and then call the common path on that. But
it does that only for `enum`s that match a regexp. So this particular
change does not work as-is, though would work if we give a suitable
name to the `enum` (I double-checked running the scripts with the
changes applied, and it seemed to work).

But who knows what other projects are doing anyway, so it may be best
to just hold on this change, since anyway Rust Binder is still an RFC
-- thus if Linus/Greg decide to merge the Rust support, this change
can still wait. And we may not need it anyway if we do it on the Rust
side.

In conclusion, I would drop this patch for the moment, and I will fix
it on our side.

(By the way, please note there was a v2:
https://lore.kernel.org/lkml/20220517102813.10310-1-ojeda@kernel.org/)

Cheers,
Miguel