Hello! On Fri, Aug 14, 2009 at 04:36:45PM +0300, Sergiu Ivanov wrote: > On Tue, Aug 11, 2009 at 11:42:41AM +0200, Thomas Schwinge wrote: > > This comment is based on the version of the patch that you installed into > > master. (By the way: this commit didn't show up on commit-hurd; I'll > > have a look at that.) > > Is it my duty to look after my commits showing up on commit-hurd? If > so, I'm sorry and please tell me how to do that, because I don't even > know what ``commit-hurd'' is :-(
Heh, no problem: that's simply the commit-hurd mailing list where all
commits (should) show up.
> I agree with your statement about always syncing all nodes and, IIRC,
> antrik has already mentioned that, and I have forgotten about that :-(
>
> I'm rather inclined to implement the ``last one wins'' approach, since
> it really is simpler and more common. Please, take a look at my new
> patch and tell me if it's acceptable.
Why did you send it in a separate message by the way? Anyways, I'll post
my few comments in here.
| --- a/netfs.c
| +++ b/netfs.c
| @@ -282,7 +283,45 @@ error_t
| netfs_attempt_sync (struct iouser *cred, struct node *np,
| int wait)
| {
| - return EOPNOTSUPP;
Your new patch should be based on what is in unionfs' master branch at
the moment, that is, it will be committed on top of the previously
committed patch.
| + error_t err = 0;
Generally, move local variables into the innermost frame where they are
needed. Here, move err into the node_ulfs_iterate_unlocked loop.
| + /* A pessimistic combination (failure wins) of the results of all
| + calls to file_sync. */
| + error_t combined_err = 0;
``combination of'' sounds like ``combined_err |= ...'' (which is wrong,
of course). Perhaps use the name final_err, and adjust the comment
accordingly, perhaps something like: ``The error we're finally going to
report back: the last failure wins.''
| + /* 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?
| + {
| + /* Get the information about the current filesystem. */
| + err = ulfs_get_num (i, &ulfs);
| + if (err)
| + combined_err = err;
| +
| + 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.
| + && (ulfs->flags & FLAG_ULFS_WRITABLE))
If you got err != 0 you probably shouldn't proceed here to dereference
ulfs, and instead continue with the next ulfs (as I had it in my
suggestion).
| + {
| + err = file_sync (node_ulfs->port, wait, 0);
| + if (err)
| + combined_err = err;
| + }
| +
| + ++i;
| + }
| +
| + mutex_unlock (&ulfs_lock);
| + return combined_err;
Regards,
Thomas
signature.asc
Description: Digital signature
