Programming

13384 readers

1 users here now

All things programming and coding related. Subcommunity of Technology.

This community's icon was made by Aaron Schneider, under the CC-BY-NC-SA 4.0 license.

founded 2 years ago

MODERATORS

remington

alyaza

Hexorg

A fun exercise in re the design quality of the Linux kernel: How many lines of code do you think was the diff to add "-o nosymfollow" (ignore symlinks) as a mount option? (git.kernel.org)

submitted 9 months ago* (last edited 9 months ago) by mozz@mbin.grits.dev to c/programming

4 comments fedilink hide all child comments

Think of a similar scope of change to a large codebase you're familiar with, for frame of reference.

you are viewing a single comment's thread
view the rest of the comments

[–] TheOctonaut@mander.xyz 8 points 9 months ago (1 children)

Little bit unfair as this was already an existing thing that got a new way to be triggered rather than a completely new feature needing code to handle not following symlinks.

To accurately guess you'd need to know that "don't follow symlinks in this particular scenario" already exists and we're just adding an OR to an if statement.

[–] mozz@mbin.grits.dev 4 points 9 months ago* (last edited 9 months ago) (1 children)

You have misunderstood. No, it wasn't an existing thing. This is the code that implements it. That's the point.

The change to fs/namei.c is the code to handle not following symlinks; the rest is some necessary code to create the option and expose it to userland.

(Edit: Rereading I do see a little better what you were saying - I actually looked it up and the code that “originally” implemented not following symlinks, that you’re saying we're now adding an or statement to activate, was 2 lines to expose the option, 2 lines of white space, and 2 lines to implement not following symlinks).

[–] TheOctonaut@mander.xyz 1 points 9 months ago (1 children)

Yeah I get that but you dig deeper and that implantation was just throwing an error that needs to be handled elsewhere. The "real" code is what is handling that error.

But then we're back to act acknowledging a meaningful point of having commits that do one thing and do it well and understandably, and I'm back to appreciating the difference between the kernel and our app.

[–] mozz@mbin.grits.dev 1 points 9 months ago

There is no code somewhere else that needed to be written somewhere else to handle the error. It just started working because those lines were added. That's what I keep saying. That is the nature of the good design I am pleased by to give the example.

I'm such a stubborn person that I literally had a look in namei.c to see what happens from that return, to make sure. I was fairly confident that that's how it works, just because it's rare in kernel development to add a feature without the means to trigger it and then add the means to trigger it in a separate patch (why would it not just all go in at once?), but I had a look and here's what I saw:

get_link() returns ELOOP
In one case, vfs_readlink() returns an error straight to the user after going through the VFS layer
In the more complex case where we were actually trying to follow it to do some other operation, pick_link() sees the error and returns an error
Which is what gets returned from step_into()
step_into() gets called from a bunch of places, but I looked at them briefly and quickly searched for ELOOP and NOSYMFOLLOW and convinced myself pretty well that there isn't anywhere that handles the error any differently than it would any other we-can't-do-this error, and ultimately it's going to just return the failure to the user. Because why would it do anything different? It looks to me although I'm not sure like step_into() up to walk_component() up to link_path_walk() up to path_lookupat() or path_openat() is sort of the main path you'd get to from userspace via do_file_open() or similar functions, and they all plan to just return the refusal to follow the symlink back to userspace.

Where are you getting your assertion that obviously it's already implemented somewhere else separate from this patch?