Page MenuHomeFreeBSD

Remove name length limitation from autofs(5).
ClosedPublic

Authored by trasz on Feb 13 2016, 11:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 13, 6:19 AM
Unknown Object (File)
Tue, Dec 10, 1:24 PM
Unknown Object (File)
Nov 8 2024, 8:30 AM
Unknown Object (File)
Nov 7 2024, 7:18 AM
Unknown Object (File)
Nov 4 2024, 7:41 PM
Unknown Object (File)
Nov 3 2024, 5:31 AM
Unknown Object (File)
Nov 2 2024, 8:08 AM
Unknown Object (File)
Oct 29 2024, 12:28 PM
Subscribers

Details

Summary

Remove name length limitation from autofs(5). The linear search with
strlens is rather suboptimal, but it's a temporary measure that will be
replaced with red-black trees later on.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

trasz retitled this revision from to Remove name length limitation from autofs(5)..
trasz updated this object.
trasz edited the test plan for this revision. (Show Details)

Fix partial reads, and the namelen problem debugged by Mike Hibler.

I have absolutely no intent of digging into the bug report and debugging the issue to understand what the problem is.

If you do not bother to explain what is the problem and what is the fix, why should I spend time on this ?

Ah, sorry. Basically, for simplicity autofs used fixed length dirents, and this means there's a name length limit. This patch makes it use variable-length dirents, thus removing the name length limitation.

Overall it looks fine.

sys/fs/autofs/autofs_vnops.c
352 ↗(On Diff #14196)

Could the magic '4' constant replaced by __alignof(struct dirent) ?

365 ↗(On Diff #14196)

You do not need this memset, you only need to clear the unfilled portion of the memory after the factual end of the string, which is 3 bytes max.

375 ↗(On Diff #14196)

Please remove all blank lines from the function (and other places touched by the patch), except the blank line after locals declaration block. Style(9) requests blank line to delimit the scope of the code, explained by previous multi-line comment.

384 ↗(On Diff #14196)

The KASSERT is rather useless.

435 ↗(On Diff #14196)

What does this check try to achieve ?

(I agree with the parts I didn't comment upon, btw.)

sys/fs/autofs/autofs_vnops.c
384 ↗(On Diff #14196)

It was supposed to quiet down static analysis tools, like Coverity, but on the other hand such a tool should be smart enough to figure out that there can be no error returned here. So ok, I'll remove it.

435 ↗(On Diff #14196)

It's to detect whether we should copy out the ".." entry or not, based on the current offset into the directory.

sys/fs/autofs/autofs_vnops.c
435 ↗(On Diff #14196)

This should be explained in a comment.

454 ↗(On Diff #14196)

BTW, is the list stable for two disjoint calls to find the same children at the same offsets ?

sys/fs/autofs/autofs_vnops.c
454 ↗(On Diff #14196)

Yes. The directories are append-only.

trasz edited edge metadata.

Remove magic constant, don't zero out things that are overwritten
anyway, remove useless KASSERT, and improve comments.

I've tried removing the whitespace, but it makes things harder
to read for me. Sorry, it's a personal thing; I just prefer to have
more whitespace.

sys/fs/autofs/autofs_vnops.c
429 ↗(On Diff #14275)

The comment still does not explain the check in if(), it restates what the code does inside the 'then' block.

Update comments.

sys/fs/autofs/autofs_vnops.c
429 ↗(On Diff #14275)

Ok, I've updated the comment on the first block (the one for ".") to mention the offset check, and that it applies to the others.

kib edited edge metadata.
This revision is now accepted and ready to land.Mar 12 2016, 4:29 PM
This revision was automatically updated to reflect the committed changes.