--GvXjxJ+pjyke8COw
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Dec 21, 2005 at 07:04:13PM +0100, Reinoud Zandijk wrote:
>=20
> The second patch is my initial commit of my UDF filingsystem implementati=
on=20
> for CD and DVD. It is dependent on the 1st patch as it uses the ioctl's t=
o=20
> avoid it having intimate knowledge of SCSI internals. Its currently=20
> supports read access to all media types that CD/DVD type drives can=20
> recognize including DVD-RAM and BD- drives. Write access is planned and i=
n=20
> preparation. To facilitate this some hooks are present in the code that a=
re=20
> not strictly needed in a read-only implementation but which allow writing=
=20
> to be added more easily. UDF versions upto the latest 2.60 are to be=20
> supported though due to lack of test media version 2.50 and 2.60 are not=
=20
> implemented yet though easy to add. Both open and closed media are=20
> supported. Support for harddisc partitions and optionally for files (like=
=20
> using vnd) is not implemented yet though on my TODO list.

Ok, here are some initial comments. I see that some issues have been=20
addressed by Hubert and Christos.

+UDF is a filingsystem that is defined by the OSTA standardisation group an=
d is
+specified especially for data interchange on optical discs like CDs and DV=
Ds
+between different operating systems. More and more its also found on other
+media like Compact Flash (CF) cards.

As noted, "file system". Also, I suggest changing that paragraph around.=20
The "between" phrase applies to "data interchange" and not "like CDs", but=
=20
the latter is in the way. Also, "such as" sounds much better in formal=20
text than does "like." So:

UDF is a file system defined by the OSTA standardisation group. It was=20
designed for data interchange between different operating systems using=20
optical media, such as CDs and DVDs. It is also used on other media, such=
=20
as Compact Flash (CF) cards.


Oh, minor point. If you want to contract "it is", you get "it's". "Its" is
the possessive of "it". You can remember this as "his, her, its" (as in
his car, her car, its car).

Another minor point, if you transfer copyright to TNF, I thought you then=
=20
can no longer claim copyright on the file. So if you're contributing this=
=20
(which I think is great!), you shouldn't also have your copyright on=20
there. Or at least the checked-in code shouldn't [could be useful to=20
leave in until the transfer happens].

In udf_mount(), I'd suggest catching update mounts earlier. Also, your=20
UPDATE error routine doesn't vput().

The (error) {  } block below open and udf_mountfs() calls vn_lock(). But=20
if the open fails, the vnode will still be locked. Does udf_mountfs()=20
unlock the node? VOP_UNLOCK() + vrele() =3D=3D vput().

Oh. Your namei() call doesn't set LOCKLEAF. Which is why the vn_lock()=20
call is ok for now. But VOP_OPEN() is supposed to be passed a locked=20
vnode(). So you need to change the lookup, and change udf_mountfs() so it=
=20
takes a locked vnode. Then if udf_mountfs() returns the node unlocked, you=
=20
need to adjust things to clean up correctly for the VOP_CLOSE() (which=20
also should get a locked vnode).

Will a future version be using SYSTEM vnodes? If not, I'd say rip that=20
part out.

Why do you have udf_rwXX() routines? Use htole() or letoh().

VFS_VGET() is not used by NFS. Currently, it is only used by fs-internal=20
routines. Its main use is so that ufs can use the same lookup code on lfs=
=20
and ffs.

VFS_FHTOVP() is not used to look up "the NFS file", it is used to look up=
=20
the file for the specified file handle. More than just NFS uses file=20
handles. :-)

VFS_VPTOFH() returns a file handle, not an NFS file handle... As above.

udf_inactive() would only need to do something if the fs is r/w and the=20
file has been removed. Metadata flushing is also good.

udf_strategy(): Remove the XXX. Also, the write case handling needs=20
cleaning up; unindented printf()s aren't ok. :-)

udf_readdir(): Is '..' always the first node in the UDF stream? You fix up=
=20
'.', so I wonder if '..' needs work too. Unfortunately some code wants '.'=
=20
and '..' to be the first two nodes. It shouldn't, but it does.

udf_lookup(): In the '..' path, you should set PDIRUNLOCK as soon as you=20
unlock the parent. islastcn & such don't matter. Then clear it when=20
relocking the parent succeeds.

Suggestion: do the udf_rwXX translation when you read/write a file entry,=
=20
rather than each time you access the in-core data. The second time you=20
stat a file (on a BE system), you save ops.

udf.h: struct file_entry and struct extfile_entry should have "udf" or=20
"udf_" (or something like it you prefer) prefixed to the name. When=20
looking at the code, I first thought it was a NetBSD-global definition.=20
:-(

udf_link: "/* XXX Locking status of dvp does not match manual page. */"=20
should not appear in the file. :-) Fix the locking to match kern/vnode.src=
=20
please. :-) Or if you did that and the man page is off, let's fix the man=
=20
page.

Ok, unfortunately I have to go. I didn't get any farther than through=20
udf_vfsops.c and part way through udf_vnops. I think we need more review.

As I know nothing about UDF, I can't make comments on it. :-)

Take care,

Bill

--GvXjxJ+pjyke8COw
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3 (NetBSD)

iD8DBQFDqgDKWz+3JHUci9cRAjCYAJ0faB1DXbrwlg5zNbknKQrjkFVtPACdGByB
drMcvfQOPn2ZP6js+0IPZZE=
=Pdy/
-----END PGP SIGNATURE-----

--GvXjxJ+pjyke8COw--