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

[Hitoshi Mitake]

At Thu, 18 Jul 2013 13:34:03 +0800,
Liu Yuan wrote:
> 
> 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.

OK, at least in this patch, readability is not a problem. I'll send v2
later.

Thanks,
Hitoshi