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

[Hitoshi Mitake]

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