[sheepdog] [PATCH v2 1/4] util: solve cyclic dependency of util.h and logger.h

[MORITA Kazutaka]

At Wed, 10 Jul 2013 23:05:35 +0900,
Hitoshi Mitake wrote:
> 
> At Wed, 10 Jul 2013 15:54:28 +0900,
> MORITA Kazutaka wrote:
> > 
> > At Wed, 10 Jul 2013 10:57:22 +0900,
> > Hitoshi Mitake wrote:
> > > 
> > > At Tue, 9 Jul 2013 17:04:38 +0800,
> > > Liu Yuan wrote:
> > > > 
> > > > On Tue, Jul 09, 2013 at 05:56:44PM +0900, Hitoshi Mitake wrote:
> > > > > At Tue, 9 Jul 2013 16:51:04 +0800,
> > > > > Liu Yuan wrote:
> > > > > > 
> > > > > > On Tue, Jul 09, 2013 at 04:42:00PM +0900, Hitoshi Mitake wrote:
> > > > > > > Current sheepdog specialized assert() in util.h cannot be used by
> > > > > > > util.h itself because it depends on logger.h and logger.h depends on
> > > > > > > util.h, too.
> > > > > > > 
> > > > > > > This patch solves this cyclic dependency.
> > > > > > > 
> > > > > > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> > > > > > > ---
> > > > > > > 
> > > > > > > v2:
> > > > > > >  - stop let treeview.c and shadow_file.c include logger.h
> > > > > > > 
> > > > > > >  include/logger.h |    6 ++++++
> > > > > > >  include/util.h   |   39 ++++++++++++++++++++++++---------------
> > > > > > >  2 files changed, 30 insertions(+), 15 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/include/logger.h b/include/logger.h
> > > > > > > index 5c9aab6..41a1488 100644
> > > > > > > --- a/include/logger.h
> > > > > > > +++ b/include/logger.h
> > > > > > > @@ -81,4 +81,10 @@ void sd_backtrace(void);
> > > > > > >  		log_write(SDOG_DEBUG, __func__, __LINE__, fmt, ##args);	\
> > > > > > >  	} while (0)
> > > > > > >  
> > > > > > > +static inline void do_assert(const char *expr, const char *func, int line)
> > > > > > > +{
> > > > > > > +	log_write(SDOG_EMERG, func, line, "Asserting `%s' failed.", expr);
> > > > > > > +	abort();
> > > > > > > +}
> > > > > > > +
> > > > > > >  #endif	/* LOG_H */
> > > > > > > diff --git a/include/util.h b/include/util.h
> > > > > > > index e70bb9f..1517680 100644
> > > > > > > --- a/include/util.h
> > > > > > > +++ b/include/util.h
> > > > > > > @@ -14,6 +14,23 @@
> > > > > > >  #include "bitops.h"
> > > > > > >  #include "list.h"
> > > > > > >  
> > > > > > > +static inline void do_assert(const char *expr, const char *func, int line);
> > > > > > > +
> > > > > > > +#ifdef assert
> > > > > > > +#undef assert
> > > > > > > +#endif
> > > > > > > +
> > > > > > > +#ifndef NDEBUG
> > > > > > > +
> > > > > > > +#define assert(expr) ((expr) ?				\
> > > > > > > +			(void)0 : do_assert(#expr, __func__, __LINE__))
> > > > > > > +
> > > > > > > +#else
> > > > > > > +
> > > > > > > +#define assert(expr) ((void)0)
> > > > > > > +
> > > > > > > +#endif	/* NDEBUG */
> > > > > > > +
> > > > > > >  #define SECTOR_SIZE (1U << 9)
> > > > > > >  #define BLOCK_SIZE (1U << 12)
> > > > > > >  
> > > > > > > @@ -161,21 +178,6 @@ int atomic_create_and_write(const char *path, char *buf, size_t len);
> > > > > > >  	__removed;							\
> > > > > > >  })
> > > > > > >  
> > > > > > > -#ifdef assert
> > > > > > > -#undef assert
> > > > > > > -#endif
> > > > > > > -
> > > > > > > -#ifndef NDEBUG
> > > > > > > -
> > > > > > > -#define assert(expr) ((expr) ?						\
> > > > > > > -			(void)0 : panic("Asserting `%s' failed.", #expr))
> > > > > > > -
> > > > > > > -#else
> > > > > > > -
> > > > > > > -#define assert(expr) ((void)0)
> > > > > > > -
> > > > > > > -#endif	/* NDEBUG */
> > > > > > > -
> > > > > > >  /* urcu helpers */
> > > > > > >  
> > > > > > >  /* Boolean data type which can be accessed by multiple threads */
> > > > > > > @@ -243,4 +245,11 @@ extern mode_t sd_def_dmode;
> > > > > > >  /* Force a compilation error if the condition is true */
> > > > > > >  #define BUILD_BUG_ON(condition) ((void)sizeof(struct { int: -!!(condition); }))
> > > > > > >  
> > > > > > > +/*
> > > > > > > + * The below including of logger.h is required for suppressing compiler warning
> > > > > > > + * because if .c file include util.h but not logger.h, the prototype
> > > > > > > + * declaration of do_assert() is treated as defined but not used function.
> > > > > > > + */
> > > > > > > +#include "logger.h"
> > > > > > > +
> > > > > > >  #endif
> > > > > > 
> > > > > > This looks too hacky to me, can we find a better method?
> > > > > 
> > > > > I tried to seek other methods (e.g. using gcc extensions) but
> > > > > concluded this solution is an only way to solve the problem.
> > > > > 
> > > > > I think this one is not so hacky because almost every .c file includes
> > > > > both of util.h and logger.h. treeview.c and shadow_files.c are only
> > > > > exceptions.
> > > > 
> > > > What is the problem this patch tries to solve? Not every one including 'util.h'
> > > > want to use assert(). and we don't have problems in the old code, do we?
> > > 
> > > Yes, as you say, not every .c file uses assert(). And the previous
> > > source tree didn't have any problems about it.
> > > 
> > > But this patch changes the definiton of assert(). This is for using
> > > assert() by util.h itself. The new definition of assert() is a simple
> > > macro which wraps the function do_assert() defined in logger.h. And
> > > logger.h includes util.h, the cyclic dependency happens. This patch
> > > tries to solve it.
> > 
> > I think the correct way to go is moving the assert definition from
> > util.h to logger.h.  If the caller doesn't include logger.h, the
> > default behavior of assert (print an error message to stderr) is okay.
> 
> I tried the above strategy but it doesn't work well. Because assert()
> in refcount_dec() is replaced as the standard one. So the assert()
> doesn't print its error message in sheep.log when refcount_dec() is
> called by source code which includes logger.h.
> 
> I think this patch is required.

Including logger.h at the bottom of util.h is completely wrong.  Note
that all the programs don't want their logger.  Please find another
way.

Is it really necessary to include util.h in logger.h?  Looks like that
only __printf is required for logger.h.  How about creating compiler.h
to define compiler definitions like __printf and __packed, and
including it from logger.h instead of util.h?

Thanks,

Kazutaka