[PATCH] Don't stop when syncing a directory returns an error.

[Sergiu Ivanov]

* 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 08:26:18PM +0200, Thomas Schwinge wrote:
> On Fri, Aug 14, 2009 at 07:41:08PM +0300, Sergiu Ivanov wrote:
> > On Fri, Aug 14, 2009 at 05:18:59PM +0200, Thomas Schwinge wrote:
> > > 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).
> 
> Jup, arrived just fine, and I was able to import it into my repository
> with a simple ``git am < [file]''.  Doing it this way, the advantage is
> that all what belongs together can easily be viewed together (that is,
> the discussion and the patch), without having to juggle with two emails
> in parallel.

Great :-) I'll try to remember to always do it in this way.
 
> >    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;
> > +      }
> 
> Looking at ulfs_get_num's implementation I wonder whether we should
> actually report its failing back to the invoker of netfs_attempt_sync?
> Wouldn't ulfs_get_num failing be a sign of corrupted internal state?  So,
> would either a silent continue or even a assert (err == 0) be more
> appropriate here?

You are right -- if a node contains more node_ulfs entries than there
are registered in ulfs_chain, then something has gone seriously
corrupted.  However, I have a question, which is related to
consistency (again *sigh*): ulfs_get_num is invoked in two places
only: in netfs_attempt_sync and in node_init_root (node.c:533 in my
code version).  In node_init_root the return value of ulfs_get_num is
checked in an if statement.  Is it alright to check this value via an
assert in netfs_attempt_sync?  Or should I change the handling of the
return value in node_init_root instead?

Regards,
scolobb

---
 netfs.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/netfs.c b/netfs.c
index b3174fb..e2a6f65 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,27 @@ 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;
+    assert (err == 0);
 
-    if (ulfs->flags & FLAG_ULFS_WRITABLE)
+    /* Since `np` may not necessarily be present in every underlying
+       directory, having a null port is perfectly valid.  */
+    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