Date: Wed, 7 Feb 1996 03:18:02 -0800 From: Julian Elischer <julian@ref.tfs.com> To: hackers@freebsd.org Subject: TERRY patches.. Message-ID: <199602071118.DAA00521@ref.tfs.com>
next in thread | raw e-mail | index | archive | help
Terry:
regarding your patches..
Your changes accidentally SPAMMed some bde patches..
I've sent you a reverse patch to apply to your tree to UNSPAM the changes
having applied that patch.. here are comments.
I like the namei/lookup addition of the EXCLUDE
flag. Seems good to me.. the moving of some functionality from filesystems
to common code is a good idea.
The changes in vfs_init seem properly implimented, but I haven't looked
at the full impact of what they mean yet in terms of how the structures are
controlled etc... later
The reworking of mount() is ingenious, but I think we should not
impliment much of it.
I find it is harder to follow the nested if else
constructions than it is to see
return EINVAL;
or even
error = EINVAL;
goto leave;
We also lose the ability to get easy diffs from 4.4lite2
and NetBSD (who we may want to campare code with at times)
I think we would gain more by leaving the existing code "As IS"
and inserting 4-line comment groups every clause, explaining what's going on..
I think 'cosmetic reworks' should be avoided, as it tends to:
1/ make it harder to merge/compare code
2/ obfuscate the real nature of the change.
Admittedly I do prefer
mp->mnt_flag |= uap->flags & (MNT_NOSUID |
! MNT_NOEXEC |
! MNT_NODEV |
! MNT_SYNCHRONOUS |
! MNT_UNION |
! MNT_ASYNC |
! MNT_FORCE);
to
! mp->mnt_flag |= uap->flags & (MNT_NOSUID | MNT_NOEXEC | MNT_NODEV |
! MNT_SYNCHRONOUS | MNT_UNION | MNT_ASYNC | MNT_FORCE);
The changes to vfs_syscalls seem to be largely cosmetic or
changes to lead to single-exit-point structuring of functions.
some of them look good but..
In some cases it actually complicates the code to come out through a
single exit point:
wittness:
/* only set *retval if loop above completed successfully*/
you needed extra code to use the break;
Once again, is it worth breaking 'compatibility' to get
the meger benefites.. Whether the new format of the code
is more maintainable is open to debate.. I might find it
harder to maintain because I could get the 'else' clauses wrong
and also sometimes it requires some extra work as it assignes a value
to the variable 'error' rather than just returning it as a value,
not to mention just returning.. (I seem to remember that sometimes the
C compiler will notice that you are 'jumping' to a return,
and will do a return(value) anyhow..)
around line..--- 813,829 ----
a whole
NDINIT(&nd, CREATE, LOCKPARENT, UIO_USERSPACE, uap->path, p);
! error = namei(&nd);
seems to have dissappeared..
is that right?
Is that the same namei moved down 23 lines or so?
That at least uses the EXCLUDE flag and is more than a cosmetic change....
It's a pitty you couldn't submit the cosmetic changes as a separate packege
because they Hide the extent of REAL changes..
It's make it a much easier job to go over the actual functional changes..
I wanted the namebuffer freeing to be moved out as you have done.
it seemed to be in the wrong place when I did devfs..
but about 60% of this patch is cosmetic.. (single-exit-point stuff)
I f I could get the real changes separated from this stuff it's be
a lot easier.
the nfs_nameifree() changes look ok, but I
really need to spend more time..
why is it different to nameifree()
julian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199602071118.DAA00521>