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

[Liu Yuan]

On Thu, Jul 18, 2013 at 02:23:47PM +0900, Hitoshi Mitake wrote:
> 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.

In my proposal, this is allowed, the only allowed case.

> 
> 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.
> 

is this example really related? We put journal_descriptor in the journal.c in
order to not referenced by others, this is nothing related to readability.

>
> 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.

We can forget about it even it is declared in the head of f().

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

I think I have stated my idea in the previous mail about this issue, again your
example assume a *big & complex* function f(). We need to repartition the
function into well organized smaller ones to improve readability instead of
delaration in the middle and hope in vain it can improve readability.

Thanks
Yuan