Hello! 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.
> > | + /* 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?
> > Also, do you really have to define ulfs -- both ulfs_iterate and
> > node_ulfs_iterate_unlocked do that already, isn't it?
Jeez, sorry, my fault: I was looking at ulfs.h's ulfs_iterate_* stuff
instead of node.h's node_ulfs_iterate_* that you (correctly) use.
> > | + 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.
Alright, I see. Perhaps that warrants a small comment in front of this
check, something like: ``It is perfectly valid that node NP is not
present on every underlying file system.''
> 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?
Regards,
Thomas
signature.asc
Description: Digital signature
