On Tue, Jul 15, 2003 at 05:43:24AM -0500, Marcus Brinkmann wrote:
> 1. What errors do you still expect from the second time diskfs_lookup is
> invoked with REMOVE? Is there any error that is allowed at that time?
> If there is, the change is correct.
Before the second diskfs_lookup, mutex_unlock is called. This leaves a
little time when someone could remove the node.
> 2. Is it possible, and according to diskfs.h it should be, to just
> call dir_lookup with REMOVE only once, at the point you introduced it
> in your patch, and store the dirstat until it is needed for the actual
> disks_dirremove call. This saves one lookup call. Can you try such a
> change and look into the code if it is feasible?
While trying to do that now, I recalled why this cannot be done. If
diskfs_lookup is called only once in the beginning, the assertion in
ext2fs/dir.c:716 is triggered with the following commands:
$ mkdir x
$ mv x y
$ mv y x
It seems that dirstat of ext2fs can break when there are other
activities in the directory. It's possible that ext2fs is wrong, but I
didn't look further.
I send a revisited patch that I beleive is easier to read, and it fixes
some tiny bugs too.
Regards
--
Ognyan Kulev <[EMAIL PROTECTED]>
--- libdiskfs/dir-renamed.c.orig 2003-07-15 23:44:32.000000000 +0300
+++ libdiskfs/dir-renamed.c 2003-07-16 00:52:38.000000000 +0300
@@ -1,5 +1,5 @@
/*
- Copyright (C) 1994,95,96,97,98,99,2001 Free Software Foundation, Inc.
+ Copyright (C) 1994,95,96,97,98,99,2001,03 Free Software Foundation, Inc.
This program is free software; you can redistribute it and/or
modify it under the terms of the GNU General Public License as
@@ -73,9 +73,7 @@ diskfs_rename_dir (struct node *fdp, str
{
error_t err;
struct node *tnp, *tmpnp;
- void *buf = alloca (diskfs_dirstat_size);
- struct dirstat *ds;
- struct dirstat *tmpds;
+ struct dirstat *ds, *tmpds;
mutex_lock (&tdp->lock);
diskfs_nref (tdp); /* reference and lock will get consumed by
@@ -85,48 +83,51 @@ diskfs_rename_dir (struct node *fdp, str
if (err)
return err;
- /* Now, lock the parent directories. This is legal because tdp is not
- a child of fnp (guaranteed by checkpath above). */
+ /* Now, lock the parent directories. This is legal because TDP is not
+ a child of FNP (guaranteed by checkpath above). */
mutex_lock (&fdp->lock);
if (fdp != tdp)
mutex_lock (&tdp->lock);
/* 1: Lookup target; if it exists, make sure it's an empty directory. */
- ds = buf;
+ ds = alloca (diskfs_dirstat_size);
err = diskfs_lookup (tdp, toname, RENAME, &tnp, ds, tocred);
assert (err != EAGAIN); /* <-> assert (TONAME != "..") */
-
- if (tnp == fnp)
+ if ((tnp == fnp) || (err && err != ENOENT))
{
- diskfs_drop_dirstat (tdp, ds);
- diskfs_nput (tnp);
- mutex_unlock (&tdp->lock);
- if (fdp != tdp)
- mutex_unlock (&fdp->lock);
- return 0;
+ if (tnp == fnp)
+ err = 0; /* Renaming node to itself. */
+ fnp = 0; /* Don't unlock the unlocked FNP. */
+ goto out;
}
-
- /* Now we can safely lock fnp */
- mutex_lock (&fnp->lock);
-
if (tnp)
{
+ assert (!err);
if (! S_ISDIR(tnp->dn_stat.st_mode))
err = ENOTDIR;
else if (!diskfs_dirempty (tnp, tocred))
err = ENOTEMPTY;
+ if (err)
+ goto out;
}
- if (err && err != ENOENT)
+ /* 2: Check permissions of the node being moved, and lock FNP. */
+ tmpds = alloca (diskfs_dirstat_size);
+ err = diskfs_lookup (fdp, fromname, REMOVE, &tmpnp, tmpds, fromcred);
+ assert (!tmpnp || tmpnp == fnp);
+ if (tmpnp)
+ diskfs_nrele (tmpnp);
+ diskfs_drop_dirstat (fdp, tmpds);
+ if (err)
goto out;
- /* 2: Set our .. to point to the new parent */
+ /* 3: Set our .. to point to the new parent */
if (fdp != tdp)
{
if (tdp->dn_stat.st_nlink == diskfs_link_max - 1)
{
err = EMLINK;
- return EMLINK;
+ goto out;
}
tdp->dn_stat.st_nlink++;
tdp->dn_set_ctime = 1;
@@ -157,16 +158,12 @@ diskfs_rename_dir (struct node *fdp, str
}
- /* 3: Increment the link count on the node being moved and rewrite
- tdp. */
+ /* 4: Increment the link count on the node being moved and rewrite
+ TDP. */
if (fnp->dn_stat.st_nlink == diskfs_link_max - 1)
{
- mutex_unlock (&fnp->lock);
- diskfs_drop_dirstat (tdp, ds);
- mutex_unlock (&tdp->lock);
- if (tnp)
- diskfs_nput (tnp);
- return EMLINK;
+ err = EMLINK;
+ goto out;
}
fnp->dn_stat.st_nlink++;
fnp->dn_set_ctime = 1;
@@ -188,6 +185,7 @@ diskfs_rename_dir (struct node *fdp, str
else
{
err = diskfs_direnter (tdp, toname, fnp, ds, tocred);
+ ds = 0;
if (diskfs_synchronous)
diskfs_file_update (tdp, 1);
}
@@ -195,17 +193,19 @@ diskfs_rename_dir (struct node *fdp, str
if (err)
goto out;
- /* 4: Remove the entry in fdp. */
- ds = buf;
+ /* 5: Remove the entry in FDP. */
mutex_unlock (&fnp->lock);
- err = diskfs_lookup (fdp, fromname, REMOVE, &tmpnp, ds, fromcred);
- assert (tmpnp == fnp);
- diskfs_nrele (tmpnp);
+ err = diskfs_lookup (fdp, fromname, REMOVE, &tmpnp, tmpds, fromcred);
+ assert (!tmpnp || tmpnp == fnp);
+ if (tmpnp)
+ diskfs_nrele (tmpnp);
if (err)
- goto out;
+ {
+ diskfs_drop_dirstat (fdp, tmpds);
+ goto out;
+ }
- diskfs_dirremove (fdp, fnp, fromname, ds);
- ds = 0;
+ diskfs_dirremove (fdp, fnp, fromname, tmpds);
fnp->dn_stat.st_nlink--;
fnp->dn_set_ctime = 1;
if (diskfs_synchronous)
_______________________________________________
Bug-hurd mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-hurd
