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
