At Thu, 17 Jan 2013 17:57:21 +0800,
Liu Yuan wrote:
>
> On 01/17/2013 10:25 AM, Hitoshi Mitake wrote:
> > sheep/cluster/shepherd.c | 704 ++++++++++++++++++++++++++++++++++++
> > shepherd/Makefile.am | 44 +++
> > shepherd/shepherd.c | 883 ++++++++++++++++++++++++++++++++++++++++++++++
>
> Please separate cluster driver and sph daemon as two patches.
OK, I'll separate them in v7.
>
> > +enum sph_msg_type {
> > + _SPH_MSG_PADDING = 0, /* for making first member 1 */
> > + SPH_MSG_JOIN,
>
> simply assign SPH_MSG_JOIN = 1
>
> > + SPH_MSG_JOIN_REPLY,
> > + SPH_MSG_JOIN_RETRY,
> > +
> > + SPH_MSG_NEW_NODE,
> > + SPH_MSG_NEW_NODE_REPLY,
> > + SPH_MSG_NEW_NODE_FINISH,
> > +
> > + SPH_MSG_NOTIFY,
> > + SPH_MSG_NOTIFY_FORWARD,
> > +
> > + SPH_MSG_BLOCK,
> > + SPH_MSG_BLOCK_FORWARD,
> > +
> > + SPH_MSG_LEAVE,
> > + SPH_MSG_LEAVE_FORWARD,
> > +
> > + SPH_MSG_MASTER_ELECTION,
> > +
> > + NR_SPH_MSG, /* this must be last one */
>
> Seems that NR_SPH_MSG isn't used at all. If no outside user, I'd suggest
> remove it.
Yes, NR_SPH_MSG is obsolete. I'll clean the enum based on your comment.
>
> > + iov[0].iov_base = &msg;
> > + iov[0].iov_len = sizeof(msg);
> > +
> > + msg_join = xzalloc(msg_join_len);
> > + msg_join->node = this_node;
> > + memcpy(msg_join->opaque, kept_opaque, kept_opaque_len);
> > +
> > + iov[1].iov_base = msg_join;
> > + iov[1].iov_len = msg_join_len;
>
> I see a lot of iov handling code, I think you'd better add some iov
> helpers to reduce the duplicate code. Also, as a general rule, if we
> have one function larger than *60 lines*, we'd rethink the structure and
> see if we can refactor it into several smaller (possible inline)
> functions that do better self-description.
OK, I'll prepare helpers in lib/.
>
> On 01/17/2013 10:25 AM, Hitoshi Mitake wrote:> +static void
> init_addr_port(const char *s, struct sockaddr_in *addr)
> > +{
> > + /* format: <address>[:<port>] */
> > + char *sep, *copied, *p;
> > + uint8_t tmp_addr[16];
> > + bool addr_only = false;
> > +
> > + copied = strdup(s);
> > + if (!copied)
> > + panic("strdup() failed: %m");
> > +
> > + sep = strchr(copied, ':');
> > + if (!sep) {
> > + addr_only = true;
> > + goto addr_trans;
> > + }
>
> I think strtok will make the parsing easier.
Thanks, I'll use strtok() for init_addr_port().
>
> On 01/17/2013 10:25 AM, Hitoshi Mitake wrote:> +static void
> init_msg_handlers(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(msg_handlers); i++) {
> > + if (msg_handlers[i])
> > + continue;
> > +
> > + msg_handlers[i] = msg_invalid;
> > + }
> > +}
>
> Do we really need this? Simply ignore the invalid message won't cause
> trouble? I think you can re-structure enum sph_msg_type and put those
> that have handlers at the front. Then we don't need install msg_invalid
> handlers for invalid message.
OK, I'll remove init_msg_handlers() completely by reordering the enum.
>
> > + vprintf(SDOG_INFO, "received op: %s\n", sph_msg_to_str(rcv.type));
> > +
>
> If you use SDOG_INFO intensively, I'd suggest add a new helper iprintf().
I agree with your opinion, we need a new helper. But, I think the name
iprintf is not good.
How about this naming scheme:
sd_printf(): for SDOG_INFO
sd_dprintf(): for SDOG_DEBUG
sd_eprintf(): for SDOG_ERROR
sd_vprintf(): takes one argument for forwarding to the first argument
of log_write()
The names dprintf and vprintf are provided by glibc, so we have to
clean them. If it is a good timing, I'll do that.
>
> Also, any functions that calls printf should be prefixed as 'sph_' to
> distinguish it from normal sheep functions.
Like zk_ prefix in zookeeper.c? What is the purpose of this naming
convention? These functions are declared as static, so I think we
don't have to care about their names for distinguishing from sheep
functions. For cscope?
>
> printf helpers will print function name, so you don't need to embed
> function name in those helpers.
Ah, yes, I'll remove the needless fucntion names.
Thanks for your detailed review,
Hitoshi
|
|