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

[Hitoshi Mitake]

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.

Thanks,
Hitoshi