[sheepdog] [PATCH] lib: choose a correct output in logger

[Liu Yuan]

On Thu, Jul 18, 2013 at 12:38:33AM +0900, Hitoshi Mitake wrote:
> At Wed, 17 Jul 2013 16:51:57 +0800,
> Liu Yuan wrote:
> > 
> > On Wed, Jul 17, 2013 at 04:48:10PM +0900, Hitoshi Mitake wrote:
> > > At Tue, 16 Jul 2013 15:01:07 +0800,
> > > Kai Zhang wrote:
> > > > 
> > > > 
> > > > On Jul 15, 2013, at 9:03 PM, Hitoshi Mitake <mitake.hitoshi at gmail.com> wrote:
> > > > 
> > > > > Current logger chooses stderr as an output of logs in cases
> > > > > logarea_init() isn't called or default formatter is used.
> > > > > 
> > > > > This patch lets logger choose stderr when a priority of logs is less
> > > > > than or equal to LOG_NOTICE. If the priority is larger than or equal
> > > > > to SDOG_INFO, stdout is used.
> > > > > 
> > > > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> > > > > ---
> > > > > lib/logger.c |    6 ++++--
> > > > > 1 files changed, 4 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/lib/logger.c b/lib/logger.c
> > > > > index 253163d..059140f 100644
> > > > > --- a/lib/logger.c
> > > > > +++ b/lib/logger.c
> > > > > @@ -407,8 +407,10 @@ static notrace void dolog(int prio, const char *func, int line,
> > > > > 		memset(msg, 0, sizeof(struct logmsg));
> > > > > 		init_logmsg(msg, &tv, prio, func, line);
> > > > > 		len = format->formatter(str_final, MAX_MSG_SIZE, msg);
> > > > > -		xwrite(fileno(stderr), str_final, len);
> > > > > -		fflush(stderr);
> > > > > +
> > > > > +		FILE *f = SDOG_INFO <= prio ? stdout : stderr;
> > > > > +		xwrite(fileno(f), str_final, len);
> > > > > +		fflush(f);
> > > > > 	}
> > > > > }
> > > > > 
> > > > 
> > > > It is odd to declare a variable 'f' in the middle of the method.
> > > 
> > > No. We employed gnu99 style in 7141b4535bad, interleaving declarations
> > > and statements are allowed.
> > > 
> > > Declaring variables at top of functions has no meaning. Narrowed
> > > scope of variables improves readability significantly.
> > > 
> > 
> > What do you mean by 'improves readablility significantly'? It seems that you
> > assume we write big complex function, in which case declaration in the middle
> > might help people get the idea what the variable is. But this assumption is
> > fundamentally wrong to me. Trust me, big function will always be harder to
> > understand than partitioning it into smaller well organized functions. Any
> > effort of trying to improve the readability of a big & complex function will be
> > in vain.
> 
> Improvement of readability obtained by smaller scope is not limited to
> big, complex functions. e.g. there are many blocks which has variable
> declarations on top of it. e.g.
> 	for (i = start; i < end; i++) {
> 		size_t nr_oids;
> 		struct sd_node *node = cur + i;
> (from prepare_object_list())

I think head declarations in the block is pretty fine. This is traditional and
well accepted.

> we write variable declarations on top of these blocks because it is a
> efficient way to express that these variables are not used in outside
> of the blocks.
>
> Interleaving of declarations and statements is a natural expansion of
> it. The declaration of *f in my patch expresses *f is not used in the
> predecessor statements. Of course the block is small and its
> readability will not be a problem. But I don't know any reason of why
> we _shouldn't_ write the declaration on the top of the block.

Interleaving of declarations dosen't make any thing better but just put nerve
of someone who is already used to old fationed C89 style on rack.

It is a matter of taste, but if I am not in minority, I'd like to suggest a
restriction on declaration:

1. We should always declare variables at the head of block statements.
2. Only 'for (int i=0;...)' is allowed.

Free style declarations really nerve-racks me a bit.

Thanks
Yuan