Re: [PATCH] Move implementation of <file_change_detection.h> into a C file

[Stefan Liebler <stli at linux dot ibm dot com>]

Hi Florian,

I've run some builds on my s390x and x86_64 machines with your applied 
patch. I do not see the -Werror=maybe-uninitialized anymore.

Despite of the whitespace issues, this patch looks good to me.

At least the second line of arguments for __file_is_unchanged needs two 
more spaces (see below).

I've also opened include/file_change_detection.h and 
io/file_change_detection.c in my emacs and it showed more whitespace 
errors: tab vs whitespaces.
Please have a look at the attached patch to see what I mean. I'm not 
exactly sure if this is really required. If yes, please also check the 
remaining files in the patch.

On 2/17/20 12:28 PM, Florian Weimer wrote:
> file_change_detection_for_stat partially initialize
> struct file_change_detection in some cases, when the size member
> alone determines the outcome of all comparisons.  This results
> in maybe-uninitialized compiler warnings in case of sufficiently
> aggressive inlining.
>
> Once the implementation is moved into a separate C file, this kind
> of inlining is no longer possible, so the compiler warnings are gone.
>
> (Note that I could not reproduce this warning in my tests.)
>
> -----
>   include/file_change_detection.h | 110 +++++++---------------------------
>   io/Makefile                     |   2 +-
>   io/Versions                     |   4 ++
>   io/file_change_detection.c      | 129 ++++++++++++++++++++++++++++++++++++++++
>   io/tst-file_change_detection.c  |  57 +++++++++---------
>   resolv/res_init.c               |   2 +-
>   resolv/resolv_conf.c            |   6 +-
>   7 files changed, 185 insertions(+), 125 deletions(-)
>
> diff --git a/include/file_change_detection.h b/include/file_change_detection.h
> index aaed0a9b6d..f5220745a9 100644
> --- a/include/file_change_detection.h
> +++ b/include/file_change_detection.h
> @@ -16,9 +16,10 @@
>      License along with the GNU C Library; if not, see
>      <https://www.gnu.org/licenses/>.  */
>   
> -#include <errno.h>
> +#ifndef _FILE_CHANGE_DETECTION_H
> +#define _FILE_CHANGE_DETECTION_H
> +
>   #include <stdbool.h>
> -#include <stddef.h>
>   #include <stdio.h>
>   #include <sys/stat.h>
>   #include <sys/types.h>
> @@ -38,103 +39,32 @@ struct file_change_detection
>   
>   /* Returns true if *LEFT and *RIGHT describe the same version of the
>      same file.  */
> -static bool __attribute__ ((unused))
> -file_is_unchanged (const struct file_change_detection *left,
> -                   const struct file_change_detection *right)
> -{
> -  if (left->size < 0 || right->size < 0)
> -    /* Negative sizes are used as markers and never match.  */
> -    return false;
> -  else if (left->size == 0 && right->size == 0)
> -    /* Both files are empty or do not exist, so they have the same
> -       content, no matter what the other fields indicate.  */
> -    return true;
> -  else
> -    return left->size == right->size
> -      && left->ino == right->ino
> -      && left->mtime.tv_sec == right->mtime.tv_sec
> -      && left->mtime.tv_nsec == right->mtime.tv_nsec
> -      && left->ctime.tv_sec == right->ctime.tv_sec
> -      && left->ctime.tv_nsec == right->ctime.tv_nsec;
> -}
> +bool __file_is_unchanged (const struct file_change_detection *left,
> +                        const struct file_change_detection *right);
Here we need two more spaces before const.


Thanks,
Stefan

diff --git a/include/file_change_detection.h b/include/file_change_detection.h
index f5220745a9..0581d2f584 100644
--- a/include/file_change_detection.h
+++ b/include/file_change_detection.h
@@ -40,25 +40,25 @@ struct file_change_detection
 /* Returns true if *LEFT and *RIGHT describe the same version of the
    same file.  */
 bool __file_is_unchanged (const struct file_change_detection *left,
-                        const struct file_change_detection *right);
+			  const struct file_change_detection *right);
 
 /* Extract file change information to *FILE from the stat buffer
    *ST.  */
 void __file_change_detection_for_stat (struct file_change_detection *file,
-                                       const struct stat64 *st);
+				       const struct stat64 *st);
 
 /* Writes file change information for PATH to *FILE.  Returns true on
    success.  For benign errors, *FILE is cleared, and true is
    returned.  For errors indicating resource outages and the like,
    false is returned.  */
 bool __file_change_detection_for_path (struct file_change_detection *file,
-                                       const char *path);
+				       const char *path);
 
 /* Writes file change information for the stream FP to *FILE.  Returns
    ture on success, false on failure.  If FP is NULL, treat the file
    as non-existing.  */
 bool __file_change_detection_for_fp (struct file_change_detection *file,
-                                     FILE *fp);
+				     FILE *fp);
 
 #ifndef _ISOMAC
 libc_hidden_proto (__file_is_unchanged)
diff --git a/io/file_change_detection.c b/io/file_change_detection.c
index 1ac869c3ec..fcc0705bff 100644
--- a/io/file_change_detection.c
+++ b/io/file_change_detection.c
@@ -25,7 +25,7 @@
    same file.  */
 bool
 __file_is_unchanged (const struct file_change_detection *left,
-                     const struct file_change_detection *right)
+		     const struct file_change_detection *right)
 {
   if (left->size < 0 || right->size < 0)
     /* Negative sizes are used as markers and never match.  */
@@ -48,7 +48,7 @@ libc_hidden_def (__file_is_unchanged)
    *ST.  */
 void
 __file_change_detection_for_stat (struct file_change_detection *file,
-                                  const struct stat64 *st)
+				  const struct stat64 *st)
 {
   if (S_ISDIR (st->st_mode))
     /* Treat as empty file.  */
@@ -72,7 +72,7 @@ libc_hidden_def (__file_change_detection_for_stat)
    false is returned.  */
 bool
 __file_change_detection_for_path (struct file_change_detection *file,
-                                  const char *path)
+				  const char *path)
 {
   struct stat64 st;
   if (stat64 (path, &st) != 0)
@@ -84,13 +84,13 @@ __file_change_detection_for_path (struct file_change_detection *file,
       case ENOENT:
       case ENOTDIR:
       case EPERM:
-        /* Ignore errors due to file system contents.  Instead, treat
-           the file as empty.  */
-        file->size = 0;
-        return true;
+	/* Ignore errors due to file system contents.  Instead, treat
+	   the file as empty.  */
+	file->size = 0;
+	return true;
       default:
-        /* Other errors are fatal.  */
-        return false;
+	/* Other errors are fatal.  */
+	return false;
       }
   else /* stat64 was successfull.  */
     {
@@ -105,7 +105,7 @@ libc_hidden_def (__file_change_detection_for_path)
    as non-existing.  */
 bool
 __file_change_detection_for_fp (struct file_change_detection *file,
-                                FILE *fp)
+				FILE *fp)
 {
   if (fp == NULL)
     {
@@ -117,13 +117,13 @@ __file_change_detection_for_fp (struct file_change_detection *file,
     {
       struct stat64 st;
       if (fstat64 (__fileno (fp), &st) != 0)
-        /* If we already have a file descriptor, all errors are fatal.  */
-        return false;
+	/* If we already have a file descriptor, all errors are fatal.  */
+	return false;
       else
-        {
-          __file_change_detection_for_stat (file, &st);
-          return true;
-        }
+	{
+	  __file_change_detection_for_stat (file, &st);
+	  return true;
+	}
     }
 }
 libc_hidden_def (__file_change_detection_for_fp)