Libguestfs - Jan 2014 - Re: [PATCH] fuse: provide a stub "flush" implementation (RHBZ#660687).

It seems that FUSE can invoke flush to make sure the pending changes
(e.g. to the attributes) of a file are set. Since a missing flush
implementation is handled as if it were returning ENOSYS, this can cause
issues later.

To overcome this, just provide a stub implementation which does nothing,
since we have nothing to do and don't want to have FUSE error out.

Furthermore, uncomment the timestamp checks in test-fuse.sh, since now
they should be working fine.
---
 fuse/test-fuse.sh | 23 +++++++++++------------
 src/fuse.c        | 15 +++++++++++++++
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/fuse/test-fuse.sh b/fuse/test-fuse.sh
index f1e03d0..30b3c31 100755
--- a/fuse/test-fuse.sh
+++ b/fuse/test-fuse.sh
@@ -220,18 +220,17 @@ if truncate --help >/dev/null 2>&1; then
     rm -f truncated
 fi
 
-# Disabled because of RHBZ#660687 on Debian.
-# stage Checking utimens and timestamps
-# for ts in 12345 1234567 987654321; do
-#     # NB: It's not possible to set the ctime with touch.
-#     touch -a -d @$ts timestamp
-#     [ "$(stat -c %X timestamp)" -eq $ts ]
-#     touch -m -d @$ts timestamp
-#     [ "$(stat -c %Y timestamp)" -eq $ts ]
-#     touch    -d @$ts timestamp
-#     [ "$(stat -c %X timestamp)" -eq $ts ]
-#     [ "$(stat -c %Y timestamp)" -eq $ts ]
-# done
+stage Checking utimens and timestamps
+for ts in 12345 1234567 987654321; do
+    # NB: It's not possible to set the ctime with touch.
+    touch -a -d @$ts timestamp
+    [ "$(stat -c %X timestamp)" -eq $ts ]
+    touch -m -d @$ts timestamp
+    [ "$(stat -c %Y timestamp)" -eq $ts ]
+    touch    -d @$ts timestamp
+    [ "$(stat -c %X timestamp)" -eq $ts ]
+    [ "$(stat -c %Y timestamp)" -eq $ts ]
+done
 
 stage Checking writes
 cp hello.txt copy.txt
diff --git a/src/fuse.c b/src/fuse.c
index 967a744..748b933 100644
--- a/src/fuse.c
+++ b/src/fuse.c
@@ -876,6 +876,20 @@ mount_local_removexattr(const char *path, const char *name)
   return 0;
 }
 
