mtod macro: Drop the type argument

[cem]

Summary

In the past we've moved away from macros with explicit types as parameters
in favor of casting void* results as needed (e.g., MALLOC()). Here, do the
same with mtod()*.

Obviously, this requires changing a lot of C files. To do so, a Coccinelle
semantic patch was used. It is provided in tools for consumers that want to
convert their code to the 1 argument variant.

*: For out of tree consumers, such as ports, a 2-argument variant of the
mtod macro is retained with identical API to the prior version.

The collection of preprocessor macro hacks used to construct a variadic
mtod() is shamelessly appropriated from Gregory Pakosz:
https://stackoverflow.com/a/1872506/218830

Test Plan

The real patch will mechanically convert all files that reference the
macro, but that does not seem especially useful to post on Phabricator. So
instead I have converted a single file for example: netinet/ip_options.c.
No hand-editing was performed.

TODO:

• Bump __FreeBSD_version
• Update the manual page mtod is in
• Fix minor unused variable in cocci script

[cem]

convert_mtod.cocci

24 ↗

(On Diff #62142)

This identifier i line is unused and can be dropped. I forgot to remove it when I separated out r1 from r2.

sys/netinet/ip_options.c

104

This and others like it are good examples of nicely eliminated duplication.

456

This is about as legible as it was before, I think.

[markj]

I have no objections to the change in principle, it's a nice cleanup. It would cause a massive amount of churn: we have about 2000 uses of mtod() in the tree, and downstream trees will have many thousands more. I think such a change deserves a wider discussion on -arch or so.

[emaste]

I agree with mark - this will be a nice cleanup and I think should be done, but will take some careful coordination with downstreams, and ports.

I would suggest that we add convert_mtod.cocci to the tree (somewhere under tools/ perhaps) and give an example of its use to help downstream consumers.

[cem]

Both remarks seem reasonable to me. I wasn’t going to drop this in the tree without circulating the idea past net@ at a minimum 😀. But I wanted some feedback from a smaller audience first— thank you both.

One additional possibility would be to define mtod in such a way that either 1 or 2 argument versions continue to compile, as an easier onramp for out of tree consumers. (I believe this is possible but haven’t yet confirmed so.) It would be an ugly macro hack, but importantly the ugliness would just be in one place. If that is possible, do either of you have an opinion on whether that seems worse or better than requiring a flag day for out of tree consumers?

[markj]

In D21669#473159, @cem wrote:

Both remarks seem reasonable to me. I wasn’t going to drop this in the tree without circulating the idea past net@ at a minimum 😀. But I wanted some feedback from a smaller audience first— thank you both.

One additional possibility would be to define mtod in such a way that either 1 or 2 argument versions continue to compile, as an easier onramp for out of tree consumers. (I believe this is possible but haven’t yet confirmed so.) It would be an ugly macro hack, but importantly the ugliness would just be in one place. If that is possible, do either of you have an opinion on whether that seems worse or better than requiring a flag day for out of tree consumers?

I think having a macro compatible with both uses would be much easier to adopt. The only real downsides are that the macro is uglier (not a big deal), and that inconsistencies in its use would be confusing (not really an issue if we can just convert in-tree code using coccinelle).

[cem]

Here's what the 1 or 2-argument macro looks like (warning: highly ugly, but it works): https://godbolt.org/z/4qBp7t . Will update review shortly.

Shamelessly ripped off from Gregory Pakosz on Stackoverflow: https://stackoverflow.com/questions/1872220/is-it-possible-to-iterate-over-arguments-in-variadic-macros

[cem]

• Add mtod variant that takes prior two arguments.
• Place coccinelle script in the tree under tools.

The kernel actually compiles as-is (i.e., ip_options.c converted and the rest
left using the 2-argument variant). I think this demonstrates the variadic
macro is functional.

[emaste]

Ugly, but amazing. I would suggest committing the macro change first followed by the refactoring.

[emaste]

And mbuf.9 change along with mbuf.h.

[cem]

CC @bz

Suggestions of the right mailing list to circulate this on welcome.

[cem]

CCing some networking folks, names kindly provided by Ed.

[emaste]

I would suggest describing this plan on freebsd-net@ and freebsd-arch@ mailing lists

[rrs]

Well I am going to voice a different view. I do *not* see this as a nice cleanup. I see it
has a lessening of information.

Has a code reader if you see

i = mtod(m, struct ip *)

You *know* that i is a struct ip *.

Now if I can just do

i = mtod(m)

You have less information at that point in the code. Yes in a lot of
places in the network stack we do the

ip = mtod(m, struct ip *)

somewhere at the top so its obvious. But the problem is that
whats in the inbound ip changes and down the road in the code
I may do

p = mtod(m, struct sctphdr *);

or

p = mtod(m, struct tcphdr *);

After doing some sort of m_adj of course...

To me this makes the mtod macro provide *less* information. Yes I can go back in the code
and lookup what a p was, But thats a detriment not a plus in my thinking.

[cem]

In D21669#476596, @rrs wrote:

Well I am going to voice a different view. I do *not* see this as a nice cleanup. I see it
has a lessening of information.

Has a code reader if you see

i = mtod(m, struct ip *)

You *know* that i is a struct ip *.

Hi Randall,

The automated semantic patch converts instances of i = mtod(m, struct ip *) to i = (struct ip *)mtod(m), generally. Is it not just as clear that i is a struct ip * afterwards?

There are limited exceptions where the cast is dropped automatically — at declaration, where the type of i is already present; and when type is void *. It would be possible to make the same macro change without one or both of these exceptions, if you see either of them as helpful.

I think it would be perfectly fine for the network stack to retain a style convention of explicit casts of mtod() (which you effectively have with the 2-argument version, today) indefinitely. I had no plans to drop the explicit casts in any existing code after this conversion.

Best,
Conrad

[cem]

In D21669#476223, @emaste wrote:

I would suggest describing this plan on freebsd-net@ and freebsd-arch@ mailing lists

Sent; I'm in the moderation queue on net@, though.

[tuexen]

I'm contemplating what we gain by this change... We get a much more complex definition of mtod() and, for example, in the case of the SCTP source, which are shared with other BSD like platforms which don't make that change, we have to provide a way to do it still the old way. I guess, I have to replace mtod() by SCTP_MTOD() with the 2 argument semantic in the SCTP sources and then make platform specific definitions: On FreeBSD, call your version and do the explicit cast, on the other platforms just call mtod(). Another point is that these mechanical global changes might make MFCing code harder...

So in my view, there is a price to pay. So I would like to understand what we gain.

[cem]

In D21669#476636, @tuexen wrote:

I'm contemplating what we gain by this change... We get a much more complex definition of mtod()

The complexity only comes with backwards compatibility for the 2-argument variant. With a flag day conversion to 1-arg, there isn't really any additional complexity.

and, for example, in the case of the SCTP source, which are shared with other BSD like platforms which don't make that change,

That seems like a self-inflicted SCTP problem. If SCTP is not native FreeBSD software, maybe it shouldn't live in the tree. If it is, it should not stand in the way of FreeBSD refactoring. It could use a trivial compatibility definition ifndef FreeBSD.

we have to provide a way to do it still the old way. I guess, I have to replace mtod() by SCTP_MTOD() with the 2 argument semantic in the SCTP sources and then make platform specific definitions: On FreeBSD, call your version and do the explicit cast, on the other platforms just call mtod().

I don't think it's as complicated as that.

Another point is that these mechanical global changes might make MFCing code harder...

Yes, that's true. That's what the complex definition is for. It helps, but does not remove merge conflicts.

So in my view, there is a price to pay. So I would like to understand what we gain.

If it doesn't make sense to you I don't think I can help you with that.

I think we should move away from these kinds of macros generally, and the gigantic functions we tend to have as well, but I don't see much enthusiasm in networking for that kind of thing. So I guess I'll butt out.

[gnn]

Coming back to this after a discussion with cem@ and emaste@ I think this idea still deserves some merit. It makes the resulting code easier to read, at least to my eyes and removes a somewhat confusing and error prone macro from the code base. It would be easy enough to propose the same change to the other BSDs as well.

[tuexen]

In D21669#546629, @cem wrote:

In D21669#476636, @tuexen wrote:

I'm contemplating what we gain by this change... We get a much more complex definition of mtod()

The complexity only comes with backwards compatibility for the 2-argument variant. With a flag day conversion to 1-arg, there isn't really any additional complexity.

OK. Then I would vote for a flag day.

and, for example, in the case of the SCTP source, which are shared with other BSD like platforms which don't make that change,

That seems like a self-inflicted SCTP problem. If SCTP is not native FreeBSD software, maybe it shouldn't live in the tree. If it is, it should not stand in the way of FreeBSD refactoring. It could use a trivial compatibility definition ifndef FreeBSD.

It is just a problem of supporting multiple platforms with the same code. But it can be handled by #ifdefs. And I agree, it should not stand in the way of generic code evolution.

we have to provide a way to do it still the old way. I guess, I have to replace mtod() by SCTP_MTOD() with the 2 argument semantic in the SCTP sources and then make platform specific definitions: On FreeBSD, call your version and do the explicit cast, on the other platforms just call mtod().

I don't think it's as complicated as that.

OK. Maybe I find an easier way.

Another point is that these mechanical global changes might make MFCing code harder...

Yes, that's true. That's what the complex definition is for. It helps, but does not remove merge conflicts.

So in my view, there is a price to pay. So I would like to understand what we gain.

If it doesn't make sense to you I don't think I can help you with that.

I'm not saying that it doesn't make sense. I just wanted to know what you think is the gain.

I think we should move away from these kinds of macros generally, and the gigantic functions we tend to have as well, but I don't see much enthusiasm in networking for that kind of thing. So I guess I'll butt out.

I think these are two points:

• macros involving type casts
• gigantic functions

I guess the first can be solved by small steps. The second is harder to not introduce regressions, but they are independent. Or am I missing something?