* netfs.c (netfs_attempt_sync): Move err inside the loop. Declare
final_err. On an error, store the error and go to the next item.
Check for the directory port being NULL.
---
Hello,
On Fri, Aug 14, 2009 at 05:18:59PM +0200, Thomas Schwinge wrote:
> On Fri, Aug 14, 2009 at 04:36:45PM +0300, Sergiu Ivanov wrote:
> > I agree with your statement about always syncing all nodes and, IIRC,
> > antrik has already mentioned that, and I have forgotten about that :-(
> >
> > I'm rather inclined to implement the ``last one wins'' approach, since
> > it really is simpler and more common. Please, take a look at my new
> > patch and tell me if it's acceptable.
>
> Why did you send it in a separate message by the way? Anyways, I'll post
> my few comments in here.
I just didn't think about sending it in one message. (I hope this one
is formatted properly).
> | + /* The index of the currently analyzed filesystem. */
> | + int i = 0;
> | +
> | + /* The information about the currently analyzed filesystem. */
> | + ulfs_t * ulfs;
> | +
> | + mutex_lock (&ulfs_lock);
> | +
> | + /* Sync every writable directory associated with `np`.
> | +
> | + TODO: Rewrite this after having modified ulfs.c and node.c to
> | + store the paths and ports to the underlying directories in one
> | + place, because now iterating over both lists looks ugly. */
> | + node_ulfs_iterate_unlocked (np)
>
> Having had a look at ulfs.h again: why didn't you use ulfs_iterate and
> have that one handle the locking?
I don't really need to handle any locking here. The ulfs_chain needs
to be locked only when it is to be modified, while np is already
locked by the caller.
> Also, do you really have to define ulfs -- both ulfs_iterate and
> node_ulfs_iterate_unlocked do that already, isn't it?
The problem is that at each iteration of the syncing loop I need to
(1) know whether the current filesystem is writable and (2) have the
port to its root directory. The information about filesystems is
stored in ulfs_chain (1), while ports are stored in lists of node_ulfs
associated with every node (2). ulfs_iterate* define ulfs, which
iterates over the ulfs_chain, while node_ulfs_iterate* define
node_ulfs, which traverses the list of node_ulfs associated with the
given node. I hope it's clear from my explanation that whichever
macro I choose, I'll still need to keep a separate variable to go
through the other list.
> | + {
> | + /* Get the information about the current filesystem. */
> | + err = ulfs_get_num (i, &ulfs);
> | + if (err)
> | + combined_err = err;
> | +
> | + if ((node_ulfs->port != MACH_PORT_NULL)
>
> What is the rationale for this check -- do we really need it, or won't
> calling file_sync ([invalid port], ...) already do the right thing
> (nothing) and report back a suitable error value? Perhaps I'm missing
> something.
Note that netfs_attempt_sync is meant to work okay on *every* node
handled by unionfs. This means that the situation when one of the
entries in the list of ports is MACH_PORT_NULL is completely normal
and valid (consider the situation when a certain directory is only
present in one of the merged filesystems). I'm strongly inclined to
think that returning an error in a normal situation is not we would
like to have.
> | + && (ulfs->flags & FLAG_ULFS_WRITABLE))
>
> If you got err != 0 you probably shouldn't proceed here to dereference
> ulfs, and instead continue with the next ulfs (as I had it in my
> suggestion).
Sorry, I didn't really notice this detail :-(
Regards,
scolobb
---
netfs.c | 17 ++++++++++++-----
1 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/netfs.c b/netfs.c
index b3174fb..2ca8505 100644
--- a/netfs.c
+++ b/netfs.c
@@ -283,7 +283,8 @@ error_t
netfs_attempt_sync (struct iouser *cred, struct node *np,
int wait)
{
- error_t err = 0;
+ /* The error we are going to report back (last failure wins). */
+ error_t final_err = 0;
/* The index of the currently analyzed filesystem. */
int i = 0;
@@ -300,23 +301,29 @@ netfs_attempt_sync (struct iouser *cred, struct node *np,
place, because now iterating over both lists looks ugly. */
node_ulfs_iterate_unlocked (np)
{
+ error_t err;
+
/* Get the information about the current filesystem. */
err = ulfs_get_num (i, &ulfs);
if (err)
- break;
+ {
+ final_err = err;
+ continue;
+ }
- if (ulfs->flags & FLAG_ULFS_WRITABLE)
+ if ((node_ulfs->port != MACH_PORT_NULL)
+ && (ulfs->flags & FLAG_ULFS_WRITABLE))
{
err = file_sync (node_ulfs->port, wait, 0);
if (err)
- break;
+ final_err = err;
}
++i;
}
mutex_unlock (&ulfs_lock);
- return err;
+ return final_err;
}
/* This should sync the entire remote filesystem. If WAIT is set,
--
1.6.3.3
