Page MenuHomeFreeBSD

Improve error message for exports lines
ClosedPublic

Authored by walker.aj325_gmail.com on Apr 26 2023, 8:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 16, 5:07 AM
Unknown Object (File)
Sat, Mar 16, 5:07 AM
Unknown Object (File)
Sat, Mar 16, 5:06 AM
Unknown Object (File)
Tue, Mar 12, 9:44 AM
Unknown Object (File)
Tue, Mar 12, 9:44 AM
Unknown Object (File)
Tue, Mar 12, 9:44 AM
Unknown Object (File)
Tue, Mar 12, 9:44 AM
Unknown Object (File)
Fri, Mar 8, 5:33 AM
Subscribers

Details

Summary

Currently mountd will print error message "symbolic link in export path or statfs failed" in case some path component in an exports line fails validation. This revision improves the error message by giving more information about the precise error as well as the path component that caused the issue.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This revision is now accepted and ready to land.Apr 27 2023, 1:37 PM

Made a couple of inline suggestions, but the code does
look correct to me.
I assume you do not have src commit bit, given your login name?

If that is the case..mav@ are you going to commit this or should I?
(I think it can qualify as a bug fix and go into main during code slush.
Do you guys agree?)

usr.sbin/mountd/mountd.c
1640–1641

Minor nit. Although not needed for correctness,
I think setting err_msg = NULL here might be useful
as an extra safety belt and for readability.
(I'd only initialize it to NULL
when declared if the compiler complains it "might not
be initialized".)

But I'll leave this change up to you.

3829

I'd update this comment to accurately note what this
function does. Then I would put an updated comment
on check_dirpath() as follows:
check_dirpath() now also does
statfs(), which is not a part of checking for a symbolic link,
so I'd say the comment for check_dirpath() should mention
that as well as checking for a symbolic link.

usr.sbin/mountd/mountd.c
3857

Since statfs() is really a separate operation from
checking for a symbolic link, you might want to move
the above code block into a separate function and call it
after check_dirpath(), similar to how the unpatched
code called check_dirpath() and then statfs().

Just a suggestion.

Moved statfs check to different function. Updated comments. Initialized err_msg to NULL on each loop iteration.

This revision now requires review to proceed.May 4 2023, 12:34 PM

Except for the minor nit that "ret" no longer
does anything and can be removed, it
looks fine to me now.

Can you commit it or do you want me to do so?
(If I do it, I'll put a submitted by on it.)
If I'm committing it, I'll delete "ret".

Thanks for the patch, rick

usr.sbin/mountd/mountd.c
3833–3834

You could get rid of "ret", since it no longer
does anything.

This revision is now accepted and ready to land.May 4 2023, 1:57 PM

After some discussion with Mav, I switched to initializing err_msg to NULL and resetting it after free() if we encounter an error. Removed now-unused variable.

This revision now requires review to proceed.May 4 2023, 5:29 PM

Thanks for the feedback Rick. I unfortunately don't have commit rights and someone else will have to do that for me.

Fix undefined reference that crept in while applying suggested changes.

The "int ret = 1;" appears to still be in the patch.
I suspect that is just uploading the wrong version.
Trivial to fix during commit.

So mav@, do you want to commit it or shall I?
(Also, if you have any comments w.r.t. the current
version, please do so.)

This revision is now accepted and ready to land.May 5 2023, 1:14 PM
This revision now requires review to proceed.May 5 2023, 3:27 PM

Looks fine to me. I'll wait to hear from mav@
w.r.t. which one of us will commit it.

Again, thanks for the patch, rick

This revision is now accepted and ready to land.May 6 2023, 2:43 PM
This revision was automatically updated to reflect the committed changes.