Date: Wed, 7 Feb 1996 15:10:40 -0700 (MST) From: Terry Lambert <terry@lambert.org> To: julian@ref.tfs.com (Julian Elischer) Cc: hackers@FreeBSD.org Subject: Re: TERRY patches.. Message-ID: <199602072210.PAA06511@phaeton.artisoft.com> In-Reply-To: <199602071118.DAA00521@ref.tfs.com> from "Julian Elischer" at Feb 7, 96 03:18:02 am
next in thread | previous in thread | raw e-mail | index | archive | help
I responded to this via email; I was unaware that it had been repeated
here. I will respond in detail for the benefit of other readers; I
believe I can justify all of my coding decisions, if that's required.
> 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.
This is (apparently) a result of a bad interaction with SUP and CVS
with regard to the ability to do a "cvs update" to a locally modified
tree and have the changes correctly merged. ("M" logging case).
Anyone who does not have tree commit priviledges and is using SUP+CVS
with the SUP firing off on a regular basis should be aware of potential
problems here.
For those of you with commit priviledges who commit to the main tree
priort to doing another local SUP, this will not be a problem, and can
be safely ignored.
> 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 EXCLUDE chnages are to handle the cache of a CREATE op used when the
code issuing the namei() expect the file to not exist. This is a logical
interface push-down. from the vfs_syscalls.c code into the vfs_lookup.c
code to maximize code reuse. It incidently cleans up about 50 lines of
garbage in the vfs_syscalls.c for backing out operations that "shouldn't
have succeeded for an existing file".
> 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
These changes get the number of elements in the opv descriptor vectors
to be uniformly allocated for all file system instantiations from the
vnode_if.c file, which has an agregate initialized "complete" structure
for the value of vfs_opv_numops.
Previously, the value of vfs_opv_numops was calculated during
initialization of the first static file system that was declared
in the agregate linker set for static file systems. This was incorrect
for several reasons:
1) In order to add ops to the operation vector, all file systems
were required to be rebuilt. This is because the operation
vector list must be at least as large (and a full intesection
set) of the total number of available operation vectors for
correct initialization by iterating the first list.
2) The use of a static list during the initialization assumes
the existance of statically linked file systems. This is
a bad assumption. In vfs_vnops.c, it is possible to provide
an overide for the entire VFS system (in fact, this is done
[and done incorrectly] for sockets and pipes). That means
that it is theoretically possible to produce a working kernel
with no VFS (Heidemann framework) file systems at all. This
may, in fact, be desirable. It would allow (with segment ID
tagging) the temporary use of a "boot FS" type different
from the root FS type. Consider NFS, RAMdisk via Novell and
Microsoft remote boot facilities, and "hosted" (DOS partition)
based boots. Also consider "drop-in" replacement of other
UNIX and UNIX-clone system kernels (SCO, UnixWare, etc.).
3) The information was already available without the extra code
for the calculation.
As far as the other implications, it is now possible to call-back
register and deregister file system types and FS mount operations.
This paves the way for demand-loading of FS kernel modules as a result
of file system "arrival" rather than an explicit user-initiate mount
request.
> The reworking of mount() is ingenious, but I think we should not
> impliment much of it.
The intention here is to allow "device arrival" trigger mounts. Consider
a portable computer with intermittent NFS connectivity, intermittent
instertion of FlashRAM or PCMCIA Type II and Type III storage device
and controller interfaces, and intermittent connectivity to a docking
station.
Consider also the desktop system with the intermittent insertion of
removable media: Syquest, ZIP, CDROM, Floppy, or other media.
The changes also remove the dependency on root heirarchy. The final
intent is to mount as volumes and insert the volumes into a framework
hierarchy by sorted order from the mount-list.
The reasoning here is that you may wish to have automounting, but you
may not want auto-coverage of mount points. The specific example I
had in mind for this was the devfs during boot-stage before root
remount.
> 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 agree with this. I frequently find a "goto" clearer than a nested
control structure with a break. There are three "obfuscated" cases
in the vfs_syscalls.c file: open, rename, and ogetdirentries. The
open and rename code have always been obfuscated. The open code wants
t be croken out into seperately handled "open" and "create" cases, and
the rename code wants to be broken on the basis of source directory
vs. non-directory and target. The ogetdirentries is just plain ugly,
mostly as a result of the compatability with the "old" vs. "new" file
system formats and the "endianess" dependencies in the directory
tagging. The correct way to handle this is to use bit-fields in the
structure that are endian-dependent instead of all the crap-cases in
the code where endianess is tested and swapping done. The ogetdirentries
code wants a rewrite, but I should not be faulted for this: I did not
cause it to need a rewrite in the first place, CSRG did.
With no further work occuring on the Lite2 code (CSRG is not doing
another release), the major compatability concern is NetBSD.
> 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..
This is should happen in any case. Much of the kernel code is poorly
documented.
> 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.
I agree. I would argue the percentages re: the "cosemetic effect" of
the changes to functions not calling namei() with an op or flag
causing the path buffer to be returned instead of freed or the 5
cases where the EXCLUDE flag actually provides a real code simplification.
I would put therese changes at 20-25% (not the 60% you quote) and I
would say that they are necessary for future work in SMP and kernel
multithreading with the least impact to the code (by allowing conditional
exclusion of the synchronication primiteives, per Garrett Wollman and
other core team members requests).
I would also note that with the changes I have introduced, along with
existing divergence from the 4.4BSD code already in that file, I believe
we are close to the point where it is legally allowable to remove the
USL copyright on the file (this is a bonus benefit of not inconsiderable
value).
As I stated in provate emil, I would be happy to work with you on
removing the 20-25% of the "cosmetic" changes, either replacing code
reordering with "goto error" clauses, or otherwise addressing the
reentrancy syncronization for the SMP/multithreading case in the
"offending" functions.
> The changes to vfs_syscalls seem to be largely cosmetic or
> changes to lead to single-exit-point structuring of functions.
The "or" is the intended case. 8-).
> 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;
I agree here. This particular case (I remember debating the comment)
was because of ethnic purity for the getfsstat() callers. This is
technically an unnecessary change, but I don't like modifying return
values if I don't have to.
Really, this should be reworked, probably as a "goto error" in place
of the "break" + "if( ...". Or the "if" should be "if( !error".
> 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?
Yes.
> the nfs_nameifree() changes look ok, but I
> really need to spend more time..
>
> why is it different to nameifree()
Than to free the cn_pnbuf directly via a free? Aside from the fact
that "free" and "FREE" were used interchangably, and the header files
make them non-interchangeable in the presence of certain compilation
options?
Because it makes the buffer an opaque object. It means I can have as
many versions of the buffer and pointers in the structure as I want
without compromising the code that calls it.
For instance, I might add a terminal path component buffer to the
structure (which is also allocated in namei and freed in nameifree)
to supply an 8.3 name to the underlying FS for VFAT. So I pass down
a vreate request for "LongFileName.txt" and a terminal component of
"LongFi~1.txt" (matching the VFAT collision semantics) and run the
collision resoloution code in the VFAT FS instead of having to do
it by multiple additional VOP calls to the underlying FS in vfs_lookup.
And that's one example, not even touching the issue of Unicode or
a Samba server that serves Win3.11 and Win95 clients and wants to
give both sets of names back to the clients, or an API allowing
system calls to take Unicode and 8 bit file names both (translation
is necessary for both NFS clients and servers).
Etc., etc.
Let me know what I need to do to work out the "cosmetic" changes to
your satisfaction.
Regards,
Terry Lambert
terry@lambert.org
---
Any opinions in this posting are my own and not those of my present
or previous employers.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199602072210.PAA06511>