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

[Hitoshi Mitake]

At Thu, 18 Jul 2013 10:29:30 +0800,
Liu Yuan wrote:
> 
> On Thu, Jul 18, 2013 at 11:13:44AM +0900, Hitoshi Mitake wrote:
> > At Thu, 18 Jul 2013 00:38:33 +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())
> > > 
> > > 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.
> > 
> > Sorry, the above is my mistake. I don't know any reason of why we
> > _shouldn't_ write the declaration between statements.
> 
> It looks messy to me. What benefit we get from it? Narrower scope doesn't
> improve code readability.

example 1: for (int i = 0; ...) -styled for loop expresses that the
index variable is only used inside the loop. We don't have to care
about i when we are focusing on outside of the loop.

exmaple 2: the definition of struct journal_descriptor is written in
journal.c. From this fact, we can know other .c files don't use the
definition of journal_descriptor. We only have to care about journal.c
when we change the definition of the struct.

Interleaved declarations and statements bring same benefits. Assume
functions like this:
f() {
stmt 1;
stmt 2;
...
stmt N;

dec v1;
stmt N+1;
}

We can forget about v1 when we are focusing on statement 1 - N thanks
to delayed declaration.

I believe this is a big improvement. How do you think?

Thanks,
Hitoshi