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

[Sergiu Ivanov]

Hello,

On Fri, Aug 14, 2009 at 11:23:22PM +0200, Thomas Schwinge wrote:
> On Fri, Aug 14, 2009 at 10:26:10PM +0300, Sergiu Ivanov wrote:
> > 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:
>
> > > >    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;
> > > > +      }
> 
> By the way: this continue statement would have been erroneous
> nevertheless, as we'd miss the increment of i in this case.

Ah, sure :-(
 
> > > 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?
[...] 
> But then, node_init_root raises other issues: what is the err check at
> the beginning of node_ulfs_iterate_unlocked good for?

I cannot see a motivation for that check being there.  I'd say it
might be a rudiment from the times when the loop contained more
operations at its end.

> > --- a/netfs.c
> > +++ b/netfs.c
> 
> OK for master, I'd say.

In view of what antrik writes in his recent E-mails, I won't commit
this change to master right away.  Rather, I would very much like that
we all come to the same opinion over this question.  Personally, I
would stand for reverting the current ``Implement the sync libnetfs
stubs'' commit and reapplying the corrected version.  I can't see a
reason for keeping two adjacent commits implementing the same
functionality.

What do you think?

Regards,
scolobb