[sheepdog] [PATCH v5 0/4] shepherd: a new cluster manager specialized for sheepdog

[Hitoshi Mitake]

At Wed, 09 Jan 2013 15:29:31 +0800,
Liu Yuan wrote:
> 
> On 01/09/2013 03:09 PM, Hitoshi Mitake wrote:
> >  .gitignore               |    1 +
> >  Makefile.am              |    2 +-
> >  configure.ac             |    3 +-
> >  include/Makefile.am      |    3 +-
> >  include/internal_proto.h |   51 +++
> >  include/net.h            |    1 +
> >  include/shepherd.h       |   78 ++++
> >  include/shepherd_msg.def |   17 +
> 
> I haven't got time to review the internal functions, but for general
> coding style:
>  1 should remove extra white space after 'break;'
>  2 find another better descriptive name for _function_name(). prefix the
> existing function name tell nothing about different between
> _function_name() and function_name().

I agree. Thanks for your pointing.

> 
> And for shepherd_msg.def, don't use it because it obscure 'cscope' and
> introduce tricky macros which isn't necessary.

Adding *.def files to cscope.files is easy, so the cscope problem is
not so serious.

This macro technique is useful for keeping consistency between symbols
and string expressions.

I believe writing both of symbols and its string expressions
separately is not so good because the string expressions can be
generated by the symbols easily. e.g. this technique might also be
useful for the declaration of sd_ops in ops.c.

If this technique is not acceptable absolutely, I'll remove them. But
I believe it is worth to be considered.

> 
> init_msg_handlers() isn't necessary. Refer to zookeeper.c for a static
> handler installation because you don't need dynamic callback
> installation.

Thanks for your advice, I'll do so from next time.

Thanks,
Hitoshi