Improve module macro

[wedsonaf]

1. Fields should be specifiable in any order
2. Only license and type should be mandatory, the others should be optional

[kloenk]

will try myself on this now, as I need alias for the net stuff.

[ojeda]

Field should be specifiable in any order

I commented on this somewhere else. While the C side allows any ordering (and uses separate macros), I think forcing always the same order will be better for consistency long-term and to always see our module! macro written the same way. The {}-style macro can be misleading though, perhaps we can change that.

(Also, we need to be mindful that if we allow arbitrary orderings now, it will be harder to go back and enforce a fixed order later on because it will require fixing many modules).

[kloenk]

If we have a fixed ordering, what if we don't need a field in a driver? For example alias_rtnl_link I only need in the network driver. what about drivers not using that field. IMHO it's easier to implement without fixed ordering, and also I don't see a real reason for ordering, besides convenience.

[wedsonaf]

I commented on this somewhere else. While the C side allows any ordering (and uses separate macros), I think forcing always the same order will be better for consistency long-term and to always see our module! macro written the same way. The {}-style macro can be misleading though, perhaps we can change that.

I don't mind if we decide to force a strict order. (I'm more interested in making optional arguments truly optional.) However, at the moment I'm failing to see the benefits we get from it (the strict ordering).

[ojeda]

If we have a fixed ordering, what if we don't need a field in a driver?

Optional fields are OK, that is orthogonal to the ordering.

I don't see a real reason for ordering, besides convenience.

Consistency is the reason :) In my view, anything that allows arbitrary ordering needs to be justified, rather than the opposite...

[ojeda]

However, at the moment I'm failing to see the benefits we get from it (the strict ordering).

Basically, we need to choose between easier-to-write vs. easier-to-read. Since most of the time we are reading, I think it is best to enforce the fixed ordering.

In the end, I assume people will be copy-pasting the macro from one file to another, so I thought it was not a big deal to have a fixed ordering.

As you know, I usually "fight" for consistency everywhere. :-P But if people really hate it (three people so far have suggested arbitrary orderings already), I am not going to be against everyone -- we can change it.

[kloenk]

@wedsonaf you said name is optional, what should the __LOG_PREFIX be if name is not set? should it default to the stringified version of type?

[ojeda]

One alternative could be the file/crate name, since kernel modules that are single-file typically have the same name as the kernel module name.

[wedsonaf]

Basically, we need to choose between easier-to-write vs. easier-to-read. Since most of the time we are reading, I think it is best to enforce the fixed ordering.

We do need to make it minimally easy to write. For example, if I swap the order of author and description, here's what I get:

error: proc macro panicked
  --> drivers/android/rust_binder.rs:32:1
   |
32 | / module! {
33 | |     type: BinderModule,
34 | |     name: b"rust_binder",
35 | |     description: b"Android Binder",
...  |
38 | |     params: {},
39 | | }
   | |_^
   |
   = help: message: assertion failed: `(left == right)`
             left: `"description"`,
            right: `"author"`

So IMO we either need to allow any order or have a better error reporting when we get things out of order.

[kloenk]

im building something like panic!("'{}' is not allowed at this position", name); where name is the name of the current field, e.g. "author"

[wedsonaf]

im building something like panic!("'{}' is not allowed at this position", name); where name is the name of the current field, e.g. "author"

Suppose I'm writing a driver and I get the error message above. It's certainly more helpful than what I get today, but it doesn't guide me to a solution: perhaps also specify what is allowed?

[ojeda]

We do need to make it minimally easy to write.

Agreed, better error reporting for module! was discussed elsewhere, but the current errors are a consequence of the current (straightforward) implementation. In general, we should not base the design off those limitations.

[kloenk]

One alternative could be the file/crate name, since kernel modules that are single-file typically have the same name as the kernel module name.

I only see this unstable api[1], to get the file/crate name. as I could stringify the type I would prefer not to use this. are you ok with that?

1: proc_macro::Span

[ojeda]

are you ok with that?

Hmm... usually kernel module names have snake_case. Not sure what we should do. Perhaps keep the name non-optional for the moment, and we can revisit later.

[ojeda]

To improve error reporting easily, what you can do is walk the token stream as if ordering was arbitrary, since that it is easy, but keep a list of seen keys. Then, at the very end, check that the list is as expected, and otherwise you can panic with:

Keys need to be ordered like this: <list properly ordered>

e.g. if the user gave license and type, only those need to appear -- no need to give all the possibilities.

That way, errors that are about invalid keys or invalid values are reported first, without bothering the user about the ordering. Then, at the end, where you know everything is valid but only the ordering was wrong, you can report exactly that, since you know only the ordering was wrong.