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

[Thomas Schwinge]

Hello!

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.


> > 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?

I don't really know the unionfs code, but if these two structures are
always meant to be kept synchronized (which I don't really know, so you'd
have to verify that -- or unify these structures, which was the long-term
plan, isn't it?), then a fatal error (assert) is OK.

But then, node_init_root raises other issues: what is the err check at
the beginning of node_ulfs_iterate_unlocked good for?


> --- a/netfs.c
> +++ b/netfs.c

OK for master, I'd say.


Regards,
 Thomas

signature.asc
Description: Digital signature