Move implementation of <file_change_detection.h> into a C file
Commit Message
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
Comments
* Stefan Liebler:
> 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.
Thanks. I believe I fixed up the whitespace issues and pushed the
patch.
Florian
@@ -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)
@@ -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)