+static int
+mount_local_flush(const char *path, struct fuse_file_info *fi)
+{
+  DECL_G ();
+  DEBUG_CALL ("%s", path);
+
+  /* Just a stub. This method is called whenever FUSE wants to flush the
+   * pending changes (f.ex. to attributes) to a file.  Since we don't have
+   * anything to do and don't want FUSE to think something went badly,
+   * just return 0.
+   */
+  return 0;
+}
+
 static struct fuse_operations mount_local_operations = {
   .getattr	= mount_local_getattr,
   .access	= mount_local_access,
@@ -902,6 +916,7 @@ static struct fuse_operations mount_local_operations = {
   .getxattr	= mount_local_getxattr,
   .listxattr	= mount_local_listxattr,
   .removexattr	= mount_local_removexattr,
+  .flush        = mount_local_flush,
 };
 
 int
-- 
1.8.3.1

On Thu, Dec 12, 2013 at 04:28:31PM +0100, Pino Toscano
wrote:
> It seems that FUSE can invoke flush to make sure the pending changes
> (e.g. to the attributes) of a file are set. Since a missing flush
> implementation is handled as if it were returning ENOSYS, this can cause
> issues later.
> 
> To overcome this, just provide a stub implementation which does nothing,
> since we have nothing to do and don't want to have FUSE error out.
> 
> Furthermore, uncomment the timestamp checks in test-fuse.sh, since now
> they should be working fine.
> ---
>  fuse/test-fuse.sh | 23 +++++++++++------------
>  src/fuse.c        | 15 +++++++++++++++
>  2 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/fuse/test-fuse.sh b/fuse/test-fuse.sh
> index f1e03d0..30b3c31 100755
> --- a/fuse/test-fuse.sh
> +++ b/fuse/test-fuse.sh
> @@ -220,18 +220,17 @@ if truncate --help >/dev/null 2>&1; then
>      rm -f truncated
>  fi
>  
> -# Disabled because of RHBZ#660687 on Debian.
> -# stage Checking utimens and timestamps
> -# for ts in 12345 1234567 987654321; do
> -#     # NB: It's not possible to set the ctime with touch.
> -#     touch -a -d @$ts timestamp
> -#     [ "$(stat -c %X timestamp)" -eq $ts ]
> -#     touch -m -d @$ts timestamp
> -#     [ "$(stat -c %Y timestamp)" -eq $ts ]
> -#     touch    -d @$ts timestamp
> -#     [ "$(stat -c %X timestamp)" -eq $ts ]
> -#     [ "$(stat -c %Y timestamp)" -eq $ts ]
> -# done
> +stage Checking utimens and timestamps
> +for ts in 12345 1234567 987654321; do
> +    # NB: It's not possible to set the ctime with touch.
> +    touch -a -d @$ts timestamp
> +    [ "$(stat -c %X timestamp)" -eq $ts ]
> +    touch -m -d @$ts timestamp
> +    [ "$(stat -c %Y timestamp)" -eq $ts ]
> +    touch    -d @$ts timestamp
> +    [ "$(stat -c %X timestamp)" -eq $ts ]
> +    [ "$(stat -c %Y timestamp)" -eq $ts ]
> +done
>  
>  stage Checking writes
>  cp hello.txt copy.txt
> diff --git a/src/fuse.c b/src/fuse.c
> index 967a744..748b933 100644
> --- a/src/fuse.c
> +++ b/src/fuse.c
> @@ -876,6 +876,20 @@ mount_local_removexattr(const char *path, const char
*name)
>    return 0;
>  }
>  
> +static int
> +mount_local_flush(const char *path, struct fuse_file_info *fi)
> +{
> +  DECL_G ();
> +  DEBUG_CALL ("%s", path);
> +
> +  /* Just a stub. This method is called whenever FUSE wants to flush the
> +   * pending changes (f.ex. to attributes) to a file.  Since we don't
have
> +   * anything to do and don't want FUSE to think something went badly,
> +   * just return 0.
> +   */
> +  return 0;
> +}
> +
>  static struct fuse_operations mount_local_operations = {
>    .getattr	= mount_local_getattr,
>    .access	= mount_local_access,
> @@ -902,6 +916,7 @@ static struct fuse_operations mount_local_operations =
{
>    .getxattr	= mount_local_getxattr,
>    .listxattr	= mount_local_listxattr,
>    .removexattr	= mount_local_removexattr,
> +  .flush        = mount_local_flush,
>  };
>  
>  int
> -- 
> 1.8.3.1
I had to check what file_operations.flush is used for (to check that
it's not fsync), but as you say it's just used to indicate that the
file descriptor has been closed.

So, ACK.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Hi,

On Thursday 12 December 2013 16:28:31 Pino Toscano
wrote:
> It seems that FUSE can invoke flush to make sure the pending changes
> (e.g. to the attributes) of a file are set. Since a missing flush
> implementation is handled as if it were returning ENOSYS, this can
> cause issues later.
> 
> To overcome this, just provide a stub implementation which does
> nothing, since we have nothing to do and don't want to have FUSE
> error out.
> 
> Furthermore, uncomment the timestamp checks in test-fuse.sh, since now
> they should be working fine.
Apparently this way not the right fix for rh#660687 (although the 
addition of the patch did not harm). I should have hopefully found the 
right cause of it, will follow-up with the fix.

-- 
Pino Toscano

Not all the fields of struct stat are actually filled by us. This caused
rubbish to appear in the microseconds fields, which were then used as
base when changing atime/ctime (with e.g. touch), triggering EINVAL by
futimens/utimensat when those rubbish values were out of the range
allowed for microseconds.
---
 src/fuse.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/fuse.c b/src/fuse.c
index 288c02a..dd4f139 100644
--- a/src/fuse.c
+++ b/src/fuse.c
@@ -175,6 +175,7 @@ mount_local_readdir (const char *path, void *buf,
fuse_fill_dir_t filler,
         if (ss->val[i].ino >= 0) {
           struct stat statbuf;
 
+          memset (&statbuf, 0, sizeof statbuf);
           statbuf.st_dev = ss->val[i].dev;
           statbuf.st_ino = ss->val[i].ino;
           statbuf.st_mode = ss->val[i].mode;
@@ -255,6 +256,7 @@ mount_local_getattr (const char *path, struct stat *statbuf)
   if (r == NULL)
     RETURN_ERRNO;
 
+  memset (statbuf, 0, sizeof *statbuf);
   statbuf->st_dev = r->dev;
   statbuf->st_ino = r->ino;
   statbuf->st_mode = r->mode;
-- 
1.8.3.1