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

[Liu Yuan]

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.

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

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

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.

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.

> +	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().

Also, any functions that calls printf should be prefixed as 'sph_' to
distinguish it from normal sheep functions.

printf helpers will print function name, so you don't need to embed
function name in those helpers.

Thanks,
Yuan