Skip to content

Conversation

@DerDakon
Copy link
Member

@DerDakon DerDakon commented Nov 9, 2020

Split out from #153. The .a version has the advantage that the linker can drop the strings it doesn't need. The .o version has all usernames in all affected binaries, but has much simpler build rules.

@schmonz
Copy link
Member

schmonz commented Nov 10, 2020

Sorry it took me this long to notice, but these aren't IDs in the usual Unix numeric sense (and the sense in which pre-notqmail qmail was terribly annoying). They're user and group names. So maybe usersgroups.o? Or possibly auto_usersgroups.o since things generated with that mechanism currently get that prefix.

(Sorry.)

While I'm here, at a glance, most files that link uid.o also link gid.o. Maybe let's merge to uidgid.o (or id.o) as part of this PR?

(Double sorry.)

@DerDakon
Copy link
Member Author

The advantage of putting all these things in a .a would be that the resulting binaries will not grow. Adding uid.o and gid.o to that archive also is trivial then.

@schmonz
Copy link
Member

schmonz commented Nov 10, 2020

The users/groups and the corresponding numeric lookup routines do mostly get linked together. The .o make rules as presented here are nicely shrunken, but I think the savings from

  1. Merging uid.c and gid.c into uidgid.c and
  2. Linking uidgid.o into auto_usergroup.a

would be about as good. And then the .a link-time benefit you mention means we'd avoid any unwanted changes to binaries. So I think I finally have a strong opinion. 😜

@DerDakon
Copy link
Member Author

So it boils down to only have the first commit, and add uid.o and gid.o to the same archive, right?

@schmonz
Copy link
Member

schmonz commented Nov 11, 2020

So it boils down to only have the first commit, and add uid.o and gid.o to the same archive, right?

Yes, I’ve finally landed on a preference for the approach in your first commit plus including those symbols in the same archive. Before you do that, try merging uid.c and gid.c into one .c file that generates one .o file, and then include that one additional .o in the archive. This would reduce some more duplicated make and C boilerplate without introducing any new symbols in binaries, if I understand correctly.

@DerDakon
Copy link
Member Author

It would introduce new symbols in qmail-rspawn and qmail-queue as they don't contain init_gid() yet. The linker can drop that only if it comes from it's own object file (not talking about -flto or advanced features like that).

@schmonz
Copy link
Member

schmonz commented Nov 11, 2020

Ah, okay. Then yes, exactly as you had suggested, please :-)

Copy link
Member

@schmonz schmonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just these nits left to pick

Copy link
Member

@schmonz schmonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retracting my remaining nitpicks about ordering. Having observed that replacing a list of .o leaves the new .a in between some remaining .o, you're moving the new .a near its siblings. I understand your intention now.

Separate commits would make this more apparent, but we've already workshopped this one more than enough! I agree that git diff --word-diff suffices to see what's happening here.

This PR is a terrific clarification of the make rules. It's so much easier to grok dependencies at a glance. qmail should have always looked up numeric IDs at runtime, and then it should always have been built this way.

Copy link

@josuah josuah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds 0 portability cost and much readability.

@DerDakon DerDakon merged commit aedd8be into master Nov 14, 2020
@DerDakon DerDakon deleted the Dakon-ids-object branch November 14, 2020 08:23
@schmonz schmonz modified the milestones: 1.90, 1.09 Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants