[PATCH] Implement the sync libnetfs stubs.

[Sergiu Ivanov]

* netfs.c (netfs_attempt_sync): Sync every writable directory
associated with the supplied node.
(netfs_attempt_syncfs): Send file_syncfs to every writable
directory maintained by unionfs.
---

Hello,

On Mon, Oct 26, 2009 at 01:03:29AM +0100, olafBuddenhagen@gmx.net wrote:
> On Mon, Aug 17, 2009 at 11:44:59PM +0300, Sergiu Ivanov wrote:
> 
> > @@ -282,7 +283,45 @@ error_t
> >  netfs_attempt_sync (struct iouser *cred, struct node *np,
> >                 int wait)
> >  {
> > -  return EOPNOTSUPP;
> > +  /* The error we are going to report back (last failure wins).  */
> > +  error_t final_err = 0;
> > +
> > +  /* The index of the currently analyzed filesystem.  */
> > +  int i = 0;
> 
> I think the initialization of "i" should be as close to the loop as
> possible -- after all, it's a loop counter...

I moved it closer to the loop itself, but I didn't move it further
than locking the mutex, because locking the mutex is also a part of
initialization, and I am somehow inclined to keep variable
definitions before operations (but this is subjective).
 
> > +    /* Get the information about the current filesystem.  */
> > +    err = ulfs_get_num (i, &ulfs);
> > +    assert (err == 0);
> 
> Minor nitpick: it's more common to do such checks with "!err".

Fixed.
 
> > +
> > +    /* Since `np` may not necessarily be present in every underlying
> > +       directory, having a null port is perfectly valid.  */
> > +    if ((node_ulfs->port != MACH_PORT_NULL)
> > +   && (ulfs->flags & FLAG_ULFS_WRITABLE))
> 
> Not sure whether I asked this before: is there actually any reason not
> to attempt syncing filesystems without FLAG_ULFS_WRITABLE as well?...
> 
> (I don't know how file_sync() or file_syncfs() bahave on filesystems or
> nodes that really are not writable -- but IIRC that's not what
> FLAG_ULFS_WRITABLE conveys anyways?...)

A quick search didn't reveal any indications about whether these RPCs
should fail on a really read-only filesystem, so, technically, syncing
such filesystems should not be a problem.  At first, I could not see
*conceptual* reasons for syncing directories not marked with
FLAG_ULFS_WRITABLE flag, but I can see one now.  Since this
unionfs-specific flag only influences the work of unionfs, and unionfs
does not control *regular* files in unioned directories, a user may
modify files in directories not marked with FLAG_ULFS_WRITABLE.  On
invocation of file_sync{,fs} on such a directory, these changes should
be expected to be synced, too.  That's why I think I agree with you
and I made unionfs sync every unioned directory.

> > +  /* Sync every writable filesystem maintained by unionfs.
> > +
> > +     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 (netfs_root_node)
> > +  {
> > +    error_t err;
> > +
> > +    /* Get the information about the current filesystem.  */
> > +    err = ulfs_get_num (i, &ulfs);
> > +    assert (err == 0);
> > +
> > +    /* Note that, unlike the situation in netfs_attempt_sync, having a
> > +       null port here is abnormal.  */
> 
> Perhaps it would be helpful to state more explicitely that having a NULL
> port *on the unionfs root node* is abnormal -- I didn't realize this
> point at first.
> 
> (Maybe you should actually assert() this.)

Done.

Regards,
scolobb

---
 netfs.c |   83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 80 insertions(+), 3 deletions(-)

diff --git a/netfs.c b/netfs.c
index 89d1bf6..84bc779 100644
--- a/netfs.c
+++ b/netfs.c
@@ -1,5 +1,6 @@
 /* Hurd unionfs
-   Copyright (C) 2001, 2002, 2003, 2005 Free Software Foundation, Inc.
+   Copyright (C) 2001, 2002, 2003, 2005, 2009 Free Software Foundation, Inc.
+
    Written by Moritz Schulte <moritz@duesseldorf.ccc.de>.
 
    This program is free software; you can redistribute it and/or
@@ -282,7 +283,45 @@ error_t
 netfs_attempt_sync (struct iouser *cred, struct node *np,
                    int wait)
 {
-  return EOPNOTSUPP;
+  /* The error we are going to report back (last failure wins).  */
+  error_t final_err = 0;
+
+  /* The information about the currently analyzed filesystem.  */
+  ulfs_t * ulfs;
+
+  /* The index of the currently analyzed filesystem.  */
+  int i = 0;
+
+  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)
+  {
+    error_t err;
+
+    /* Get the information about the current filesystem.  */
+    err = ulfs_get_num (i, &ulfs);
+    assert (!err);
+
+    /* Since `np` may not necessarily be present in every underlying
+       directory, having a null port is perfectly valid.  */
+    if ((node_ulfs->port != MACH_PORT_NULL)
+       && (ulfs->flags & FLAG_ULFS_WRITABLE))
+      {
+       err = file_sync (node_ulfs->port, wait, 0);
+       if (err)
+         final_err = err;
+      }
+
+    ++i;
+  }
+
+  mutex_unlock (&ulfs_lock);
+  return final_err;
 }
 
 /* This should sync the entire remote filesystem.  If WAIT is set,
@@ -290,7 +329,45 @@ netfs_attempt_sync (struct iouser *cred, struct node *np,
 error_t
 netfs_attempt_syncfs (struct iouser *cred, int wait)
 {
-  return 0;
+  /* The error we are going to report back (last failure wins).  */
+  error_t final_err = 0;
+
+  /* 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 filesystem maintained by unionfs.
+
+     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 (netfs_root_node)
+  {
+    error_t err;
+
+    /* Get the information about the current filesystem.  */
+    err = ulfs_get_num (i, &ulfs);
+    assert (err == 0);
+
+    /* Note that, unlike the situation in netfs_attempt_sync, having a
+       null port on the unionfs root node is abnormal.  */
+    assert (node_ulfs->port != MACH_PORT_NULL);
+    if (ulfs->flags & FLAG_ULFS_WRITABLE)
+      {
+       err = file_syncfs (node_ulfs->port, wait, 0);
+       if (err)
+         final_err = err;
+      }
+
+    ++i;
+  }
+
+  mutex_unlock (&ulfs_lock);
+  return final_err;
 }
 
 /* lookup */
-- 
1.6.4.3