Extract stats into their own file by TorunR · Pull Request #129 · inotify-tools/inotify-tools

TorunR · 2021-01-25T10:52:00Z

Hi,

we've extracted some parts concerning the file access event stats into their own file during the course of a scientific study. We want to check with whether this extraction seems sensible for you.

ericcurtin · 2021-01-25T11:19:13Z

I'll take a look later, only just very briefly looked at it. Is this only a refactoring? Any functional changes to point out?

ericcurtin · 2021-01-25T11:20:34Z

What is the scientific study also? I'm trying to learn a little more about what the varying users of this are using the tools for 😄

ericcurtin · 2021-01-25T14:53:12Z

Thanks for making it easy to review, new unique lines:

++ b/libinotifytools/src/Makefile.am
libinotifytools_la_SOURCES = inotifytools.c inotifytools_p.h redblack.c redblack.h stats.c stats.h
++ b/libinotifytools/src/inotifytools.c
#include "stats.h"
int collect_stats = 0;
int init = 0;
void _niceassert( long cond, int line, char const * file,
++ b/libinotifytools/src/inotifytools_p.h
										  /**
* @internal
* Assert that a condition evaluates to true, and optionally output a message
* if the assertion fails.
*
* You should use the niceassert() preprocessor macro instead.
*
* @param  cond  If 0, assertion fails, otherwise assertion succeeds.
*
* @param  line  Line number of source code where assertion is made.
*
* @param  file  Name of source file where assertion is made.
*
* @param  condstr  Stringified assertion expression.
*
* @param  mesg  A human-readable error message shown if assertion fails.
*/
void _niceassert( long cond, int line, char const * file,
                  char const * condstr, char const * mesg );
extern int init;
extern struct rbtree *tree_wd;
++ b/libinotifytools/src/stats.c
#include "stats.h"
++ b/libinotifytools/src/stats.h
#ifndef STATS_H
#define STATS_H
#include "inotifytools.h"
#include "inotifytools/inotify.h"
#include "inotifytools_p.h"
extern int collect_stats;
void record_stats( struct inotify_event const * event );
unsigned int *stat_ptr(watch *w, int event);
watch *watch_from_wd(int wd);
#endif // STATS_H

TorunR · 2021-01-25T15:24:12Z

Hi,

thanks for accepting the Pullrequest. We don't use inotify directly, it rather is an evaluation target for an automated refactoring approach we are developing. We want to extract "modules" based on keyphrases, in this case "stats".
I'd be curious about your opinion, whether an approach like this can be interesting.

ericcurtin · 2021-01-25T15:30:34Z

Could you show the code/script/designs? I guess it worked out ok here.

NiLuJe · 2021-01-27T21:06:51Z

This made libinotifytools' _niceassert symbol public (it was static before this), which breaks static builds because the tools also define a public _niceassert symbol.

/bin/sh ../libtool  --tag=CC   --mode=link gcc -Wall -Wextra -Wshadow -Wpointer-arith -Werror -std=c99 -I../libinotifytools/src -g -O2   -o inotifywait inotifywait.o common.o ../libinotifytools/src/libinotifytools.la 
libtool: link: gcc -Wall -Wextra -Wshadow -Wpointer-arith -Werror -std=c99 -I../libinotifytools/src -g -O2 -o inotifywait inotifywait.o common.o  ../libinotifytools/src/.libs/libinotifytools.a
/usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: ../libinotifytools/src/.libs/libinotifytools.a(inotifytools.o): in function `_niceassert':
inotifytools.c:(.text+0x330): multiple definition of `_niceassert'; common.o:common.c:(.text+0x520): first defined here
collect2: error: ld returned 1 exit status

ericcurtin · 2021-01-27T21:29:53Z

Thanks so much for reporting @NiLuJe ! Two things needed here, I think. Add a statically linked build to our build script and remove the code duplication.

ericcurtin · 2021-01-27T21:42:14Z

Would you mind adding the exact way you build to:

build_and_test.sh

@NiLuJe

and opening a PR?

Running inotifywait 3.22.1.0 on Ubuntu 22.04, I observed a bug apparently due to a bad file descriptor: $ inotifywait -e modify foo Setting up watches. Couldn't watch foo: Bad file descriptor I could also reproduce with the master branch. Investigating further, it appears that inotifytools_init() in inotifytools.c exits immediately after entry, because the global variable "init" is non-null, even though we just called it at the beginning of inotifywait.c's main(), and the global variable is supposed to be initialised at 0. Bisecting showed that this variable used to be static, but was changed at some point to non-static to share it with stats.c, and this is where the bug fist occurs. As far as I understand, the symbol conflicts with the init() function from the program's entry point [0]. When entering inotifytools_init(), it seems that the value for that function is picked up and compared to 0, found non-null, and the initialization function exits, leaving inotify_fd uninitialized (-1) and a bad file descriptor. Renaming the variable to something other than "init" is enough to fix the bug. $ ./src/inotifywait -e modify foo Setting up watches. Watches established <waits> [0] https://www.geeksforgeeks.org/executing-main-in-c-behind-the-scene/ Fixes: dd59c53 ("Extract stats into their own file (inotify-tools#129)")

Running inotifywait 3.22.1.0 on Ubuntu 22.04, I observed a bug apparently due to a bad file descriptor: $ inotifywait -e modify foo Setting up watches. Couldn't watch foo: Bad file descriptor I could also reproduce with the master branch. Investigating further, it appears that inotifytools_init() in inotifytools.c exits immediately after entry, because the global variable "init" is non-null, even though we just called it at the beginning of inotifywait.c's main(), and the global variable is supposed to be initialised at 0. Bisecting showed that this variable used to be static, but was changed at some point to non-static to share it with stats.c, and this is where the bug fist occurs. As far as I understand, the symbol conflicts with the init() function from the program's entry point [0]. When entering inotifytools_init(), it seems that the value for that function is picked up and compared to 0, found non-null, and the initialization function exits, leaving inotify_fd uninitialized (-1) and a bad file descriptor. Renaming the variable to something other than "init" is enough to fix the bug. $ ./src/inotifywait -e modify foo Setting up watches. Watches established <waits> [0] https://www.geeksforgeeks.org/executing-main-in-c-behind-the-scene/ Fixes: dd59c53 ("Extract stats into their own file (#129)")

Running inotifywait 3.22.1.0 on Ubuntu 22.04, I observed a bug apparently due to a bad file descriptor: $ inotifywait -e modify foo Setting up watches. Couldn't watch foo: Bad file descriptor I could also reproduce with the master branch. Investigating further, it appears that inotifytools_init() in inotifytools.c exits immediately after entry, because the global variable "init" is non-null, even though we just called it at the beginning of inotifywait.c's main(), and the global variable is supposed to be initialised at 0. Bisecting showed that this variable used to be static, but was changed at some point to non-static to share it with stats.c, and this is where the bug fist occurs. As far as I understand, the symbol conflicts with the init() function from the program's entry point [0]. When entering inotifytools_init(), it seems that the value for that function is picked up and compared to 0, found non-null, and the initialization function exits, leaving inotify_fd uninitialized (-1) and a bad file descriptor. Renaming the variable to something other than "init" is enough to fix the bug. $ ./src/inotifywait -e modify foo Setting up watches. Watches established <waits> [0] https://www.geeksforgeeks.org/executing-main-in-c-behind-the-scene/ Fixes: dd59c53 ("Extract stats into their own file (inotify-tools#129)")

#170) Running inotifywait 3.22.1.0 on Ubuntu 22.04, I observed a bug apparently due to a bad file descriptor: $ inotifywait -e modify foo Setting up watches. Couldn't watch foo: Bad file descriptor I could also reproduce with the master branch. Investigating further, it appears that inotifytools_init() in inotifytools.c exits immediately after entry, because the global variable "init" is non-null, even though we just called it at the beginning of inotifywait.c's main(), and the global variable is supposed to be initialised at 0. Bisecting showed that this variable used to be static, but was changed at some point to non-static to share it with stats.c, and this is where the bug fist occurs. As far as I understand, the symbol conflicts with the init() function from the program's entry point [0]. When entering inotifytools_init(), it seems that the value for that function is picked up and compared to 0, found non-null, and the initialization function exits, leaving inotify_fd uninitialized (-1) and a bad file descriptor. Renaming the variable to something other than "init" is enough to fix the bug. $ ./src/inotifywait -e modify foo Setting up watches. Watches established <waits> [0] https://www.geeksforgeeks.org/executing-main-in-c-behind-the-scene/ Fixes: dd59c53 ("Extract stats into their own file (#129)")

Extract stats into their own file

35750b8

ericcurtin merged commit dd59c53 into inotify-tools:master Jan 25, 2021

NiLuJe mentioned this pull request Jan 27, 2021

Add static builds to the CI script #131

Closed

amir73il mentioned this pull request May 16, 2021

Fanotify support #134

Merged

Qeole mentioned this pull request Oct 5, 2022

libinotifytools: Rename init variable to fix conflict with entry point #170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

Extract stats into their own file#129

Extract stats into their own file#129
ericcurtin merged 1 commit intoinotify-tools:masterfrom
TorunR:extraction

TorunR commented Jan 25, 2021

Uh oh!

ericcurtin commented Jan 25, 2021

Uh oh!

ericcurtin commented Jan 25, 2021 •

edited

Loading

Uh oh!

ericcurtin commented Jan 25, 2021

Uh oh!

TorunR commented Jan 25, 2021

Uh oh!

ericcurtin commented Jan 25, 2021

Uh oh!

NiLuJe commented Jan 27, 2021 •

edited

Loading

Uh oh!

ericcurtin commented Jan 27, 2021 •

edited

Loading

Uh oh!

ericcurtin commented Jan 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Comments

Conversation

TorunR commented Jan 25, 2021

Uh oh!

ericcurtin commented Jan 25, 2021

Uh oh!

ericcurtin commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericcurtin commented Jan 25, 2021

Uh oh!

TorunR commented Jan 25, 2021

Uh oh!

ericcurtin commented Jan 25, 2021

Uh oh!

NiLuJe commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericcurtin commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericcurtin commented Jan 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ericcurtin commented Jan 25, 2021 •

edited

Loading

NiLuJe commented Jan 27, 2021 •

edited

Loading

ericcurtin commented Jan 27, 2021 •

edited

Loading